-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
RFC: stopPropagation vs. preventDefault for escape #33750
base: master
Are you sure you want to change the base?
Conversation
📊 Bundle size report✅ No changes found |
Pull request demo site: URL |
- Interop with v8, v0, the HTML `<dialog>` element, and other non-Fluent controls | ||
- No way to opt out of stopping propagation for escape | ||
|
||
## Open Issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Also good to mention: when we used to have stopPropagation
prior to #29262, every once in a while we would have to pair with some lost engineer on TMP side that didn't know why the click handler doesn't work when a dialog/popover is closed.
- Follows the most common established pattern | ||
- Interop with v8, v0, the HTML `<dialog>` element, and other non-Fluent controls | ||
- No way to opt out of stopping propagation for escape | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Higher level actors on the page might lose information as we're stopping the emission of an event. There were cases where this was affecting telemetry tools for example. |
Added an RFC for the
stopPropagation
vs.preventDefault
question for the escape key, kicked off by a question from OWA.The goal is also to get documentation into our storybook if we keep the
preventDefault
approach, both to have something we can refer to, and to give teams a heads up that they need to handle it themselves.(cc/ @bsunderhus)