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

feat(toggle): fixes disabled issue and adds test #13958

Merged

Conversation

AlexanderMelox
Copy link
Contributor

@AlexanderMelox AlexanderMelox commented Jun 8, 2023

Closes #13956

Before
https://github.com/carbon-design-system/carbon/assets/12755042/071a7579-fc21-4b48-b101-2ef6382667a6

After

Screen.Recording.2023-06-08.at.12.04.25.PM.mov

This PR fixes the issue where you are able to still toggle when the Toggle is disabled.

Changelog

Changed

  • packages/react/src/components/Toggle/Toggle.js
  • packages/react/src/components/Toggle/Toggle-test.js

Testing / Reviewing

Checked on storybook if you are able to toggle while disabled. Added test to make sure you are not able to toggle when disabled.

@netlify
Copy link

netlify bot commented Jun 8, 2023

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit 551f369
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/6487452e912086000834d458
😎 Deploy Preview https://deploy-preview-13958--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tw15egan
Copy link
Member

tw15egan commented Jun 8, 2023

I'm also seeing some focus styles being applied when clicking on the disabled Toggle.

I was able to remove those styles locally by changing the selector to this on L77 in _toggle.scss

  .#{$prefix}--toggle__button:not(:disabled):focus
    + .#{$prefix}--toggle__label
    .#{$prefix}--toggle__switch,
  .#{$prefix}--toggle__button:not(:disabled):active
    + .#{$prefix}--toggle__label
    .#{$prefix}--toggle__switch,
  .#{$prefix}--toggle:not(.#{$prefix}--toggle--disabled):active
    .#{$prefix}--toggle__switch {

Seems like the HCM fix at the bottom of the file uses the same selector, so should update that block as well.

@AlexanderMelox
Copy link
Contributor Author

I'm also seeing some focus styles being applied when clicking on the disabled Toggle.

I was able to remove those styles locally by changing the selector to this on L77 in _toggle.scss

  .#{$prefix}--toggle__button:not(:disabled):focus
    + .#{$prefix}--toggle__label
    .#{$prefix}--toggle__switch,
  .#{$prefix}--toggle__button:not(:disabled):active
    + .#{$prefix}--toggle__label
    .#{$prefix}--toggle__switch,
  .#{$prefix}--toggle:not(.#{$prefix}--toggle--disabled):active
    .#{$prefix}--toggle__switch {

Seems like the HCM fix at the bottom of the file uses the same selector, so should update that block as well.

I went ahead and added the changes. I removed the CSS that shows focus and active states when the toggle is disabled. In terms of keyboard navigation should the toggle still have focus even thought it's disabled?

@netlify
Copy link

netlify bot commented Jun 8, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 551f369
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6487452e6b35e2000891f0c2
😎 Deploy Preview https://deploy-preview-13958--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tw15egan
Copy link
Member

tw15egan commented Jun 8, 2023

@AlexanderMelox it seems like something is still in the tab order... it goes from the link in the top right, presumably something inside the toggle (but with no focus state), and then to the controls toggle in the storybook panel. I think we should remove the tab stop rather than add focus when disabled. But it doesn't show up when using voiceover, so I'm not sure where the focus is going. Might be a storybook issue?

Styles look great 👍🏻

@AlexanderMelox
Copy link
Contributor Author

@tw15egan It's just focusing the selector tab at the bottom! So the disabled Toggle is not in the Tab Order :)

Screen.Recording.2023-06-09.at.10.33.05.AM.mov

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

LGTM but seems like 1 tab stop is getting lost, I have to tab twice from the top panel to get to the bottom and it's not clear where the focus is on that "ghost" tab stop

Screen.Recording.2023-06-09.at.10.40.50.AM.mov

@AlexanderMelox
Copy link
Contributor Author

I'm trying to see where the focus can go, we did disabled this so idk if this is enough.

Screenshot 2023-06-09 at 10 52 11 AM

@AlexanderMelox
Copy link
Contributor Author

AlexanderMelox commented Jun 9, 2023

Screenshot 2023-06-09 at 10 54 47 AM Screenshot 2023-06-09 at 10 54 57 AM

So i did some debugging and it looks like focus is still being applied while disabled, we just don't see the styles

@tw15egan
Copy link
Member

tw15egan commented Jun 9, 2023

@AlexanderMelox weird, even setting the tabIndex on the div (and/or button) it still stays in the tab order 🤔

@AlexanderMelox
Copy link
Contributor Author

If we find a solution let me know, I have other things to work on. Thought this would be a quick fix.

@tw15egan
Copy link
Member

tw15egan commented Jun 9, 2023

@AlexanderMelox @francinelucca 🙃 figured it out, it's the storybook iframe taking up a tab stop, so not an issue with the component. Sorry about that! 😓

Fixed an issue with linting on the test, the rest looks great. Thanks for contributing!

@tw15egan tw15egan force-pushed the 13956--toggle-disabled-bug branch from 7c57996 to 41949f7 Compare June 9, 2023 15:59
@AlexanderMelox
Copy link
Contributor Author

The tests are oddly failing

@kodiakhq kodiakhq bot merged commit 5a76564 into carbon-design-system:main Jun 12, 2023
@AlexanderMelox AlexanderMelox deleted the 13956--toggle-disabled-bug branch June 13, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Toggle still toggles while disabled
3 participants