Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Maybe by design] Button of type submit not disabled with isPending at true #6197

Open
alexis-lanoix-sixthfin opened this issue Apr 12, 2024 · 3 comments · May be fixed by #6209
Open

[Maybe by design] Button of type submit not disabled with isPending at true #6197

alexis-lanoix-sixthfin opened this issue Apr 12, 2024 · 3 comments · May be fixed by #6209
Labels
bug Something isn't working

Comments

@alexis-lanoix-sixthfin
Copy link

Provide a general summary of the issue here

Not sure if this is by design, but I thought that with a Button of type submit, having the isPending prop set to true would prevent the form from being submitted but this is not the case.

🤔 Expected Behavior?

With a Button component of type submit and isPending set to true, pressing the button should not submit the form.

😯 Current Behavior

With a Button component of type submit and isPending set to true, pressing the button is triggering a form submit.

💁 Possible Solution

If this is by design, perhaps it should be explicitly explained and emphasized in the documentation.
Like this for example:

Button events are disabled while isPending is true (this does not include submit events, you should use isDisabled for that).

🔦 Context

No response

🖥️ Steps to Reproduce

Preview the console and spam click on the "Click me" button:
https://codesandbox.io/p/sandbox/peaceful-water-mynct4?file=%2Fsrc%2FApp.js

Version

3.32.2

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

Windows 10

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response
@reidbarber
Copy link
Member

reidbarber commented Apr 12, 2024 •
edited

This does look like a bug. I think we're just setting aria-disabled="true" and removing any passed-in event handlers:

react-spectrum/packages/@react-spectrum/button/src/Button.tsx

Lines 36 to 50 in 759ac41

function disablePendingProps(props) {
// Don't allow interaction while isPending is true
if (props.isPending) {
props.onPress = undefined;
props.onPressStart = undefined;
props.onPressEnd = undefined;
props.onPressChange = undefined;
props.onPressUp = undefined;
props.onKeyDown = undefined;
props.onKeyUp = undefined;
props.onClick = undefined;
props.href = undefined;
}
return props;
}

but instead, we could initialize isDisabled as props.isDisabled || props.isPending here:

react-spectrum/packages/@react-spectrum/button/src/Button.tsx

Lines 56 to 66 in 759ac41

let {
elementType: Element = 'button',
children,
variant,
style = variant === 'accent' || variant === 'cta' ? 'fill' : 'outline',
staticColor,
isDisabled,
isPending,
autoFocus,
...otherProps
} = props;

This may have been intentionally avoided though to prevent disabling the button while it is keyboard focused.

Maybe we should instead add event handlers to the button element while in its pending state that prevent the default behavior.

@reidbarber reidbarber added the bug Something isn't working label Apr 12, 2024
@snowystinger
Copy link
Member

This was intentional, setting isDisabled will prevent screen reader users from accessing the button at all, so they won't know there is some long running task they are waiting for, they'll just wonder where the submit button is. It would also lose focus as @reidbarber pointed out.

As @reidbarber suggested, we should probably preventDefault onClick for the pending button so that a form submit isn't called, this could happen even if the form was already submitted, resulting in a second submission. Is that what you're dealing with? Otherwise, if you're applying isPending due to something else, I would recommend not using it except when processing a direct user action on the button.

@alexis-lanoix-sixthfin
Copy link
Author

This was intentional, setting isDisabled will prevent screen reader users from accessing the button at all, so they won't know there is some long running task they are waiting for, they'll just wonder where the submit button is. It would also lose focus as @reidbarber pointed out.

As @reidbarber suggested, we should probably preventDefault onClick for the pending button so that a form submit isn't called, this could happen even if the form was already submitted, resulting in a second submission. Is that what you're dealing with? Otherwise, if you're applying isPending due to something else, I would recommend not using it except when processing a direct user action on the button.

We are currently using isPending to disable the form while processing and sending a request on form submit.
I think it would be nice to prevent a form submit while isPending is true like you are suggesting.

@sookmax sookmax linked a pull request Apr 16, 2024 that will close this issue
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Prevent default when the submit button is clicked while isPending is true sookmax/react-spectrum
3 participants
@snowystinger @reidbarber @alexis-lanoix-sixthfin