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

fix: fixed VO and JAWS problem in FF and Safari #14156

Merged
merged 14 commits into from
Jul 13, 2023

Conversation

guidari
Copy link
Contributor

@guidari guidari commented Jul 7, 2023

Closes #13888

Added tabIndex={-1} to avoid that screen readers passes throught it.

I tested only with VO, but it should work on JAWS also.

Testing / Reviewing

  • Open the FireFox and Safari
  • Open Modal component
  • Tab/Arrow in the component

You should expect the same behaivor as Chrome in both browsers

@guidari guidari requested a review from a team as a code owner July 7, 2023 17:51
@guidari guidari requested review from tw15egan and francinelucca July 7, 2023 17:51
@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 9734893
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/64a85099b768c40008ee7cd2
😎 Deploy Preview https://deploy-preview-14156--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 configuration.

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 9734893
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64a850991087630008735322
😎 Deploy Preview https://deploy-preview-14156--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 configuration.

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit e95d991
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/64aff53d756b690008eff2aa
😎 Deploy Preview https://deploy-preview-14156--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 configuration.

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit e95d991
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/64aff53d756b690008eff2af
😎 Deploy Preview https://deploy-preview-14156--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 configuration.

@tw15egan
Copy link
Collaborator

tw15egan commented Jul 7, 2023

The issue with this is that it breaks the focus wrap behavior, looks like the tab order now goes outside of the modal. We need to be able to focus these elements for the focusWrap behavior to work unfortunately

@guidari
Copy link
Contributor Author

guidari commented Jul 7, 2023

The issue with this is that it breaks the focus wrap behavior, looks like the tab order now goes outside of the modal. We need to be able to focus these elements for the focusWrap behavior to work unfortunately

Ohh! I got it

francinelucca
francinelucca previously approved these changes Jul 7, 2023
@guidari
Copy link
Contributor Author

guidari commented Jul 10, 2023

@tw15egan I paired up with Taylor on that so we could tested using JAWS.

Thanks @tay1orjones for pairing up!

@tay1orjones
Copy link
Member

Just pushed up an update from @guidari and I's pairing session - we realigned the initial focus behavior to more closely match what's happening in Modal.

The root issue that we resolved here was that the modal was rendering, focusing the initial element (input), and then re-rendering which was causing the focus on the input to be lost and revert to the modal container/focus sentinel.

The changes ensure that isOpen is a dependency of the useEffect housing initial focus behavior. Focus is now correctly placed on the first input and the focus sentinel is no longer read in FF using JAWS.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

This also moves the focus sentinels outside of the dialog element and ensures they have tabIndex={0}

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.

The VO seems to be reading out the focus sentinels to me on safari, is that correct?

Screen.Recording.2023-07-10.at.12.51.40.PM.mov

@guidari
Copy link
Contributor Author

guidari commented Jul 10, 2023

I'm not able to replicate that @francinelucca 🤔

I'm in Safari using the VO

https://deploy-preview-14156--carbon-components-react.netlify.app/iframe.html?id=components-composedmodal--with-state-manager&viewMode=story

Edit: Now I can replicate it

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Confirmed JAWS 2023 in firefox does not read focus sentinels 👍

@guidari guidari requested a review from francinelucca July 12, 2023 14:13
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 in composedModal, I do see the same issue in Modal though, could we port this fix over to Modal? or open another issue for Modal?

@guidari
Copy link
Contributor Author

guidari commented Jul 12, 2023

LGTM in composedModal, I do see the same issue in Modal though, could we port this fix over to Modal? or open another issue for Modal?

We basically copy a few things from Modal to fix ComposedModal 😅.
I tried to port the differences between those to test if would fix Modal, but it didn't. It might be worth open an issue to spend more time in it.

I can open it and take a look

@guidari guidari enabled auto-merge (squash) July 12, 2023 17:29
@guidari guidari merged commit ef1336a into carbon-design-system:main Jul 13, 2023
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.

[a11y]: ComposedModal [all variants] - identifies visually hidden controls
4 participants