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

[core] Prepare focus visible polyfill in ref phase #15851

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented May 25, 2019

Moves the the setup of the focus-visible polyfill from the passive? side-effect phase into the ref phase. This has some benefits:

  1. As far as I know useEffect will have a lower priority than user inputs in concurrent react. This would mean that potentially no focus visible styling is applied on the first user inputs.
  2. No more false positive errors in test environments without fully mocked host instances (e.g. react-test-renderer snapshots)
  3. In case the host instance changes to one with another ownerDocument (no idea how this would look) we get the polyfill properly setup. This was not the case previously.

However this means that setup of the event listeners is now synchronously. Once the scheduler is stable we should schedule this with a user input priority.

Closes #15598 (actually),
Closes #15842

Edit:
Another drawback: In case a function component is passed to to component prop the focus-visible polyfill won't work. It will trigger warnings but no more errors. An error would be preferable but I don't think there is a deterministic way to check if the ref was attached to a proper component.

@eps1lon eps1lon added component: tooltip This is the name of the generic UI component, not the React module! component: ButtonBase The React component. labels May 25, 2019
@mui-pr-bot
Copy link

@material-ui/core: parsed: -0.09% 😍, gzip: -0.16% 😍
@material-ui/lab: parsed: -0.21% 😍, gzip: -0.34% 😍

Details of bundle changes.

Comparing: e54cea1...41c8dba

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.09% -0.16% 315,971 315,678 86,587 86,445
@material-ui/core/Paper 0.00% 0.00% 67,870 67,870 20,158 20,158
@material-ui/core/Paper.esm 0.00% +0.01% 🔺 61,152 61,152 18,947 18,948
@material-ui/core/Popper 0.00% -0.06% 28,740 28,740 10,352 10,346
@material-ui/core/Textarea 0.00% -0.08% 5,513 5,513 2,378 2,376
@material-ui/core/TrapFocus 0.00% -0.32% 3,744 3,744 1,580 1,575
@material-ui/core/styles/createMuiTheme 0.00% -0.02% 15,960 15,960 5,782 5,781
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 975 975
@material-ui/lab -0.21% -0.34% 139,101 138,808 42,772 42,625
@material-ui/styles 0.00% 0.00% 51,353 51,353 15,176 15,176
@material-ui/system 0.00% -0.07% 14,458 14,458 4,184 4,181
Button -0.29% -0.54% 84,061 83,816 25,582 25,443
Modal 0.00% -0.04% 20,344 20,344 6,684 6,681
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 55,992 55,992 14,065 14,065
docs.main -0.04% -0.06% 645,692 645,405 203,073 202,946
packages/material-ui/build/umd/material-ui.production.min.js -0.11% -0.19% 294,911 294,598 84,103 83,939

Generated by 🚫 dangerJS against 41c8dba

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Nice!

@eps1lon eps1lon merged commit d7d87a6 into mui:master May 25, 2019
@eps1lon eps1lon deleted the fix/ButtonBase/focus-visible-prepare-ref branch May 25, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonBase The React component. component: tooltip 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.

Jest + react-test-renderer snapshot testing fails with ButtonBase ButtonBase ownerdocument error
3 participants