Shadows: Improve accessibility of shadows dropdown #58828
Merged
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
madhusudhand
added
[Type] Enhancement
|
Size Change: +161 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
madhusudhand
changed the title
|
Where do I find the shaddows dropdown? |
@alexstine Sorry, testing instructions were not clear. Updated the PR description. Tagging @richtabor and @jasmussen for design feedback. |
|
@alexstine could you provide feedback on this possibly before WP 6.5 Beta 3 |
alexstine
approved these changes
Feb 19, 2024
There was a problem hiding this comment.
@madhusudhand Thanks for the PR. I just tested this and can certainly notice the improvements.
The one thing that isn't great about dropdowns such as this one is there is no way to tell which option is selected without interacting with the dropdown. This is where I really wish Gutenberg would just use a native select as it was made for this reason.
For example, if I come to an aria-label of Drop shadow, that doesn't tell me which option is selected but due to the rules of changing states, it would not be viable to dynamically update that aria-label. Form controls should never convey states.
@jeryj I feel like you worked on a PR for a similar dropdown before, any thoughts?
Otherwise, approving this as it is an improvement nonetheless.
madhusudhand
force-pushed
the
shadow-accebility
branch
from
e5f80ae
to
932e4f1
Compare
madhusudhand
force-pushed
the
shadow-accebility
branch
from
932e4f1
to
5f35fea
Compare
|
Flaky tests detected in 585e821. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8005449614
|
madhusudhand
added
the
Backport to WP 6.7 Beta/RC
jeryj
reviewed
Feb 23, 2024
There was a problem hiding this comment.
Thanks for your work on this! I left a few comments. @alexstine the work I did for some of these kinds of elements was to show the selected value on the button that opens the popover. That could be a nice follow-up to this: Add the selected shadow value as an aria-label and visual icon on the button that opens the shadow options.
| } | ||
| ) } | ||
| render={ | ||
| <Button |
There was a problem hiding this comment.
Do these still need to be buttons? Having a button within an Option seems odd. Can we remove the button and have the shadow icon be the direct content of the <option> instead?
Comment on lines
+52
to
+58
| const { CompositeV2: Composite, useCompositeStoreV2: useCompositeStore } = | ||
| unlock( componentsPrivateApis ); | ||
| const compositeStore = useCompositeStore(); | ||
| return ! presets ? null : ( | ||
| <Grid columns={ 6 } gap={ 0 } align="center" justify="center"> | ||
| <Composite | ||
| store={ compositeStore } | ||
| role="listbox" |
There was a problem hiding this comment.
I'm surprised we need to unlock a component for this. Is there another way to build this custom listbox without needing to unlock the componentsPrivateApis for it?
There was a problem hiding this comment.
Thanks @jeryj. I followed the implementation of duotone as they both have similar UI. I will create a followup issue and address it as enhancement.
madhusudhand
added
[Type] Bug
|
Changing the label from enhancement to bug as the acceptability of the shadows dropdown wasn't correct, and this fixes it. |
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Improve accebility of shadows dropdown * fix mobile unit tests by moving privateApis unlock inside components
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: 52c4570 |
getdave
added
Backported to WP Core
9 tasks
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
* Improve accebility of shadows dropdown * fix mobile unit tests by moving privateApis unlock inside components
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?
This improves the keyboard accessibility of the shadows dropdown.
Before:
Each shadow item was a button before this change and a
Tabnavigation is required to switch shadows with keyboard.After:
Shadows are now act a listbox and allows to change between the shadow items using keyboard
left <-andright ->arrows.Also, introduced a
transform: 1.2on hover for better mouse accessibility (similar to duotone)Testing Instructions
Testing Instructions for Keyboard
Try invoking and applying shadows using keyboard navigation
Tab:to switch between menu itemsSpace/Return:to invoke or apply a shadowArrow keys:to switch between shadows.