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

[Chip] Replace :focus with :focus-visible #22430

Merged
merged 11 commits into from
Sep 9, 2020

Conversation

alexmotoc
Copy link
Contributor

@alexmotoc alexmotoc commented Aug 31, 2020

Fix #22386

Clicking a chip no longer makes it appear selected. The :focus-visible styling only appears when using keyboard navigation.

@alexmotoc
Copy link
Contributor Author

@oliviertassinari At the moment the current test coverage misses line 308. The following test is designed to meet the condition of the if block but the final assertion fails as the focusVisible class is still present. Is there anything I am missing?

diff --git a/packages/material-ui/src/Chip/Chip.test.js b/packages/material-ui/src/Chip/Chip.test.js
index 6d42dc4c8..716362673 100644
--- a/packages/material-ui/src/Chip/Chip.test.js
+++ b/packages/material-ui/src/Chip/Chip.test.js
@@ -554,5 +554,19 @@ describe('<Chip />', () => {
 
       expect(chip).to.have.class(classes.focusVisible);
     });
+
+    it('should reset the focused state', () => {
+      const { container, setProps } = render(<Chip label="Test Chip" onClick={() => {}} />);
+      const chip = container.querySelector(`.${classes.root}`);
+      simulatePointerDevice();
+
+      focusVisible(chip);
+      
+      expect(chip).to.have.class(classes.focusVisible);
+
+      setProps({ disabled: true });
+
+      expect(chip).not.to.have.class(classes.focusVisible);
+    });

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 31, 2020

Details of bundle changes

Generated by 🚫 dangerJS against d2cd1a3

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: chip This is the name of the generic UI component, not the React module! labels Aug 31, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 31, 2020

@alexmotoc Turns out, it was because we didn't forward the disabled prop down to the ButtonBase component. I have pushed the approach one step further. It seems that we can use focusVisibleClassName with no downsides.

@oliviertassinari oliviertassinari force-pushed the chips/focus-visible branch 4 times, most recently from 966903b to c522df2 Compare August 31, 2020 22:03
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I'm missing some context how we went from clicking behavior that's not expected to applying :focus-visible. :focus-visible is not about :focus that is visible but keyboard focus.

It's not clear why a generic div should be focus-visible and it seems to me there are a couple of concepts mixed up here. I'd like to go back to the issue and identify what the problem actually is in the context of #20470

@eps1lon eps1lon changed the title [Chip] Improve support for :focus-visible use case [Chip] Replace :focus with :focus-visible Sep 4, 2020
eps1lon
eps1lon previously requested changes Sep 4, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Looks good. Updating it with the latest commit on next for good measure and then some minor nitpick about naming.

test/utils/focusVisible.ts Outdated Show resolved Hide resolved
@@ -87,7 +87,7 @@ export const styles = (theme) => {
userSelect: 'none',
WebkitTapHighlightColor: 'transparent',
cursor: 'pointer',
'&:hover, &:focus': {
'&$focusVisible, &:hover': {
backgroundColor: emphasize(backgroundColor, 0.08),
Copy link
Member

Choose a reason for hiding this comment

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

Related note: we should probably look into applying https://material.io/design/interaction/states.html. This is would be for another pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting we apply different styling to focus compared with hover?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's something we have started to do, see #10870 for progress.

packages/material-ui/src/Chip/Chip.js Outdated Show resolved Hide resolved
packages/material-ui/src/Chip/Chip.js Show resolved Hide resolved
@oliviertassinari oliviertassinari merged commit 1332b92 into mui:next Sep 9, 2020
@oliviertassinari
Copy link
Member

@alexmotoc Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: chip 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.

[Chip] Replace :focus with :focus-visible
5 participants