Global Styles: add accessible label to Back button #35325
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
…ng to previous screen
ciampo
added
[a11y] Labelling
[Type] Bug
|
Size Change: +48 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
mtias
reviewed
Oct 4, 2021
packages/edit-site/src/components/global-styles/navigation-button.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Oct 4, 2021
| <Item onClick={ () => navigator.push( path, { isBack } ) } { ...props }> | ||
| <Item | ||
| onClick={ () => navigator.push( path, { isBack } ) } | ||
| aria-label={ labelProp ?? defaultLabel } |
There was a problem hiding this comment.
Should this be label prop like the Button component. I also wonder why it's not taking the "content" as a label like it's supposed to do by default for buttons.
There was a problem hiding this comment.
Should this be label prop like the Button component.
The Item component renders a vanilla HTML button (and not a WordPress Button — sorry if I caused any confusion in a previous comment where I stated the opposite).
I also wonder why it's not taking the "content" as a label like it's supposed to do by default for buttons.
I believe it is, but in the case of the back buttons in the Global Styles sidebar, these buttons only have an icon as their content (no human-readable text)
There was a problem hiding this comment.
Got it Thanks, should we use a label prop to align with Button or keep the raw aria-label? I think it's a bit mixed right now in the different components we have and not sure what's the best path forward (maybe a small preference for label in components where it's a generic use-case for compatibility with mobile).
There was a problem hiding this comment.
I agree that it would be nice to have a less mixed approach to how we pass and name props related to labels.
Given the fact that Item renders a HTML button, at the moment we have to pass the aria-label prop to Item (since label is not a standard HTML attribute).
Maybe a way to improve the situation would be to rename the label prop on the NavigationButton component to aria-label? I could also remove the changes from the NavigationButton and pass the aria-label attribute directly from the ScreenHeader component:
diff --git a/packages/edit-site/src/components/global-styles/header.js b/packages/edit-site/src/components/global-styles/header.js
index 1ddd562aa2..1496bb440c 100644
--- a/packages/edit-site/src/components/global-styles/header.js
+++ b/packages/edit-site/src/components/global-styles/header.js
@@ -31,6 +31,7 @@ function ScreenHeader( { back, title, description } ) {
}
size="small"
isBack
+ aria-label={ __( 'Navigate to the previous screen' ) }
/>
</View>
<Spacer>Separately from this PR, we could consider rendering a WordPress Button instead of a HTML button inside Item, which could help to make our APIs a bit more coherent
There was a problem hiding this comment.
I could also remove the changes from the NavigationButton and pass the aria-label attribute directly from the ScreenHeader component
I went ahead and applied that suggested change in a528e22 (#35325) — which greatly simplifies the code changes in this PR
youknowriad
approved these changes
Oct 4, 2021
priethor
added
[Focus] Accessibility (a11y)
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.
Description
Fixes #35291
Pass an
aria-labelprop to the back button inScreenHeadersaying'Navigate to the previous view'How has this been tested?
npm run distclean && npm ci && npm run wp-env start && npm run devlocalhost:8888/wp-admin, active theTT1-blockstheme and open the Site EditorScreenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).