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

JoshuaKGoldberg commented 2 years ago

Have you read the Contributing Guidelines on issues?

[X] I have read the Contributing Guidelines on issues.

Prerequisites

[X] I'm using the latest version of Docusaurus.
[X] I have tried the npm run clear or yarn clear command.
[X] I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
[X] I have tried creating a repro with https://new.docusaurus.io.
[X] 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

Activate a screenreader such as NVDA or VoiceOver
Tab over to the light/dark mode toggle
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

Public source code: https://github.com/typescript-eslint/typescript-eslint
Public site URL: https://typescript-eslint.io/
Docusaurus version used: 2.0.0-beta.21
Environment name and version (e.g. Chrome 89, Node.js 16.4): Chrome 102
Operating system and version (e.g. Ubuntu 20.04.2 LTS): Windows 10

Self-service

[X] I'd be willing to fix this bug myself.

Josh-Cena commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Josh-Cena commented 2 years ago

@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 commented 2 years ago

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

noobyogi0010 commented 2 years ago

@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.

slorber commented 2 years ago

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?

mturoci commented 2 years ago

@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 commented 2 years ago

@mturoci the unmount/remount problem is a known problem (https://github.com/facebook/docusaurus/issues/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.

dawei-wang commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

https://user-images.githubusercontent.com/11358567/209221266-121c645a-6930-4988-a895-5033e438e52f.mp4

Vishrut19 commented 1 year ago

Is this issue still open?

AravindAkuthota commented 2 weeks ago

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

facebook / docusaurus