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

a11y: light & dark mode checkbox doesn't announce state switches #7667

Open
7 tasks done
JoshuaKGoldberg opened this issue Jun 23, 2022 · 16 comments · Fixed by #8174
Open
7 tasks done

a11y: light & dark mode checkbox doesn't announce state switches #7667

JoshuaKGoldberg opened this issue Jun 23, 2022 · 16 comments · Fixed by #8174
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: a11y Related to accessibility concerns of the default theme good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. hacktoberfest

Comments

@JoshuaKGoldberg
Copy link
Contributor

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

Following up from #6665: The color mode toggle now does correctly indicate what its state is with its label. But, when you activate it while using a screenreader, nothing narrates that it switched.

Discussed on stream with @BenDMyers on Twitch: https://www.twitch.tv/videos/1498962341.

Reproducible demo

https://typescript-eslint.io

Steps to reproduce

  1. Activate a screenreader such as NVDA or VoiceOver
  2. Tab over to the light/dark mode toggle
  3. Press Enter to trigger it

Expected behavior

There should be a narration, such as what's provided by aria-pressed.

See more here: https://sarahmhigley.com/writing/playing-with-state

Actual behavior

No narration.

Your environment

Self-service

  • I'd be willing to fix this bug myself.
@JoshuaKGoldberg JoshuaKGoldberg added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Jun 23, 2022
@Josh-Cena Josh-Cena added domain: a11y Related to accessibility concerns of the default theme and removed status: needs triage This issue has not been triaged by maintainers labels Jun 23, 2022
@Josh-Cena
Copy link
Collaborator

I'm not sure how we are to fix this. aria-pressed definitely doesn't seem like a fit. Would aria-live="assertive" help?

@BenDMyers
Copy link
Contributor

aria-pressed could be a fit if the toggle button's name were something like "Enable dark mode" — that way, as you toggle dark mode, you'd get announcements along the lines of "Enable dark mode, pressed" or "Enable dark mode, not pressed." Since aria-pressed is an ARIA state attribute, it should be reliably announced when changed, unlike the accessible name as the linked Sarah Higley article points out.

The button itself probably shouldn't receive aria-live, since you'll have a mismatch of roles. If Docusaurus already has a mechanism for aria-live announcements, that could work.

@Josh-Cena
Copy link
Collaborator

Yes, but our button is light/dark instead of dark/not dark, so pretty sure that's not aria-pressed. I'm just not sure how to force the screen reader to announce the change in button title.

@noobyogi0010
Copy link

@Josh-Cena I would like to work on this issue if no one else is already working on it.
Thanks!

@Josh-Cena
Copy link
Collaborator

@noobyogi0010 Since it hasn't had any activities in the last month, you can assume no-one is working on it and just go ahead.

@noobyogi0010
Copy link

@Josh-Cena Okay, I'll get started with it.
Thanks!

@noobyogi0010
Copy link

@Josh-Cena
Will something like this be a good fix for this issue?
(The red underlined text)
a11yDocusaurusRef

I used aria-pressed and set it as false for dark mode and true for light mode.

mturoci added a commit to mturoci/docusaurus that referenced this issue Oct 4, 2022
slorber added a commit that referenced this issue Oct 5, 2022
…8174)

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
@slorber
Copy link
Collaborator

slorber commented Oct 12, 2022

I think there's an issue with our accessibility fix.

When we navigate from one page to another, triggering a global layout unmount/remount (ie navigating from homepage to docs or to blog), the dark mode switch will announce itself on navigation.

We should probably only make it announce after having been pressed once no?

@slorber slorber reopened this Oct 12, 2022
@mturoci
Copy link
Contributor

mturoci commented Oct 16, 2022

@slorber after a bit of investigation I found out that aria-live regions are reannounced every time the React component is remounted (inserted into the DOM) which makes sense.

The question is why is the ThemeToggle button reinserted on the navigation.

From my quick look into the code, it seems like the whole page is wrapped within react-router. The problem with this approach is that it tears down everything and builds an entirely new tree from the ground up - remounts. This includes layout, nav, and footer despite the content being the only one that needs a real remount. The rest of the components would be fine with just an update. Wrapping just the content part within the react-router would not only solve the issue that you mentioned but could also prevent a lot of bugs in the future + is a nice perf improvement.

Note that the above proposal is a bit naive and doesn't account for any hidden problems that may have led to the current architecture which I would like to learn more about.

@slorber
Copy link
Collaborator

slorber commented Oct 19, 2022

@mturoci the unmount/remount problem is a known problem (#2891) and not so simple to solve in the short term. I'm more looking for a solution considering we won't be able to fix this deeper problem immediately.

The layout should not automatically be applied to all Docusauurus routes: some sites need to have pages with a different layout. And there can also be multiple layout levels (see docs or blog). We should have support for nested routes (like Remix, and soon Next.js have).


I think only adding aria-live=true after the first toggle happens should be good enough temporarily. Good first issue for anyone willing to contribute. Open a PR directly.

@slorber slorber added good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. hacktoberfest labels Oct 19, 2022
slorber added a commit that referenced this issue Oct 28, 2022
…8174)

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
@dawei-wang
Copy link
Contributor

dawei-wang commented Dec 20, 2022

Another problem with the accessibility fix is that the screen reader will continue to announce that the page is in dark mode or light mode for five or six times if the user does not move away from the light/dark mode button. I am able to reduce the announcements to two times if I remove aria-live="polite" from the web inspector. I expected the states to be announced one time on change only.

@slorber
Copy link
Collaborator

slorber commented Dec 22, 2022

Another problem with the accessibility fix is that the screen reader will continue to announce that the page is in dark mode or light mode for five or six times if the user does not move away from the region. I am able to reduce the announcements to two times if I remove aria-live="polite" from the web inspector. I expected the states to be announced one time on change only.

I'm not sure to understand. By chance are you able to record a video or something that could make it clearer?

What do you mean by "move away from the region"

@dawei-wang
Copy link
Contributor

By moving away, I mean focusing the screen reader on another part of the screen. By region, I meant the light/dark mode button.

@dawei-wang
Copy link
Contributor

Here's a video. I hope this makes it clearer:

Docusaurus_screenreader_color_mode_demo.mp4

@Vishrut19
Copy link

Is this issue still open?

@AravindAkuthota
Copy link

@JoshuaKGoldberg i would like to work on this .Is there anyone working on it or would you assign it to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution domain: a11y Related to accessibility concerns of the default theme good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. hacktoberfest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants