Material-ui: [Chip] Replace :focus with :focus-visible

[cakirpro]

Use-Case: Assume that there are 2 Chips, clickable, which are always visible and used for filtering purposes. Clicking on it adds a check icon to the chip and filters the list, clicking again removes the icon and list is unfiltered.

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Chip background color is not restored to original value after deselecting which confuses the user.

Expected Behavior 🤔

Chip background color should be restored to original value after deselecting.

Steps to Reproduce 🕹

Steps:

1. Visit the demo page https://material-ui.com/components/chips/#chip
2. Click on any of clickable chips => Selected and background color changes
3. Click again to unselect it => Unselected but background color still the same

a) Clicking on the page anywhere cause to change the background color again since focus changes or b) using a custom blur function, which is triggered after a timeout period.

Context 🔦

Your Environment 🌎

Mac OS

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.9.9 |
| React | v16.13.1 |
| Browser | Chrome/Chromium |
| TypeScript | v3.8.3 |

[oliviertassinari]

@cakirpro Thanks for raising, this comes from the use of the pseudo class :focus instead of :focus-visible. The ButtonBase brings this capability, unfortunately, the chip can also render a simple div which prevents us from leveraging the capability of the ButtonBase component.

It seems that we should introduce the usage of the useIsFocusVisible helper.

Do you want to work on it? :)

[oliviertassinari]

Also worth mentioning that #20470 could make the implementation here simpler.

[alexmotoc]

@oliviertassinari I've been having a go at this and think I managed to get the desired behaviour working. When you click on a chip it no longer appears as selected, but when you navigate through them using the keyboard you can clearly see which one is highlighted.

This solution was adapted from the ButtonBase file. I am new to this code base so my approach might not be perfect but I can work on improvements. What's your take on these changes?

diff --git a/packages/material-ui/src/Chip/Chip.js b/packages/material-ui/src/Chip/Chip.js
index 2cb69af4b..791b48ffb 100644
--- a/packages/material-ui/src/Chip/Chip.js
+++ b/packages/material-ui/src/Chip/Chip.js
@@ -8,6 +8,8 @@ import { emphasize, fade } from '../styles/colorManipulator';
 import useForkRef from '../utils/useForkRef';
 import unsupportedProp from '../utils/unsupportedProp';
 import capitalize from '../utils/capitalize';
+import useEventCallback from '../utils/useEventCallback';
+import useIsFocusVisible from '../utils/useIsFocusVisible';
 import ButtonBase from '../ButtonBase';

 export const styles = (theme) => {
@@ -87,7 +89,7 @@ export const styles = (theme) => {
       userSelect: 'none',
       WebkitTapHighlightColor: 'transparent',
       cursor: 'pointer',
-      '&:hover, &:focus': {
+      '&$focusVisible, &:hover': {
         backgroundColor: emphasize(backgroundColor, 0.08),
       },
       '&:active': {
@@ -96,31 +98,31 @@ export const styles = (theme) => {
     },
     /* Styles applied to the root element if `onClick` and `color="primary"` is defined or `clickable={true}`. */
     clickableColorPrimary: {
-      '&:hover, &:focus': {
+      '&$focusVisible, &:hover': {
         backgroundColor: emphasize(theme.palette.primary.main, 0.08),
       },
     },
     /* Styles applied to the root element if `onClick` and `color="secondary"` is defined or `clickable={true}`. */
     clickableColorSecondary: {
-      '&:hover, &:focus': {
+      '&$focusVisible, &:hover': {
         backgroundColor: emphasize(theme.palette.secondary.main, 0.08),
       },
     },
     /* Styles applied to the root element if `onDelete` is defined. */
     deletable: {
-      '&:focus': {
+      '&$focusVisible': {
         backgroundColor: emphasize(backgroundColor, 0.08),
       },
     },
     /* Styles applied to the root element if `onDelete` and `color="primary"` is defined. */
     deletableColorPrimary: {
-      '&:focus': {
+      '&$focusVisible': {
         backgroundColor: emphasize(theme.palette.primary.main, 0.2),
       },
     },
     /* Styles applied to the root element if `onDelete` and `color="secondary"` is defined. */
     deletableColorSecondary: {
-      '&:focus': {
+      '&$focusVisible': {
         backgroundColor: emphasize(theme.palette.secondary.main, 0.2),
       },
     },
@@ -130,7 +132,7 @@ export const styles = (theme) => {
       border: `1px solid ${
         theme.palette.type === 'light' ? 'rgba(0, 0, 0, 0.23)' : 'rgba(255, 255, 255, 0.23)'
       }`,
-      '$clickable&:hover, $clickable&:focus, $deletable&:focus': {
+      '&$focusVisible, $clickable&:hover': {
         backgroundColor: fade(theme.palette.text.primary, theme.palette.action.hoverOpacity),
       },
       '& $avatar': {
@@ -158,7 +160,7 @@ export const styles = (theme) => {
     outlinedPrimary: {
       color: theme.palette.primary.main,
       border: `1px solid ${theme.palette.primary.main}`,
-      '$clickable&:hover, $clickable&:focus, $deletable&:focus': {
+      '&$focusVisible, $clickable&:hover': {
         backgroundColor: fade(theme.palette.primary.main, theme.palette.action.hoverOpacity),
       },
     },
@@ -166,7 +168,7 @@ export const styles = (theme) => {
     outlinedSecondary: {
       color: theme.palette.secondary.main,
       border: `1px solid ${theme.palette.secondary.main}`,
-      '$clickable&:hover, $clickable&:focus, $deletable&:focus': {
+      '&$focusVisible, $clickable&:hover': {
         backgroundColor: fade(theme.palette.secondary.main, theme.palette.action.hoverOpacity),
       },
     },
@@ -260,6 +262,8 @@ export const styles = (theme) => {
         color: theme.palette.secondary.main,
       },
     },
+    /* Pseudo-class applied to the root element if keyboard focused. */
+    focusVisible: {},
   };
 };

@@ -280,6 +284,7 @@ const Chip = React.forwardRef(function Chip(props, ref) {
     component: ComponentProp,
     deleteIcon: deleteIconProp,
     disabled = false,
+    focusVisibleClassName,
     icon: iconProp,
     label,
     onClick,
@@ -294,6 +299,39 @@ const Chip = React.forwardRef(function Chip(props, ref) {
   const chipRef = React.useRef(null);
   const handleRef = useForkRef(chipRef, ref);

+  const {
+    isFocusVisibleRef,
+    onFocus: handleFocusVisible,
+    onBlur: handleBlurVisible
+  } = useIsFocusVisible();
+  const [focusVisible, setFocusVisible] = React.useState(false);
+  if (disabled && focusVisible) {
+    setFocusVisible(false);
+  }
+  React.useEffect(() => {
+    isFocusVisibleRef.current = focusVisible;
+  }, [focusVisible, isFocusVisibleRef]);
+
+  const handleBlur = useEventCallback((event) => {
+      handleBlurVisible(event);
+      if (isFocusVisibleRef.current === false) {
+        setFocusVisible(false);
+      }
+    },
+    false,
+  );
+
+  const handleFocus = useEventCallback((event) => {
+    if (!chipRef.current) {
+      chipRef.current = event.currentTarget;
+    }
+
+    handleFocusVisible(event);
+    if (isFocusVisibleRef.current === true) {
+      setFocusVisible(true);
+    }
+  });
+
   const handleDeleteIconClick = (event) => {
     // Stop the event from bubbling up to the `Chip`
     event.stopPropagation();
@@ -415,6 +453,8 @@ const Chip = React.forwardRef(function Chip(props, ref) {
           [classes[`clickableColor${capitalize(color)}`]]: clickable && color !== 'default',
           [classes.deletable]: onDelete,
           [classes[`deletableColor${capitalize(color)}`]]: onDelete && color !== 'default',
+          [classes.focusVisible]: focusVisible,
+          [focusVisibleClassName]: focusVisible,
           [classes.outlinedPrimary]: variant === 'outlined' && color === 'primary',
           [classes.outlinedSecondary]: variant === 'outlined' && color === 'secondary',
         },
@@ -423,7 +463,9 @@ const Chip = React.forwardRef(function Chip(props, ref) {
       )}
       aria-disabled={disabled ? true : undefined}
       tabIndex={clickable || onDelete ? 0 : undefined}
+      onBlur={handleBlur}
       onClick={onClick}
+      onFocus={handleFocus}
       onKeyDown={handleKeyDown}
       onKeyUp={handleKeyUp}
       ref={handleRef}

[oliviertassinari]

@alexmotoc This looks like a solid start.

I think that we can remove focusVisibleClassName from your diff. It was introduced on the ButtonBase to edge against the classes prop that is not forwarded to the composed component. For instance, if you take the Button, it's built on the ButtonBase. Without focusVisibleClassName Button would need to reintroduce the focus visible hook.
In v5, we can probably remove focusVisibleClassName from the ButtonBase too and have developers use .Mui-focused directly. cc @mnajdova

For the future, I wonder if we couldn't look into exposing our focus visible hook publicly as done in useFocusRing. If we go into this direction I think that we should aim to simplify its usage. After #22102, it starts to be quite a challenge.
I also wonder, what would stop us from using :focus-visible directly? Browser support seems good enough. It's not supported on mobile, but we don't need it there. cc @eps1lon

[alexmotoc]

@oliviertassinari I was considering using :focus-visible in my solution but it appears that there is not much browser support at the moment? For example it does not work on my current Chrome version (85.0.4183.83).

Are there any other major changes I need to make other than removing focusVisibleClassName? If not, I can try writing some tests and open a pull request.

[oliviertassinari]

@alexmotoc Oh, right, I wasn't looking at the right place (caniuse.com was down for me this afternoon) 😅. Ok, so I think that after this issue, it would be interesting to look into how we can expose the hook to the public API :)

[oliviertassinari]

@alexmotoc Do you want to move forward with the diff (and without the focusVisibleClassName prop)? Regarding the hook, I have open #22425 to push the discussion further.

[alexmotoc]

@oliviertassinari Yes, I am on it 👍

[eps1lon]

Click on any of clickable chips => Selected and background color changes

Our clickable Chip does not have a selected state. Visually or otherwise. It implements the focused state which is different. Clicking a focused element does not unfocus it so the behavior with the steps you described is expected.

Wouldn't it be more accurate to say that our Chip is missing the selected state?
Edit: https://material.io/components/chips#action-chips lists the selected state.

[oliviertassinari]

From what I understand, the problem is unrelated to the "selected" state. It's about reproducing the behavior of <Button /> with a clickable chip (that renders as a button), end-users don't want to see the "keyboard focused" state when they click on a button.

[eps1lon]

From what I understand, the problem is unrelated to the "selected" state. It's about reproducing the behavior of <Button /> with a clickable chip (that renders as a button), end-users don't want to see the "keyboard focused" state when they click on a button.

I have only access to what is included in the original issue:

Chip background color should be restored to original value after deselecting.

[oliviertassinari]

@eps1lon Agree, the description of the issue is confusing. My assumption is that the author did his best to describe what was wrong using his "own" terminology. Does it make #22430 an acceptable change (if we ignore the selected state in the equation)?

[eps1lon]

Does it make #22430 an acceptable change (if we ignore the selected state in the equation)?

I don't think so. That PR removes the focused state from the component and adds focus-visible. This could be a fairly invasive breaking change for what is essentially a feature request for a selected state.

[oliviertassinari]

I think that we could extend the scope of the issue to all the components built on top of the ButtonBase. Shouldn't we never use :focus and always :focus-visible for buttons? It would mean extending the current behavior/logic of:

• Button
• PaginationItem
• ListItem
• Link
• Fab
• CardActionArea
• AccordionSummary

to:

• Chip
• BreadcrumbCollapsed

From what I understand, right now, we have :focus to help keyboard users. The purpose of :focus-visible (why it was proposed in the spec) is to be more granular when this style is needed, avoiding confusion as described in the issue.

[alexmotoc]

I had a look over the Material Demo and it seems that the chips do not have a "keyboard focused" state when they are clicked (this is the current behaviour described in this issue). E.g. If I click a chip, the :hover styling should not persist?

I think the terminology used in the issue description may be a bit confusing. The selected state should probably be a different feature.

[eps1lon]

it seems that the chips do not have a "keyboard focused" state when they are clicked

That would be expected. :focus-visible does not apply on click. Only on certain user agents will a click focus a focusable element and apply :focus styles.

[oliviertassinari]

OK so we are good to move #22430 forward and ignore the case of BreadcrumbCollapsed because once we click this button, it disappears?

[eps1lon]

Could somebody summarize what the issue is now? The initial description still talks about selected state.

[oliviertassinari]

@eps1lon My understanding:

• @cakirpro Uses a ✓ in the chip to signal end-users that an item is selected.
• Clicking on a chip sets the keyboard styles and the ✓, which is almost OK-ish (but is already KO from a strict perspective).
• Clicking again on the chip removes the ✓ but keeps the same keyboard style. This time, it's definitely KO and reported in the issue description.

Proposed solution: never set the keyboard style when clicking on a chip, as done with all the other buttons of Material-UI. Chip is the only exception.

[cakirpro]

Hello together,
sorry that I could catch up a bit late.

First of all, if you think everything is working as expected then I'd not want to waste your time, please just close the issue.

Secondly, assume a use case where a) you use the Chips to list some tags where each Chip is used like a "filter", b) you use the Chips as it is, no custom/additional styling. I think it is a valid use case.

the description of the issue is confusing. My assumption is that the author did his best to describe what was wrong using his "own" terminology

It depends from which perspective you look at it. From user's perspective (using the same use-case) a "tag" is selected (clicked/pressed/tapped whatever you mean), afterwards (visually) there is a change ("focused" to be specifically). I am not stating that this must be so or not trying to replace the concept of toggle buttons with Chips but from the user's perspective clicking on an UI Element where visual apperance changes and clicking on it again results in an "expectation" that original appearance is restored.

@oliviertassinari Thank you for your recommendations. As a workaround I removed the hover from the css which in my case solved the problem, I mean fulfilled the expectations.

[eps1lon]

Proposed solution: never set the keyboard style when clicking on a chip, as done with all the other buttons of Material-UI. Chip is the only exception.

That's fine. This will not address the select/de-select state described in the original issue though. Using :focus-visible as a selected indicator is not supported because we don't have any selected state for the Chip at the moment. With the proposed change clicking will no longer have any visual effect.