-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Consistent, descriptive accessible name for command palette button #54323
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +458 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeryj Thanks for the PR. I think it is working really well. Is it too much trouble to add these attributes while we're here? For the aria-expanded
, you need to pass in the state. If you don't have access to it easily in the store, we can revisit this later.
packages/edit-post/src/components/header/document-actions/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/header-edit-mode/document-actions/index.js
Outdated
Show resolved
Hide resolved
Also tagging @joedolson to make sure all issue requirements were met. |
Bug, yes. True functionality, yes. This long time issue comes from wordpress/interface and no one yet has been brave enough to refactor it. |
This change leaves the visible label of the button with the same value it had before, which creates a violation of WCAG 2.5.3: Label in Name, because the control's name ("Command Palette") is not present in the visible label. I think we need to have a separate button that triggers the command palette and is not also conveying the name of the current page.
|
@joedolson Ah, so do we need a different design here? Update: I should have fully read through the WCAG 2.5.3: Label in Name Success Criteria. The visible label mismatching the accessible name will fail, as it will be very difficult for people using speech-activated AT to identify what to say to trigger the button. They would reasonably expect to say, "Home Template" to activate it, and not "Command Palette." We need the design to be such that they easily know to say, "Command Palette" to open it. Could we do a split button design like this example? I know it's less than ideal to have multiple buttons with no visual separation. I'm trying to get more accessible through for now and then addressing a further redesign. But if it's better to push for a more full reconsideration of this design then we can do that instead. Update: Reconsidering the split-button design, as the Template Title would then look like a button but do... nothing? Previously, I believe there was a dropdown arrow and template switcher? Maybe that functionality could return and there'd be one ` that's for switching templates and one button for opening the command palette (command + K) with a "Command Palette" Tooltip? Tagging @richtabor for potential design solutions. Maybe there's a quick win here I'm missing. |
Hi, @jeryj! Sorry about the very late response. Yes, a split button would be totally fine from an accessibility perspective. Having two buttons in that space is not a problem, as long as they both state what they do in a meaningful way. Would love to see this move forward for 6.6. |
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. |
That must be a very recent change; can you point me to the PR that added it? It may be that it's doing the same thing here, but we just need to get the related PRs and issue closed out. |
I'm not entirely sure, none of the PRs stick out to me: https://github.com/WordPress/gutenberg/pulls?q=is%3Apr+is%3Aclosed+label%3A%22%5BPackage%5D+Commands%22 |
No, nothing there is apparent to me, either. I can see that #57427 highlights this in screenshots, so it must have already existed in December; but if so, I can't see when it shows up - I'm not finding a path that gives me that view. |
Can you tell me the path to get there? Not really clear what focus view is. |
Sure thing.
|
This PR is months old, could we please revive it? This UI label is still a total disaster. Now that the command palette has a more prominent place in the post editor, this issue keeps ballooning with no solution.
Seriously, if you were a user without vision, would you have any idea what that means? BTW, we've already got one heading 1 on the page. The hidden "Edit Post" for screen readers. That command palette button needs to just be labeled Command palate (Ctrl+K) and the title can be moved elsewhere. We continue to release new versions of WordPress with a command palette that has been inaccessible from day one. Maybe that'll change one of these days... This would be a good start. Thanks. |
Fixes #51394
What?
The command palette activation button had an H1 for the page title inside of it, meaning the command palette action button was not consistently named. It would be the equivalent of having a Save button be called "Template: Blog Home ⌘S" instead of "Save."
This PR brings consistent names to the command palette activation.
How?
aria-label
ofCommand Palette
to the button<h1>
to be the first item in the header, and visually hide it.This keeps the H1 of the page consistent, all visual changes remain the same, and the command palette button is always consistently named.
Testing Instructions
There are three places I could find the title being used.
Site Editor
Command Palette
Style Guide H1
Template Editor within the Post Editor
I didn't know this existed. It seems easy to overlook.
Command Palette
Testing Instructions for Keyboard
Screenshots or screencast