Quansight-Labs / czi-scientific-python-mgmt

🐍 Top level project management for Scientific-Python CZI grant at Labs
https://github.com/orgs/Quansight-Labs/projects/11?query=is%3Aopen+sort%3Aupdated-desc
BSD 3-Clause "New" or "Revised" License
4 stars 1 forks source link

The light/dark toggle button does not communicate its state #76

Open gabalafou opened 3 months ago

gabalafou commented 3 months ago

It communicates it visually but not through the accessibility tree

trallard commented 3 months ago

Not sure if we should also address https://github.com/pydata/pydata-sphinx-theme/discussions/1491

Carreau commented 2 months ago

It communicates it visually but not through the accessibility tree

I'm not sure I completely understand, can you expand on what should be done here, or how that should affect the accessibility tree ?

gabalafou commented 2 months ago

We went over this is in the team sync yesterday.

Reiterating here, for reference, the issue is that when you cycle through the three states (light, dark, auto) of the light/dark button (which is in the top right of the PST docs), the current state of the button is reflected visually, but nowhere is this visual state communicated in a textual way in the DOM. For assistive tech, it's important that the current state of the button is reflected in the accessibility tree.

Carreau commented 2 months ago

Thanks, I managed to see the accessibility tree in my dev console and will look into modifying it when the button switches.

Carreau commented 2 months ago

I'm reading the various aria things, and I'm wondering if the theme switch button should be an option dropdown with 3 options (auto, light, & dark).

It seem like a 3 state button is a bit of a hack (and maybe less discoverable than an option?)

I think the UI could be almost identical, but making it an option would maybe solve most of our Accessibility concerns.

Thoughts ?

Carreau commented 2 months ago

Also, I'm not sure we can do anything abut that, but the button tooltip on hover/focus seem to be added/removed by js (via bootstrap?), which removes and add a new aria-describedby attribute on every hover.

Should this aria-describedby (and tooltip element) always be present, but just visually hidden to be properly supported by screen readers ?

Carreau commented 2 months ago

be an option dropdown

And I suppose I mean a select

Carreau commented 2 months ago

Speaking of, this is what MDN is doing, I'm trying to see if we can set aria-description anyway on the the body element, but that does not seem to work. There is no way to directly modify the accessibility tree.

Screenshot 2024-07-08 at 15 33 03
Carreau commented 2 months ago

Ok, so select won't work because we can't style it with icons. So we have a to do a custom dropdown which does not fix our accessibility issue. aria-desciption does no show up in the accessibility tree, and aria-label is already set to "light/dark" translatable string.

I guess we can change the aria label to a dynamic string like : "Switch theme. Current theme : %s". though it's unclear how to translate a dynamic js string. I also realize that "auto" is unclear. MDN uses "Os default" as a description.

gabalafou commented 2 months ago

Related:

gabalafou commented 2 months ago

Responding to some of your points:

gabalafou commented 1 month ago

Some thoughts from today's call: