-
Notifications
You must be signed in to change notification settings - Fork 8.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
[a11y] Make CommandPalette announce selected item #13519
Conversation
d5d1e88
to
3a5ffdf
Compare
I'm a little concerned. How did we regress this? It was working, and I thought that a list view was supposed to automatically handle accessibility notifications when its selected item changed. Is that not true? Has this never worked? Mini-RCA? |
Nah, this isn't a regression. It's just slightly different. The implementation we had was that we added The difference here is when that information gets extracted. The
But the command palette has an additional way to change the selection without moving the focus:
This PR enables support for that second scenario. NOTE: since the experience is still accessible, the bug is marked as a Sev3 as opposed to a higher one. |
I don't know how I feel about this. Are we certain that the content that Narrator reads is identical to the one when you select a list item? What about "drilling in" for details -- can you get Narrator to read the Name and the Key Binding separately? What happens today in keyboard focus if you do that? What about this new thing? |
Is there a better way to do this than say "narrator speak these words?" Is it possible to say, "narrator, speak this control"? |
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.
Red for questions
Tested with Narrator/NVDA/JAWS: identical behavior to when you move focus onto a list item
After a bit of digging, I can't figure out a way to do this with Narrator or NVDA. I reached out to Bill and he said that NVDA just reads accelerator keys automatically; there's no way to do it on request, but he could write up a plug-in for it. It seems to be a common standard to just read the accelerator keys.
Hmm... I don't think there's a way to just hand over the control to Narrator. I don't think that's necessarily desired either though? The user has the ability to tab to the selected item then "drill in" if so desired. This notification is more intended to let the user know what the "enter" button will do if it's invoked. |
There is now, see PR nvaccess/nvda#13960 (available from 2022.4). |
{ | ||
if (const auto paletteItem = filteredCmd.Item().try_as<TerminalApp::PaletteItem>()) | ||
{ | ||
automationPeer.RaiseNotificationEvent( |
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.
okay, here's another annoying question!
How do we tell the user that the command they selected has nested commands? What is the audible version of the >
symbol?
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.
Ah, you've stumbled upon #12041 haha.
The short version is that we have help text saying "More options", but depending on the screen reader, that may or may not be read automatically. Rahul seems to have commented on the thread with a way to fix the issue, so that's nice.
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.
This is an improvement for now!
@msftbot merge this in 10 minutes |
Hello @carlos-zamora! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
## Summary of the Pull Request The command palette (and tab search by extension) doesn't ever tell screen readers what is selected. Here, we simply hook up the selection changed event to a function that tells the screen reader what is selected. With this, the user no longer has to tab into the list view to know what is selected! Will resolve the following bug upon validation from a11y team: #12065 ## Validation Steps Performed Performed repro steps from #12065. NOTE: we do NOT read the selected item when the command palette is first opened. I think that's ok. (cherry picked from commit deb5e7c) Service-Card-Id: 85254984 Service-Version: 1.15
🎉 Handy links: |
🎉 Handy links: |
Summary of the Pull Request
The command palette (and tab search by extension) doesn't ever tell screen readers what is selected. Here, we simply hook up the selection changed event to a function that tells the screen reader what is selected. With this, the user no longer has to tab into the list view to know what is selected!
Will resolve the following bug upon validation from a11y team: #12065
Validation Steps Performed
Performed repro steps from #12065.
NOTE: we do NOT read the selected item when the command palette is first opened. I think that's ok.