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

bug: adding aria labels to host and input element inside shadow root causes issues with nvda #23213

Closed
mwcampbell opened this issue Apr 20, 2021 · 10 comments
Labels
a11y Accessibility related issues package: core @ionic/core package type: bug a confirmed bug report

Comments

@mwcampbell
Copy link

Ionic version:

[ ] 4.x
[x] 5.x

I'm submitting a ...

[x] bug report
[ ] feature request

Current behavior:
The ion-toggle component sets ARIA attributes, such as role and aria-checked, on both the checkbox input element and its container. This means there's more than one switch in the accessibility tree, and only the inner switch can actually receive focus or otherwise handle programmatic interaction from assistive technologies.

Expected behavior:
Only the checkbox input element should have these ARIA attributes. The container should have no ARIA attributes, like most HTML container elements.

Steps to reproduce:
This issue can be seen on the online documentation page for the ion-toggle component.

@ionitron-bot ionitron-bot bot added the triage label Apr 20, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the issue. Can you please clarify what issue this is causing in your application?

Adding these attributes on the host is required for VoiceOver to work properly: https://github.com/ionic-team/ionic-framework/blob/master/.github/COMPONENT-GUIDE.md#example-components-2

Additionally, since ion-toggle is a shadow DOM component, the inner switch should be in its own isolated DOM tree and should not interfere with the host attributes.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 20, 2021
@ionitron-bot ionitron-bot bot removed the triage label Apr 20, 2021
@mwcampbell
Copy link
Author

I haven't yet decided to use Ionic in my own application, because I was reviewing the code for possible accessibility issues first. (Flawless accessibility is a must for any third-party front-end components that I use.) But I verified that the accessibility tree for the ion-toggle documentation page, as rendered by Chromium-based Edge on Windows, includes nodes for both the host element and the child input element. Both of these accessibility nodes have the role for a toggle button, but only the child one is marked as focusable. Meanwhile, the NVDA screen reader only presents the parent (host) element in its browse mode. So there isn't a single, canonical accessible element. This ambiguity is bound to cause problems. For example, if I navigate to the "Blueberry" toggle with NVDA's find command, NVDA puts its cursor on the parent element, and the child doesn't get the keyboard focus. Then if I press Tab, the focus doesn't move to the next toggle as expected, but somewhere else entirely.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 20, 2021
@liamdebeasi
Copy link
Contributor

Could you provide a code sample of the issue with steps to reproduce? Our testing with NVDA did not present these kinds of issues so I am interested in learning more about how we can improve ion-toggle.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 20, 2021
@ionitron-bot ionitron-bot bot removed the triage label Apr 20, 2021
@mwcampbell
Copy link
Author

Rather than write my own code sample, I'll give steps for reproducing the problem using the ion-toggle documentation page.

  1. Start NVDA and a Chromium-based browser (tested with the latest stable versions of Edge and Chrome).
  2. Go to https://ionicframework.com/docs/api/toggle
  3. Press x to jump to the first checkbox (NVDA treats toggles as checkboxes)
  4. Assuming you're using the NVDA desktop keyboard layout, press Insert+NumPad 5 (with NumLock off) to report the current navigator object.
  5. Press Tab.

These steps expose two problems:

  1. The current navigator object should be the BLueberry toggle. Instead, it's the document, which is what has the keyboard focus.
  2. Tab should move to the next toggle. Instead, it jumps to the "Open" link after all of the toggles.

I've verified that neither of these problems happen with bare HTML checkboxes.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 20, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the clear instructions. I was able to reproduce the behavior. I definitely agree that this is undesirable behavior, though the process to fix this is not very clear cut.

What NVDA Does

When you press "X", NVDA grabs the first checkbox/toggle. In this case, it sees <ion-toggle role="switch"> and grabs that. When you press "Tab", Chrome moves focus to the first focusable element, which is the <input type="checkbox"> inside of your ion-toggle. To someone using NVDA, they hear "Blueberry" twice, implying that there are two toggles named "Blueberry".

If we remove aria-label and role from the host, this issue would go away. This leads to the next question: Why do we have it on both the host and the inner input element in the first place? The answer is due to how different browser engines handle focusing with the keyboard. In particular, there is a big difference between Chromium (Chrome, Edge, etc) and WebKit (Safari) that makes this necessary.

Chromium

Chromium considers checkboxes, radios, buttons, and text inputs all focusable via keyboard input. So when you press "Tab" on a page containing an ion-toggle, Chromium will focus the <input type="checkbox"> inside the shadow root of your ion-toggle. This will cause the screen reader to read the aria-label on this input.

WebKit

WebKit considers text inputs focusable, but does not consider checkboxes, radios, and buttons focusable via keyboard input. So when you press "Tab" on a page containing an ion-toggle, nothing will happen because the <input type="checkbox"> does not get focused automatically. When you use VoiceOver in Safari on iOS and you tap the ion-toggle, VoiceOver is going to read the aria-label on the host <ion-toggle> since that is what you just selected. Because the inner <input type="checkbox"> is never focused, the label on that element is never read.

Potential Solution

Unfortunately, we cannot just remove the host aria-label and role attributes altogether as that would break accessibility in Safari on macOS and iOS. One idea I had was to detect if an app is running in a WebKit environment and add these attributes to the host but refrain from adding them if an app is not running in a WebKit environment.

I need to test this idea out of course, but this could be a potential path forward. Let me know if you have any questions. Thanks!

@liamdebeasi liamdebeasi changed the title bug: ion-toggle sets ARIA attributes on both container and checkbox control bug: adding aria labels to host and input element inside shadow root causes issues with nvda Apr 21, 2021
@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Apr 21, 2021
@ionitron-bot ionitron-bot bot removed the triage label Apr 21, 2021
@mwcampbell
Copy link
Author

I have an alternative hypothesis about why the toggle isn't accessible with VoiceOver on iOS when you don't put the ARIA attributes on the host element. When you tap on the toggle component, VoiceOver does a hit test to determine which accessible object it should announce. If the host element is a div or span without any ARIA attributes, then that hit test probably lands on one of the child elements. And I wouldn't be surprised if the hit test ends up on the visible label rather than the hidden input element. So, what happens if you remove the ARIA attributes from the host, and add the aria-hidden flag to all children other than the input element? I don't have an iPhone handy, and I haven't yet set up an Ionic dev environment, so I can't easily test this myself.

Thanks a lot for taking the time to look at this. So many developers seem to just ignore accessibility problems. It's encouraging to know that the Ionic team is really trying to get accessibility right. (Yes, accessibility matters a lot to me, not only as a developer but as a blind person.)

@brandyscarney brandyscarney added this to the a11y milestone Apr 21, 2021
@liamdebeasi
Copy link
Contributor

Ah that's an interesting idea. I am hoping to have some time in the coming days to look at this, so I will test that idea out. I also reached out to some contacts on the WebKit team at Apple to see if they could clarify what is going on under the hood.

Thanks for opening this issue. We really appreciate any community feedback on how we can improve the accessibility of our components!

@liamdebeasi liamdebeasi added the a11y Accessibility related issues label Jul 19, 2021
@liamdebeasi liamdebeasi removed this from the a11y milestone Jul 19, 2021
@liamdebeasi
Copy link
Contributor

Hi there,

We are proposing some changes to form components that seek to resolve this issue. We would love your feedback on these proposed changes! You can read more about the changes and leave comments here: #25660

liamdebeasi added a commit that referenced this issue Nov 29, 2022
resolves #25570, resolves #23213

BREAKING CHANGE:

The `--background` and `--background-checked` variables have been renamed to `--track-background` and `--track-background-checked`, respectively.
@liamdebeasi
Copy link
Contributor

Thanks for the report. In Ionic 7.0 we will be introducing a new ion-toggle syntax that resolves this issue. Details on the syntax change can be found here: #25660

The work for this was completed in #26357, so I am going to close this issue. We will have a public beta period in the future for developers to test and provide feedback on Ionic 7. Please let me know if there are any questions.

@ionitron-bot
Copy link

ionitron-bot bot commented Dec 29, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y Accessibility related issues package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

3 participants