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

[Modal] Ignore event.defaultPrevented #14991

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 21, 2019

Follow up on #13470 (comment). The change was encouraged by @stevemao.

Breaking change

The new logic closes the Modal even if event.preventDefault() is called on the key down escape event.
event.preventDefault() is meant to stop default behaviors like clicking a checkbox to check it, hitting a button to submit a form, and hitting left arrow to move the cursor in a text input etc. Only special HTML elements have these default behaviors. People should use event.stopPropagation() if they don't want to trigger a onClose event on the modal.

@oliviertassinari oliviertassinari added breaking change component: modal This is the name of the generic UI component, not the React module! labels Mar 21, 2019
@oliviertassinari oliviertassinari changed the title [Modal] Remove the defaultPrevented logic [Modal] Ignore event.defaultPrevented Mar 21, 2019
@eps1lon eps1lon mentioned this pull request Mar 21, 2019
56 tasks
@oliviertassinari oliviertassinari force-pushed the modal-remove-defaultPrevented branch 2 times, most recently from 2d7d029 to dbd7e43 Compare March 21, 2019 12:54
@oliviertassinari oliviertassinari force-pushed the modal-remove-defaultPrevented branch from dbd7e43 to b0e71d5 Compare March 21, 2019 12:56
@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: d1a7d76...b0e71d5

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.01% -0.00% 352,929 352,908 90,642 90,638
@material-ui/core/Paper 0.00% 0.00% 68,626 68,626 19,974 19,974
@material-ui/core/Paper.esm 0.00% 0.00% 62,358 62,358 19,075 19,075
@material-ui/core/Popper 0.00% 0.00% 30,460 30,460 10,528 10,528
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,384 17,384 5,726 5,726
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,037 1,037
@material-ui/lab 0.00% 0.00% 152,181 152,181 44,533 44,533
@material-ui/styles 0.00% 0.00% 53,837 53,837 15,613 15,613
@material-ui/system 0.00% 0.00% 17,138 17,138 4,520 4,520
Button 0.00% 0.00% 90,918 90,918 26,970 26,970
Modal -0.02% -0.04% 84,712 84,691 25,262 25,252
colorManipulator 0.00% 0.00% 3,232 3,232 1,300 1,300
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main -0.00% -0.07% 647,762 647,741 201,135 200,998
packages/material-ui/build/umd/material-ui.production.min.js -0.01% -0.00% 301,958 301,937 83,730 83,727

Generated by 🚫 dangerJS against b0e71d5

@eps1lon
Copy link
Member

eps1lon commented Mar 21, 2019

Like the change but it's probably a bit controversial. Is there some spec that we can point to that defines default behavior of events?

@eps1lon
Copy link
Member

eps1lon commented Mar 21, 2019

I guess https://www.w3.org/TR/uievents/#event-type-keydown will do. No mention about default behavior of the Escape key so preventDefault should indeed not stop the dialog from closing. Nice change 👍

Well spec doesn't help here really:

Default action: [...] activation behavior;
action behavior: The task MAY be defined for that element by the host language, or by author-defined variables, or both. [...]
author: In the context of this specification, an author, content author, or script author is a person who writes script [..]

Essentially means default is what we define default. The default behavior of pressing Escape on an element is not to close a modal because elements are not in a modal by default. It's a bit circular but should suffice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: modal This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants