Use aria-describedBy to communicate border button style and color state #54469
+65
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeryj
added
[Focus] Accessibility (a11y)
alexstine
reviewed
Sep 14, 2023
| </span> | ||
| </Button> | ||
| <> | ||
| <div id={ descriptionId } style={ { display: 'none' } }> |
There was a problem hiding this comment.
Hiding descriptions makes them inaccessible.
Suggested change
| <div id={ descriptionId } style={ { display: 'none' } }> | |
| <VisuallyHidden id={ descriptionId }> |
There was a problem hiding this comment.
In this case, since descriptions are attached to the button via aria-describedBy, they get announced when focus is on the related button. Using <VisuallyHidden/> it was announced twice: once before the button, and once while focus was on the button.
I was going off of @afercia's comment:
Add an aria-describedby attribute that points to an element containing the additional information. This element can also be hidden with display: none, screen readers will announce the description anyways.
I feel like one announcement semantically linked to the button is better than it announced before the button and again with the button, but I'm also aware I'm not a pro screen reader user so I'll defer to your judgement :)
There was a problem hiding this comment.
@jeryj Yeah, I tested it and display: none; is not allowing the updated description to be read. It just has the default not selected message even after selecting a color.
Should probably place the description outside of the button though.
There was a problem hiding this comment.
I'll go ahead and change it to VisuallyHidden. Which screen reader + browser are you testing on? I'm curious to see if I can use display: none; and still get it to update the message after selection so it's not as verbose.
There was a problem hiding this comment.
@jeryj NVDA with Firefox.
There was a problem hiding this comment.
To be clear, did you test with the current version of this PR that is using <VisuallyHidden/>? Or did you edit it to go back to the display: none method? I'll update this PR to the display: none; method.
There was a problem hiding this comment.
@jeryj I just tested the current version of the PR but I created a similar situation in a local testing file. If you revert back, will be interested to see if it works.
There was a problem hiding this comment.
Updated the PR to use display: none;. If it doesn't work, then one possibility is that it doesn't work reliably after a selection due to the description div being removed and remounting when the component updates?
There was a problem hiding this comment.
@jeryj I think it might be the re-mounting issue. It no longer works.
There was a problem hiding this comment.
I opened an issue to discuss and work on this: #55362
| <> | ||
| <div id={ descriptionId } style={ { display: 'none' } }> | ||
| { toggleAriaDescription } | ||
| </div> |
There was a problem hiding this comment.
Suggested change
| </div> | |
| </VisuallyHidden> |
|
Flaky tests detected in 87a07f2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6590188245
|
getdave
reviewed
Sep 25, 2023
| import type { ColorObject } from '../../color-palette/types'; | ||
| import { isMultiplePaletteArray } from '../../color-palette/utils'; | ||
| import type { DropdownProps as DropdownComponentProps } from '../../dropdown/types'; | ||
| import type { ColorProps, DropdownProps } from '../types'; | ||
|
|
||
| const getAriaLabelColorValue = ( colorValue: string ) => { | ||
| const getAriaDescriptionColorValue = ( colorValue: string ) => { |
There was a problem hiding this comment.
I think this function should be renamed to remove anything to do with aria. Small point but this seems to just be stripped var() from any CSS vars am I right? In which case let's
- Rename to better advertise it's purpose
- Consider extracting to a file and adding some quick unit tests.
There was a problem hiding this comment.
I agree this could be made into a more generic utility function. Looks like it would be useful elsewhere too. Would you be OK with this also being a follow-up? It's starting to expand the purpose of this PR a bit.
There was a problem hiding this comment.
I think we have a number of tasks that should happen as follow ups. Whilst I'm fine with them being follow ups I really think that whilst we're in the guts of this it would be good to leave the code in a better place than we found it if possible 🙏
getdave
reviewed
Sep 25, 2023
| colorValue: CSSProperties[ 'borderColor' ], | ||
| colorObject: ColorObject | undefined, |
There was a problem hiding this comment.
Does this function need to know so much? Can it not simply accept a colorValue and the consumer can pass the appropriate value. This could reduce the number of confusing conditionals.
There was a problem hiding this comment.
Oh I see it needs to get the name from the object....
There was a problem hiding this comment.
In which case it could accept
colorValuecolorName
That way it would not need to know about the structure of a colorObject. It can then simply compute appropriate strings based on what you provide it.
Again let's not do this refactoring without tests 🙏
getdave
reviewed
Sep 25, 2023
| @@ -65,68 +66,77 @@ const getColorObject = ( | |||
| return colors.find( ( color ) => color.color === colorValue ); | |||
| }; | |||
|
|
|||
| const getToggleAriaLabel = ( | |||
| const getToggleDescription = ( | |||
| colorValue: CSSProperties[ 'borderColor' ], | |||
| colorObject: ColorObject | undefined, | |||
| style: CSSProperties[ 'borderStyle' ], | |||
| isStyleEnabled: boolean | |||
There was a problem hiding this comment.
What does this boolean signify? What is "style enabled"?
There was a problem hiding this comment.
I'm honestly not sure -- I only renamed it to feel more accurately named and used what I needed from it. I haven't looked into it further. I do agree a refactor would be nice, but I'd prefer it as a follow-up.
getdave
reviewed
Sep 25, 2023
| @@ -65,68 +66,77 @@ const getColorObject = ( | |||
| return colors.find( ( color ) => color.color === colorValue ); | |||
| }; | |||
|
|
|||
| const getToggleAriaLabel = ( | |||
| const getToggleDescription = ( | |||
There was a problem hiding this comment.
I think it would be good to extract this function and write some simple tests for the various conditionals. These are outlined in your test instructinos for the PR and it would provide a lot of confidence if they were encoded into tests:
- No style, no color
- Style, no color
- Named color, no style
- Named color, with style
- Hex/rgba color, no style
- Hex/rgba color, with style
I would also say it's worth refactoring it with the aim of removing the branches and conditionals but that could be done in a follow up and should not even be attempted unless we have tests in place 🙏
There was a problem hiding this comment.
I agree that this should all get refactored. As I was working within the context of what was here, I didn't want to change up things too much since I only wanted to provide a better description.
There was a problem hiding this comment.
It needs tidying up. I appreciate that's a big ask for this PR though. Feel free to add as a follow up task if you like.
I would help but I don't have bandwidth at this time 🙇
jeryj
force-pushed
the
fix/border-color-state-description
branch
from
70dd01f to
634864f
Compare
draganescu
reviewed
Oct 17, 2023
There was a problem hiding this comment.
I think this is ready to go code wise - what is holding it back? I do agree with @getdave 's follow up points but the PR itself achieves its goals.
An approval :) |
|
Size Change: +6.93 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
The previous approach appended a long message to the aria-label to communicate the state of the color selection. This is not desireable, as it changes the accessible name of the button. The accessible name of the button should be stable and brief, and not be used to communicate state. By using aria-describedBy, we can communicate the state in a way that is similarly announced but keeps button name and state separate.
draganescu
force-pushed
the
fix/border-color-state-description
branch
from
87a07f2 to
9315c6a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Communicate border color and style via
aria-describedByusing an approach explained by @afercia in #54439 (comment)Why?
The previous approach appended a long message to the aria-label to communicate the state of the color selection. This is not desireable, as it changes the accessible name of the button. The accessible name of the button should be stable and brief, and not be used to communicate state. By using aria-describedBy, we can communicate the state in a way that is similarly announced but keeps button name and state separate.
How?
Switches from appending state to the aria-label and adds an aria-describedBy attribute with hidden text (via
display: none;, as the text will be read via focus on the button and doesn't need to announced otherwise.Testing Instructions
Screenshots or screencast