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

Fix keyboard form submission #904

Merged
merged 49 commits into from
Dec 23, 2021
Merged

Fix keyboard form submission #904

merged 49 commits into from
Dec 23, 2021

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Aug 7, 2020 •
edited
Loading

Closes #963
@LFDanLu found a bug where Forms couldn't be submitted by pressing enter or spacebar on the submit button.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu requested changes Aug 7, 2020
View reviewed changes
packages/@react-aria/interactions/src/usePress.ts Outdated Show resolved Hide resolved
packages/@react-aria/interactions/src/usePress.ts Outdated Show resolved Hide resolved
@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Aug 10, 2020
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

devongovett
devongovett reviewed Aug 10, 2020
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does preventDefault only affect keyboard events? What about clicking the button? We also prevent default there right?

Do you think we should do this sort of detection inside usePress, or should we add an option to stop preventing default that useButton could pass?

packages/@react-aria/interactions/src/usePress.ts Outdated Show resolved Hide resolved
@snowystinger
Copy link
Member Author

you would think that clicking would have the same issue, but it doesn't seem to be the case, a press still triggers onSubmit as it is in this PR

as for detection in here, I don't know, what would it look like outside?
would button expose it as a prop?
would it arrive in a button on context?
would a button with type submit / reset just pass it along and never expose it out of the aria hook?

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

ktabors
ktabors previously approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

@adobe-bot
Copy link

Build successful! 🎉

reidbarber
reidbarber previously approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adobe-bot
Copy link

Build successful! 🎉

LFDanLu
LFDanLu previously approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, confirmed that Space and Enter on a Form's submit button are properly causing onSubmit to be fired. LGTM

@adobe-bot
Copy link

Build successful! 🎉

@lottamus
Copy link

@snowystinger thanks for this fix! Can we can merge it in? 🙏

@adobe-bot
Copy link

Build successful! 🎉

@snowystinger
Copy link
Member Author

Sorry, I tried to get it into this coming release but was unsuccessful. I will try to get it in again in the new year. Thanks for the patience.

@adobe-bot
Copy link

Build successful! 🎉

devongovett
devongovett previously approved these changes Dec 23, 2021
View reviewed changes
@devongovett devongovett dismissed stale reviews from LFDanLu, reidbarber, ktabors, and themself via b99f1f3 December 23, 2021 20:55
@adobe-bot
Copy link

Build successful! 🎉

@devongovett devongovett merged commit 320c0b4 into main Dec 23, 2021
@devongovett devongovett deleted the fix-keyboard-form-submission branch December 23, 2021 21:07
@LFDanLu LFDanLu mentioned this pull request Feb 8, 2022
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dannify dannify dannify left review comments

@devongovett devongovett devongovett left review comments

@ktabors ktabors ktabors left review comments

@reidbarber reidbarber reidbarber left review comments

@LFDanLu LFDanLu LFDanLu left review comments

Assignees
No one assigned
Labels
small review Easy to review PR
Projects
None yet
Milestone
No milestone
8 participants
@snowystinger @adobe-bot @lottamus @devongovett @ktabors @reidbarber @LFDanLu @dannify