Skip to content
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

Edit field in quick picker is inaccessible to screen readers #101183

Closed
Neurrone opened this issue Jun 27, 2020 · 16 comments · Fixed by #179221
Closed

Edit field in quick picker is inaccessible to screen readers #101183

Neurrone opened this issue Jun 27, 2020 · 16 comments · Fixed by #179221
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders quick-pick Quick-pick widget issues verified Verification succeeded
Milestone

Comments

@Neurrone
Copy link

CC: @isidorn

Issue Type: Bug

  1. With NVDA running, press ctrl+p or ctrl+shift+p. Then type some text.

Expected: pressing backspace or using the arrow keys reads the relevant characters as with usual edit fields when focus is on the edit field which controls the options that appear.
Actual: nothing is heard

VS Code version: Code - Insiders 1.47.0-insider (0913b1a, 2020-06-23T09:03:35.861Z)
OS version: Windows_NT x64 10.0.19041

System Info
Item Value
CPUs Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz (8 x 1992)
GPU Status 2d_canvas: enabled
flash_3d: enabled
flash_stage3d: enabled
flash_stage3d_baseline: enabled
gpu_compositing: enabled
multiple_raster_threads: enabled_on
oop_rasterization: disabled_off
opengl: enabled_on
protected_video_decode: enabled
rasterization: enabled
skia_renderer: disabled_off_ok
video_decode: enabled
viz_display_compositor: enabled_on
webgl: enabled
webgl2: enabled
Load (avg) undefined
Memory (System) 15.86GB (4.43GB free)
Process Argv
Screen Reader yes
VM 17%
Extensions (14)
Extension Author (truncated) Version
notes can 1.0.1
vscode-markdownlint Dav 0.36.1
vscode-eslint dba 2.1.5
gitlens eam 10.2.2
prettier-vscode esb 5.1.0
Go gol 0.14.4
rest-client hum 0.24.1
python ms- 2020.6.90262
remote-wsl ms- 0.44.4
cpptools ms- 0.28.3
vsnotes pat 0.7.1
rust rus 0.7.8
markdown-preview-enhanced shd 0.5.12
vscode-webhint web 1.5.6
@chrmarti chrmarti added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues quick-pick Quick-pick widget issues labels Jun 27, 2020
@isidorn
Copy link
Contributor

isidorn commented Jun 29, 2020

I tried this out and can reproduce with NVDA.
However it works just fine with Orca and VoiceOver. So I think there seems to be an issue on the NVDA side here.
@LeonarddeR @feerrenrut Is it possible to look into this from the NVDA side. Repro should be quite easy with the comments from above.
We are setting activeDescendent to set focus on the child element so that might be confusing NVDA?

@isidorn isidorn added the under-discussion Issue is under discussion for relevance, priority, approach label Jun 29, 2020
@TylerLeonhardt
Copy link
Member

Closing this since there hasn't been any activity and @isidorn says it's likely on the NVDA side.

@TylerLeonhardt TylerLeonhardt added the info-needed Issue requires more information from poster label Oct 21, 2021
@feerrenrut
Copy link

Is there a related NVDA issue? I couldn't find one. It would be really helpful to have a test sample to ensure we look at the right thing.

@isidorn
Copy link
Contributor

isidorn commented Nov 5, 2021

@feerrenrut Sorry for the slow response, I was on vacation last week. I have created this issue nvaccess/nvda#13037 let's continue the discussion there. Thanks!

@LeonarddeR
Copy link

I can reproduce this with Narrator as well. I'd be curious about JAWS. As per nvaccess/nvda#13037 (comment) I'm not sure whether saying this is NVDA specific is a valid conclusion, may be it is Windows specific though.

@isidorn
Copy link
Contributor

isidorn commented Nov 8, 2021

@LeonarddeR I just tried JAWS and I can also reproduce there.
So I do agree it seems like the issue is not on the NVDA side, thus let's reopen this one.

What actually happens is that we use aria-activedescendant here. So the inputbox has dom focus but the screen reader because of the aria-activedescendant attribute is looking at one of the items in the list. I am not sure how this works on Orca or on VoiceOver. Since in practice left / right navigation leaves focus in the inputbox which has dom focus and then the characters should be read. up/down changes the activedescendant and then the item in the list should be read.

@joanmarie might you know how does Orca handle these "two focused items" problem?

Repro: open vscode, press ctrl+p. Type some text. Use left/right arrows.

@isidorn isidorn reopened this Nov 8, 2021
@LeonarddeR
Copy link

It might be good too look ath the Aria best practice for development guidelines. Those seem to behave correctly on Windows.

@TylerLeonhardt TylerLeonhardt added this to the Backlog milestone Nov 12, 2021
@Neurrone
Copy link
Author

Neurrone commented Feb 1, 2022

I can confirm this problem still exists.

@isidorn isidorn removed the info-needed Issue requires more information from poster label Feb 1, 2022
@isidorn
Copy link
Contributor

isidorn commented Feb 1, 2022

I can also reproduce. I also tried with the Exploration build that uses the latest Chrome and I can still reproduce.
@TylerLeonhardt can you check if we for example do not set the active-descendent does this no longer repro? Just so we make sure that is the thing that confuses screen readers on Windows. Maybe we need to introduce a workaround: when the user navigates left / right in the quick pick do not set the active descendant.

@LeonarddeR I still believe that the issue is on the Windows / Chrome / Screen reader side. And that NVDA should get events from Chrome if there is a cursor change on a parent of the active descendent.

@TylerLeonhardt
Copy link
Member

This should be working now since for accessibility mode we do not set aria-activedescendent when the input box changes. #176080

@TylerLeonhardt TylerLeonhardt modified the milestones: Backlog, April 2023 Apr 3, 2023
@TylerLeonhardt TylerLeonhardt added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Apr 3, 2023
@TylerLeonhardt TylerLeonhardt reopened this Apr 5, 2023
@TylerLeonhardt TylerLeonhardt modified the milestones: April 2023, Backlog Apr 5, 2023
@TylerLeonhardt
Copy link
Member

reopening as I am depending on aria-activedescendant again in #179217

@TylerLeonhardt
Copy link
Member

@rperez030 mentioned we could hook up Shift + Tab to remove the aria-activedescendant by maybe focusing none.

TylerLeonhardt added a commit that referenced this issue Apr 5, 2023
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Apr 5, 2023
@rperez030
Copy link
Contributor

rperez030 commented Apr 5, 2023

@TylerLeonhardt this is looking great to me. Some ideas for possible refinements.

focus management.

If I set focus to an element in the list, press shift +Tab and tab back to the list again, focus gets reset. In fact, it seems like the parent list receives focus at this point. There are also instances in which I need to Shift Tab twice to move focus from the list to the combobox, particularly if it is the second or third time I do it. Ideally focus would return to the item that was selected before pressing Shift +Tab. If this is not possible, I would then suggest that Tabs behaves like Down arrow key which focuses on the first element in the list.

controls accessible names and descriptions

Right now, the combobox has the generic name of 'input'. NVDA calculates the accessible description based on the placeholder text but JAWS doesn't. My suggestion: We could use a more descriptive name for the combobox, maybe "filter", and use aria-description for the placeholder text.
The list currently receives the accessible name from the placeholder text as we discussed, but that feels more like a description to me. suggestion: We could give the list a generic name (E.g. 'actions'), and use aria-description to communicate the placeholder text. Ideally that list will be named based on context depending on how the widget is used, but that may complicate things and open a door for future issues if it is not defined correctly.

I think it would help if we could also suppress the unnecessary announcement of search results (#175559).

Like I said, even without refinements, in my opinion, this feels even better than the initial implementation. thank you very much to everyone involved @isidorn, @meganrogge.

@TylerLeonhardt
Copy link
Member

TylerLeonhardt commented Apr 5, 2023

@rperez030 thanks for your feedback! Can you open new issues for each of these improvements so that I can tackle them independently?

I'm glad it's better for you. I may have to focus on other things I own at the moment, but I want to make sure these additional improvements are captured.

@isidorn isidorn added the verified Verification succeeded label Apr 5, 2023
@rperez030
Copy link
Contributor

Definitely. I will open separate issues. Maybe I can even tackle some of this myself with a bit of mentorship.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders quick-pick Quick-pick widget issues verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants