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

[useIsFocusVisible] Remove focus-visible if focus is re-targetted #22102

Merged
merged 6 commits into from
Aug 9, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 7, 2020

BREAKING CHANGE

-  const { isFocusVisible, onBlurVisible, ref: focusVisibleRef } = useIsFocusVisible();
+  const {
+    isFocusVisibleRef,
+    onBlur: handleBlurVisible,
+    onFocus: handleFocusVisible,
+    ref: focusVisibleRef,
+  } = useIsFocusVisible();

  const handleFocus = (event) => {
-    if (isFocusVisible(event)) {
+    handleFocusVisible(event);
+    if (isFocusVisibleRef.current === true) {
      setFocusVisible(true);
    }

-    if (focusVisible !== false) {
+    handleBlurVisible(event);
+    if (isFocusVisibleRef.current === false) {
      setFocusVisible(false);
-      onBlurVisible();
    }

Observed this bug a couple of times but wasn't able to understand/reproduce it. Saw it again in #22062 but this time reproducible:

  1. goto https://deploy-preview-22062--material-ui.netlify.app/components/trap-focus/#example
  2. click "open" to open the TrapFocus
  3. reset focus

the focus marker is still focus-visible but focus is on the the focus trap. Only every second click removes the focus-visible style:

demo

Added a test that mimics TrapFocus to prevent future regression.

@eps1lon eps1lon added bug 🐛 Something doesn't work component: ButtonBase The React component. labels Aug 7, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 7, 2020

Details of bundle changes

Generated by 🚫 dangerJS against d3eb864

@eps1lon
Copy link
Member Author

eps1lon commented Aug 7, 2020

Argos failure makes sense. It displayed all items as focus-visible but not it only displayes the last one as focus-visible. I'm pushing a visual change that will create a diff regardless of this bug fix. The diff due to this bug fix is visible in https://www.argos-ci.com/mui-org/material-ui/builds/2968

Though this test revealed another bug that I'll try to fix. It's less

@eps1lon eps1lon force-pushed the fix/ButtonBase/focus-visible-retarget branch from 90c51fd to 6a4ca14 Compare August 7, 2020 19:48
@eps1lon eps1lon added breaking change package: material-ui Specific to @mui/material and removed component: ButtonBase The React component. labels Aug 8, 2020
@eps1lon eps1lon changed the title [ButtonBase] Remove focus-visible if focus is re-targetted [useIsFocusVisible] Remove focus-visible if focus is re-targetted Aug 8, 2020
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Aug 8, 2020
@eps1lon eps1lon merged commit e58cc23 into mui:next Aug 9, 2020
@eps1lon eps1lon deleted the fix/ButtonBase/focus-visible-retarget branch August 9, 2020 09:35
@eps1lon eps1lon added this to the v5 milestone Sep 15, 2020
@oliviertassinari oliviertassinari mentioned this pull request Sep 15, 2020
42 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants