PaletteEdit: Fix palette item accessibility #58214
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
For better visual testing of PaletteItem borders
mirka
added
[Type] Bug
|
Size Change: +2.68 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
mirka
commented
Jan 24, 2024
| @@ -56,6 +56,7 @@ Default.args = { | |||
| colors: [ | |||
| { color: '#1a4548', name: 'Primary', slug: 'primary' }, | |||
| { color: '#0000ff', name: 'Secondary', slug: 'secondary' }, | |||
| { color: '#fb326b', name: 'Tertiary', slug: 'tertiary' }, | |||
There was a problem hiding this comment.
Increased this to three colors so we can better check the border radiuses in details view.
| width: ${ space( 6 ) }; | ||
| height: ${ space( 6 ) }; | ||
| pointer-events: none; | ||
| export const IndicatorStyled = styled( ColorIndicator )` |
There was a problem hiding this comment.
Changed this from CircularOptionPicker.Option (which is a button) to a simple ColorIndicator. This isn't supposed to be an independent button — the entire item row is a button with a single onClick handler.
| pointer-events: none; | ||
| export const IndicatorStyled = styled( ColorIndicator )` | ||
| && { | ||
| flex-shrink: 0; |
There was a problem hiding this comment.
Prevents the indicator from shrinking when the color name is too long.
| height: ${ space( 6 ) }; | ||
| pointer-events: none; | ||
| export const IndicatorStyled = styled( ColorIndicator )` | ||
| && { |
There was a problem hiding this comment.
Without this added specificity, the intended sizing (24px) was sometimes not being applied due to load order. This was one of the things causing style inconsistencies (#49041).
| &:first-of-type { | ||
| border-top-left-radius: ${ CONFIG.controlBorderRadius }; | ||
| border-top-right-radius: ${ CONFIG.controlBorderRadius }; | ||
| font-size: ${ font( 'default.fontSize' ) }; |
| padding-block: 3px; | ||
| padding-inline-start: ${ space( 3 ) }; | ||
| border: 1px solid ${ CONFIG.surfaceBorderColor }; | ||
| border-bottom-color: transparent; | ||
| &:first-of-type { |
There was a problem hiding this comment.
Reworked this part because :first-of-type will not work anymore with the button/div polymorphism, and Emotion doesn't want us to use :first-child for SSR safety. (Apparently :last-child is ok)
| @@ -295,7 +295,7 @@ describe( 'PaletteEdit', () => { | |||
| name: 'Show details', | |||
| } ) | |||
| ); | |||
| await click( screen.getByText( 'Primary' ) ); | |||
| await click( screen.getByRole( 'button', { name: 'Edit: Primary' } ) ); | |||
There was a problem hiding this comment.
The faux button is now properly accessible 🎉
|
Flaky tests detected in b3c0d36. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7643570992
|
t-hamano
reviewed
Jan 25, 2024
There was a problem hiding this comment.
Thank you for the PR! From what I've tested, I think it works as expected for both Global Styles and Storybook.
One thing I was concerned about is what to do with aria-label when the color name is empty. Probably we shouldn't allow empty color names to be defined, but for example, would it make sense to read out the color code instead?
6817f2e721c9c7a2b8939a73c14099f1.mp4
Also, because the color indicator is no longer a button element, the round border no longer appears in Windows high contrast mode. I think we just need to apply border 1px solid transparent to the color indicator.
Update: I've noticed that this indicator doesn't appear on other color panels either, so this may be outside the scope of this PR.
|
Thanks for the review @t-hamano!
Good call. I added some fallbacks in 3e560ed, which should be good to have in place regardless of how we enforce/validate the color naming in the future.
You're probably right, this needs to be addressed in the underlying component. Although, I'm actually not sure if a non-interactive color swatch needs a border in high contrast mode. |
t-hamano
approved these changes
Jan 25, 2024
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.
Follow-up to #57645 (comment)
Fixes #49041
What?
Fixes palette item accessibility in the details view of the PaletteEdit component.
Note
This doesn't fix the issue where focus gets stuck in the ColorPicker popover (#58188).
Why?
Only the swatch was a focusable button, and it was unlabeled.
Testing Instructions
There should be no changes in behavior when navigating by pointer.
Testing Instructions for Keyboard
Either in Storybook or the Editor (Global Styles ▸ Colors ▸ Palette):
Note
There is currently a style leak in the Editor that causes the palette item to have an unintentional background color when in editing mode.
Screenshots or screencast
Before
CleanShot.2024-01-25.at.02.00.05.mp4
After
CleanShot.2024-01-25.at.02.01.40.mp4