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

Modal issues with bx--visually-hidden #5545

Closed
tombrunet opened this issue Mar 5, 2020 · 11 comments
Closed

Modal issues with bx--visually-hidden #5545

tombrunet opened this issue Mar 5, 2020 · 11 comments

Comments

@tombrunet
Copy link
Contributor

Environment

Browser: Chrome (probably any)
Automated testing tool and ruleset: N/A, but IBM DAP surfaces the issue through color contrast rules
Assistive technology used to verify: None & JAWS

Detailed description

What version of the Carbon Design System are you using?
See https://react.carbondesignsystem.com/iframe.html?id=modal--default

What's the issue?
Content within the modal marked as bx--visually-hidden is visible to ATs and copyable text, whether the modal is hidden or not.

Steps to reproduce the issue

  1. Open https://react.carbondesignsystem.com/iframe.html?id=modal--default
  2. Select all text
  3. Copy it
  4. Paste it into a text editor
    Note: JAWS sees the same thing, whereas VoiceOver does not handle visibility correctly, so it doesn't announce it.

Result is:

Focus sentinel
Label
Modal heading

Please see ModalWrapper for more examples and demo of the functionality.

Secondary Button
Primary Button
Focus sentinel

The example doesn't give you a way to hide the modal, but if you do hide the modal, and repeat the steps above, you see:

Focus sentinel
Focus sentinel

Note: The focus sentinel doesn't make a lot of sense to me either, but simply highlights the issue of any content marked with bx--visually-hidden

Additional information

There are somewhat two issues here:

  1. What is the point of "Focus Sentinel"
  2. The visibility of the content within the modal after the modal is close. The modal is hidden via CSS visibility: hidden and the bx--visually-hidden uses visibility: visible. For CSS visibility, a visible element within a hidden element is visible. (see https://jakearchibald.com/2014/visible-undoes-hidden/ for an example). display:none is better at hiding all children, but that might affect your animation? It's the safest bet. However, even if you don't go that route, for this particular situation you could do something to override the visibility: visible within a modal, like .modal-hidden .bx--visually-hidden { visibility: hidden } (you don't currently have a .modal-hidden class as far as I can tell though.
@elizabethsjudd
Copy link
Contributor

Historically a couple different things I've done to address very similar problems:

  • Use aria hidden and toggle it's value when the modal shows/hides
  • Use two different classes within the JS. One that handles the display property (as it messes with animations) and then set a 10ms timeout and then apply the other class that makes the animation work.

WH also uses the bx--assistive-text class within Modals as well. This class does the same as bx--visually-hidden so can we make sure both use-cases are addressed with the solution.

@dakahn
Copy link
Contributor

dakahn commented Mar 5, 2020

possibly related #2740

@joshblack
Copy link
Contributor

I believe the fix here is removing the text inside the nodes, correct? Over in: https://github.com/carbon-design-system/carbon/pull/5260/files#diff-2c7f897c16aa710c426b6fd9c3ad155fR241

cc @asudoh

@tombrunet
Copy link
Contributor Author

@joshblack Removing the focus sentinel text fixes the example on the URL, but this was brought to my attention by someone using bx-assistive-text / bx--visually-hidden within the modal for other purposes, so that doesn't fix the larger issue.

@joshblack
Copy link
Contributor

@tombrunet ah got it, sorry I misread this line:

Content within the modal marked as bx--visually-hidden is visible to ATs and copyable text, whether the modal is hidden or not.

Definitely understand now 👍

@tw15egan
Copy link
Collaborator

tw15egan commented Mar 5, 2020

bx-assistive-text / bx--visually-hidden is intended to be used on elements that should be read by a screen reader, correct? i.e hidden labels.

I'm not sure there is a proper way for something to receive focus, but then not be read by a screen reader...Focus sentinel was added (I believe) to fix keyboard focus getting lost when the modal was opened.

According to the fourth rule of ARIA, aria-hidden="true" should not be used on a focusable element.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-hidden_attribute

We definitely should be toggling aria-hidden on the modal though so it is not read when closed.

Were the users just looking for a helper to hide something in the modal completely?

@joshblack joshblack mentioned this issue Mar 5, 2020
82 tasks
asudoh added a commit to asudoh/carbon-components that referenced this issue Mar 5, 2020
This change addresses a common known problem of `visibility:visible`
style; If an element with `visibility:hidden` has a child element with
`visibility:visible`, the child element will be visible even though in
most cases it's expected that the entire content (including the child
element) is hidden.

Refs carbon-design-system#5545.
@asudoh
Copy link
Contributor

asudoh commented Mar 5, 2020

Made a quick fix to visibility:visible: #5552

Wrt focus sentinel; It's for focus-wrap behavior that ensures viewport won't lose the focus before we can bring the focus back in modal. More details can be found at: https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex

The "Focus sentinel" content is a pseudo content, given DAP won't allow empty content there. Let us know if there are any suggestions for better content.

asudoh added a commit that referenced this issue Mar 10, 2020
This change addresses a common known problem of `visibility:visible`
style; If an element with `visibility:hidden` has a child element with
`visibility:visible`, the child element will be visible even though in
most cases it's expected that the entire content (including the child
element) is hidden.

Refs #5545.
aledavila pushed a commit that referenced this issue Mar 18, 2020
This change addresses a common known problem of `visibility:visible`
style; If an element with `visibility:hidden` has a child element with
`visibility:visible`, the child element will be visible even though in
most cases it's expected that the entire content (including the child
element) is hidden.

Refs #5545.
@asudoh
Copy link
Contributor

asudoh commented Mar 19, 2020

I think this issue has been solved, but @tombrunet please let us know if you think otherwise. Thanks!

@asudoh asudoh closed this as completed Mar 19, 2020
@elizabethsjudd
Copy link
Contributor

@asudoh It looks like this hasn't been released yet and https://vanilla.carbondesignsystem.com/ returns a 403 message. Is there an environment where I can test this without having to run it locally?

@asudoh
Copy link
Contributor

asudoh commented Mar 19, 2020

Uh Netlify for vanilla has been killed... I got https://vanilla.carbondesignsystem.com/ back up and running with latest master.

@elizabethsjudd
Copy link
Contributor

Thanks @asudoh. This looks to be resolved @tombrunet. I copied WH's HTML in to the Netlify demo and ran DAP without any color contrast violations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants