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.

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

[react-interactions] Allow event.preventDefault on LegacyPress responder #17113

Merged
merged 1 commit into from
Oct 17, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Oct 16, 2019

This PR brings back event.preventDefault behavior that and removes the previous warning for its usage. Whilst this might not be the right long term strategy, this change unblocks internal cases where the native events need to be prevented to handle edge-cases that are exposed because of the mixing of event systems and quirks with the systems in place. Furthermore, this is intentionally only for the LegacyPress responder.

[react-interactions] Allow event.preventDefault on LegacyPress responder
@sizebot
Copy link

sizebot commented Oct 16, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 29882d5

@necolas
Copy link
Contributor

necolas commented Oct 16, 2019

this change unblocks internal cases where the native events need to be prevented to handle edge-cases

What are those? And why did previous edge-cases not require this?

@trueadm
Copy link
Contributor Author

trueadm commented Oct 16, 2019

@necolas See internal task 55775588. I've also had another team come to me and want similar, to call preventDefault to stop text selection from changing when clicking an element that gains focus on click (like button elements).

@trueadm trueadm merged commit 8facc05 into facebook:master Oct 17, 2019
@trueadm trueadm deleted the legacy-press-prevent-default branch October 17, 2019 08:21
@necolas
Copy link
Contributor

necolas commented Oct 17, 2019

LegacyPress is likely too buggy for continued internal use and we should move them over to the Press module. So we might need to add this to other responders in that case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants