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(Combobox): improve screen reader experience for Select-Only combobox w NVDA #1987

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Dec 11, 2024

Description

This update fixes how our Comboboxes interact with NVDA during animations. Previously, NVDA users might have experienced incomplete or inaccurate announcements of the selected option when the dropdown list was animating. This was due to the way NVDA processes rapid changes in the page, which sometimes led to announcements being out of sync with the visual state.

Detail

The problem

When interacting with the Select-Only Combobox, NVDA's announcement of the selected option (e.g., "Label combo box Poppy collapsed opens list") is often interrupted by an incorrect announcement of a now-inactive option previously referenced by aria-activedescendant (e.g., "Poppy Lee 4 of 16").

Analysis revealed that the dropdown animation is interfering with NVDA’s event queue, causing it to announce outdated or incorrect information. Disabling the animation did resolve the issue, allowing NVDA to announce the correct selected option without interruption, but another solution was preferred.

Screen.Recording.2024-12-03.at.11.48.28.AM.mov

To further demonstrate the problem, the issue was reproduced in a modified version of the ARIA Authoring Practices Guide (APG) Select-Only Combobox example by adding a slide animation to the dropdown. The animation caused NVDA to fail in announcing the correct selection.

Screen.Recording.2024-12-10.at.1.43.25.PM.mov

The solution

To address this, we now apply an aria-hidden="true" attribute to the Listbox immediately when it is set to collapsed and at the start of the animation sequence. This prevents screen readers from processing its contents during the animation. Now NVDA correctly process and announce the selected option without interruptions.

Screen.Recording.2024-12-09.at.2.08.39.PM.mov

Regression testing

The fix was tested across various screen readers to ensure compatibility and continued functionality:

JAWS: Announcements work as expected.

Screen.Recording.2024-12-09.at.2.02.20.PM.mov

VoiceOver: Announcements work as expected.
[Video coming soon]

Checklist

  • [] 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ⚫ renders as expected in dark mode
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

brilliant @ze-flo 🙌

@@ -22,6 +22,9 @@ import { ThemeContext } from 'styled-components';
import { DEFAULT_THEME } from '@zendeskgarden/react-theming';
import { composeEventHandlers } from '@zendeskgarden/container-utilities';

/**
* 1. Hide from NVDA when collapsed to avoid incorrect / missing announcements caused by animation
Copy link
Member

Choose a reason for hiding this comment

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

[nit] this comment could be inlined – the header notation is more of a thing within CSS view components 😉

@ze-flo ze-flo marked this pull request as ready for review December 12, 2024 17:35
@ze-flo ze-flo requested a review from a team as a code owner December 12, 2024 17:35
@ze-flo
Copy link
Contributor Author

ze-flo commented Dec 12, 2024

@jgorfine-zendesk Any objections to merging this fix?

@jgorfine-zendesk
Copy link
Contributor

jgorfine-zendesk commented Dec 12, 2024

Nope, none from me, @ze-flo. Having watched your before & after videos, I'd say this is sounding pretty great. 🙌

Just to confirm: there's no scenario in which there's no animation, right? i.e., we're not unsetting the animation when we detect prefers-reduced-motion: reduce, or anything like that? I imagine the aria-hidden="true" would still have the same effect, even if there weren't an animation, but I figured it's worth asking if it should be conditional (and kick in only if there's an animation). 👀

@ze-flo
Copy link
Contributor Author

ze-flo commented Dec 12, 2024

Nope, none from me, @ze-flo. Having watched your before & after videos, I'd say this is sounding pretty great. 🙌

Just to confirm: there's no scenario in which there's no animation, right? i.e., we're not unsetting the animation when we detect prefers-reduced-motion: reduce, or anything like that? I imagine the aria-hidden="true" would still have the same effect, even if there weren't an animation, but I figured it's worth asking if it should be conditional (and kick in only if there's an animation). 👀

I'm glad you're bringing that up. Currently, we do not use prefers-reduced-motion: reduce to remove animations anywhere. However, it would be beneficial to add it to files like menuStyles to improve the experience for some users. I'll raise it with the team.

For peace of mind, I've tested the current aria-hidden logic on all supported SR's with and without animation. All good! 🙌 ✅

@ze-flo ze-flo merged commit 0d1817e into main Dec 12, 2024
8 checks passed
@ze-flo ze-flo deleted the ze-flo/combobox-nvda-fix branch December 12, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants