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

aria fast follow #722

Merged
merged 5 commits into from
Jan 30, 2022
Merged

aria fast follow #722

merged 5 commits into from
Jan 30, 2022

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 29, 2022

This allows the user to set an explicit ariaLabel and ariaDescription for each axis, e.g.: ariaLabel: "y-axis, percentages", ariaDescription: "vertical axis representing change in percentage, from 0% to 25%"

The default is y-axis as before At first I added the label; but we'd have wanted to remove the ↑ and → etc; see #702

The label itself is hidden (since redundant with the item above); maybe controversial since users might want to know what this text is, even if it's redundant?

Adds role=none to the tick lines and grid lines (removes a lot of noise, especially since ticks are interspersed with tick values, and so you can't listen to tick values without hearing an empty "image" for each of them). (removed, see below)

I haven't pushed the tests since all charts are affected and will be affected again when we fine-tune.

https://user-images.githubusercontent.com/7001/151651375-fb87d135-9be4-4a02-a0f3-c8f2581ac00f.mov

…e the label

add role=none to the tick lines and grid lines
@Fil Fil requested a review from mbostock January 29, 2022 06:57
@Fil Fil marked this pull request as draft January 29, 2022 06:57
@Fil
Copy link
Contributor Author

Fil commented Jan 29, 2022

I think I was wrong about role=none; there is an option in VoiceOver Utility > Web that allows the user to ignore images without a description. When this option is selected, the tick lines are not spoken any more.

@mbostock
Copy link
Member

ariaLabel: "vertical axis representing change in percentage, from 0% to 25%"

According to my (neophyte) understanding of ARIA, this would be more appropriate as an aria-description rather than an aria-label. Per MDN:

The aria-description attribute is similar to aria-label in that both provide a text string to associate with the element, but a label should be short and concise, while the description can be longer as it is intended to provide more context and information.

So, aria-label should be “y-axis”, but aria-description can be “a vertical axis representing change in percentage, from 0% to 25%”. We could/should add an ariaDescription option for axes.

I would prefer to move the default logic for ariaLabel up to the constructor, rather than doing it in render. I will open a PR with these suggestions.

Note that in the case where there is a visible label, we’re supposed to use aria-labelledby to point to that label. But that sure is a pain because it means we need to allocate a globally unique id! Per MDN:

The purpose of aria-label is the same as aria-labelledby. Both provide an accessible name for an element. If there is no visible name for the element you can reference, use aria-label to provide the user with a recognizable accessible name. If the label text is available in the DOM, and referencing the DOM content and acceptable user experience, prefer to use aria-labelledby. Don't include both. If both are present on the same element, aria-labelledby will take precedence over aria-label.

But in addition to being a pain to implement, I think our aria-label here will be better than aria-labelledby because we can include “x-axis” or “y-axis” in the label, which is not wanted for the visible label.

I don’t think we need/want the aria-hidden for the visible label. I think we should leave that alone.

@mbostock
Copy link
Member

I would prefer to move the default logic for ariaLabel up to the constructor, rather than doing it in render.

Ah, boo, this won’t work because we use mutation here e.g.:

axis.label = inferLabel(channels, scale, axis, "x");

@Fil Fil marked this pull request as ready for review January 30, 2022 16:55
@Fil Fil merged commit ef85327 into main Jan 30, 2022
@Fil Fil deleted the fil/aria-ff branch January 30, 2022 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants