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

List should announce how many items are selected #91061

Closed
isidorn opened this issue Feb 20, 2020 · 57 comments
Closed

List should announce how many items are selected #91061

isidorn opened this issue Feb 20, 2020 · 57 comments
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug list-widget List widget issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Feb 20, 2020

  1. Turn on Screen Reader
  2. Add an element to the List selection such that you have two items selected - Screen reader does not announce that an item is added to the selection

I have checked what native Windows Explorer does with NVDA - nothing.
And what Finder does with VoiceOver on mac - and they announce when the element after the first is added to the selection.
It announces NAME added to selection 2 items selected
I think we should strive for this experience if possible, so nothing announced if only one item is selected but announce when other items enter the selection

Edit:
We have done changes on the VS Code side however there is still a Chrome issue.
Upstream issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1058961.

@isidorn isidorn added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues list-widget List widget issues labels Feb 20, 2020
@isidorn
Copy link
Contributor Author

isidorn commented Feb 20, 2020

The issues seems to be that the aria-selected is only set on one item in the multie selection.
aria-selected should be set on each item in the selection. Once we have that hopefully the screen reader will be read the correct content.

@pawelurbanski
Copy link
Contributor

Please, cause announcement only in teh followin example use cases:

  • You scroll through some list - list of commits for example,
  • You select only those you are interested in,
  • Perform some action such as squashing them.

Never announce selection or item position in places where the list pops up automatically such as intelisense suggestion list.
If you did you would get the following output:

  • document 5 of 45
  • abort... 25 of 64,
  • addEventListener 4 of x,

And now imagine, you have to hear X of Y all day long when you are coding and intelisense widgget popsup.

A possible alternative, which would also trigger checkmarks in braille would be using aria-checked with similar announcements.

Will be very happy to test it - I just want to make VS Code the perfect IDE.

@isidorn
Copy link
Contributor Author

isidorn commented Feb 21, 2020

@pawelurbanski thanks a lot for the feedback and for the help.
However my suggestion is to behave like the Mac Finder and that is only announce selection if multiple items are selected. And this will never happen in intelisense (since it does not support multi selection).

If there is unnecesery output in inteli-sense we can fix it on the inteli-sense level (it can override how the tree sets roles).
As for aria-checked we only use it for items with actual checkboxes (like for example breakpoints and rename elements in the rename preview view)

@joaomoreno joaomoreno added this to the Backlog milestone Feb 26, 2020
@joaomoreno joaomoreno added the bug Issue identified by VS Code Team member as probable bug label Feb 26, 2020
@joaomoreno
Copy link
Member

@isidorn Wanna look into this?

@isidorn
Copy link
Contributor Author

isidorn commented Feb 27, 2020

@joaomoreno sure, assignign to next milestone. Since you will be out I might just merge if the fix is simple.

@isidorn isidorn modified the milestones: Backlog, March 2020 Feb 27, 2020
@pawelurbanski
Copy link
Contributor

pawelurbanski commented Feb 27, 2020 via email

@isidorn
Copy link
Contributor Author

isidorn commented Mar 4, 2020

The VS Code list has both selection and focus, and an item can be focused and not selected.
Currently we set aria-selected="true" for focused items which is a bit incorrect.

I have looked into this and did the change that aria-selected to be set on actual selected items and we add the aria-multiselectable to the list container when multi selection is supported. Unfortunetly NVDA, Orca and VoiceOver do not read anything regarding selection.

Actually the expeirence gets worse on NVDA, since with these changes when focus moves NVDA would announce that each item is not selected.

@LeonarddeR on potential ideas on how to set aria-selected that NVDA reads everything properly
@joanmarie on ideas for Orca

PR work in progress #92019
Docs for reference https://www.w3.org/WAI/PF/aria/states_and_properties#aria-multiselectable

@isidorn isidorn modified the milestones: March 2020, On Deck Mar 4, 2020
@pawelurbanski
Copy link
Contributor

pawelurbanski commented Mar 4, 2020 via email

@LeonarddeR
Copy link

There is this best practise, example 2. This one also doesn't select on focus and you can select the entries with space.

Where in VS Code can I find a multi select list, so I can investigate the behavior?

I think there are two types of multi select lists:

  1. The ones such as in Windows Explorer. Focus changes the selected item to the focused item, unless ctrl is being held. In this case, it makes sense to apply aria-selected to the focused item, but of course aria-selected has to convey the actual selected state.
  2. The one such as in the ARIA example. Items are not selected unless you explicitly do so with space. Here, focus doesn't have any relation with selection.

@LeonarddeR
Copy link

cc @zersiax

@isidorn
Copy link
Contributor Author

isidorn commented Mar 5, 2020

@LeonarddeR a multi select list is the Explorer. Just hold down shift and you can select mulitile items.
All our lists are of the 2. type so focus is not connected to selection in any way.

I will check out the best practise example 2.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 5, 2020

With my PR we actually follow all the best practises of example 2. Except that we use the role="tree" and not listbox since it is a tree. And NVDA does not announce the state of multiple items selected. So we:

  • use activedescendent
  • set multiselectable on the parent tree element
  • set aria-selected on each selected item

@joanmarie
Copy link

@isidorn: I was going to suggest using ARIA's activedescendant attribute which should cause ATs to get focus events each time the referenced element changes. But it seems you've already worked that out.

Does your PR cause Orca to present things as you'd expect? Or do you still need my help?

@LeonarddeR
Copy link

With my PR we actually follow all the best practises of example 2. Except that we use the role="tree"

This confuses me. Are we working with a list or a tree? Are we working with listitems or treeitems?
Children of a tree should be treeitem, children of a list should be listitem. I don't think ('aria-multiselectable will work on a tree. See: https://www.w3.org/TR/2017/NOTE-wai-aria-practices-1.1-20171214/examples/treeview/treeview-2/treeview-2a.html

What this example doesn't implement is selection, but a TreeItem is a child of a ListItem, so that's supported just fine by the spec.

@LeonarddeR
Copy link

I'm wrong, aria-multiselectable is allowed on tree indeed.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 5, 2020

@joanmarie @LeonarddeR Orca and NVDA do not announce selection properly IMHO. Open the native explorer, select mutliple items and note how Orca on Linux does not announce that mulitple items are part of the selection. NVDA has the same flaw with Windows Explorer. The screen readers do not announce state of selection. They only announced the focused item.
Now compare to what VoiceOver is doing in Finder on macOS, once you add an item to selection it announces the name of that item and says "2 items sleected" for example. Which is good behavior.

IMHO vscode does everything proper on our side with all the aria attributes in our trees and lists, however since NVDA and Orca have issues announcing selection in native Explorerer we can not expect that it works well for the VS Code explorer (which is a tree btw).

Hope this makes sense.

@pawelurbanski
Copy link
Contributor

pawelurbanski commented Mar 5, 2020 via email

@joanmarie
Copy link

"Contained in" seems to suffer from the same problem that "owned by" does. We have an issue for the latter, so I just added a comment mentioning "contained in". w3c/aria#748

@joanmarie
Copy link

  1. AtkSelection VS Code does not control this, we simply set aria-selected and I believe it is up to Chrome to report this to the screen reader. ...

Yeah that is why I filed this bug:

  1. Thanks for linking issue 1058961 ....

Since you reopened this bug, I won't file another. Unless I'm misunderstanding you.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 6, 2020

@joanmarie Yeah, no action needed, I missed that you were the one who just opened the Chrome bug. Thanks for doing that!
Also thanks for linking the aria issue, I will check it out.

@LeonarddeR
Copy link

In VS Code Insiders, I can now get the actual number of selected items, this is great. I will look into making this work more smoothly in NVDA by means of a pull request.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 10, 2020

@LeonarddeR great thanks!

@LeonarddeR
Copy link

@isidorn Is there an accessible way to inspect de html tray that's being generated by the VS Code gui? There seem to be some small inconsistencies with the preferred design pattern that cause item counts to fail.

Besides that, it doesn't seem to be possible to get a reliable reference from a child node of the tree view to the tree view itself. In the IA2 tree, all children have the tree view as their direct parent, but that's not in line with how the aria best practises tree view behaves.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 10, 2020

@LeonarddeR the only way to inspect the generated HTML is via Chrome dev tools. However I am not sure how accessible they are. Can you try it out and if it is not good let me know and i can help
F1 > Toggle developer tools Navigate to the Elements tab and there you should have the whole HTML tree.

Can you elabore on your second point, what is exactly not in line with aria best pracites regarding our tree? Is it that treeitems are not direct children of the tree? Unfortunetly that is just how our tree is architectured, and this is not something we could easily change. Do you know why is this a best practice? Since it would pose quite some limitations on how clients implement their trees.

@ajborka
Copy link

ajborka commented Mar 10, 2020

Is this discussion the reason why the settings tree doesn't speak with Orca? It appears that the focus and selection states are different. For example, Open settings, put focus on the tree, press orca+a to enter focus mode, then try navigating the tree. The result is that Orca fails to speak the new leaf/node name even though the tree item gains focus. Will this get fixed soon?

@joanmarie
Copy link

@ajborka I have VSCode built locally the other day and Orca master, and Orca speaks the items in the tree. So I think the answer is that it is already fixed. (?)

@ajborka
Copy link

ajborka commented Mar 10, 2020

I have the latest 1.44 build and the settings tree doesn't read reliably with Orca master. Is there a different build you are using? If VSCode master, how did you build it?

@LeonarddeR
Copy link

Can you elabore on your second point, what is exactly not in line with aria best pracites regarding our tree? Is it that treeitems are not direct children of the tree? Unfortunetly that is just how our tree is architectured, and this is not something we could easily change. Do you know why is this a best practice? Since it would pose quite some limitations on how clients implement their trees.

The point is that information like aria-posinset and aria-setsize only tells the screen reader how many items there are and what number the current item has in the group. It still relies on the several sets (e.g. sub folders) belong into their own group container, as noted in the best practise. At the moment, no positional information other than level is communicated.
Would it be possible to wrap all sub items in their own div with the group role?

@isidorn
Copy link
Contributor Author

isidorn commented Mar 11, 2020

@ajborka you are talking about the settings, that is a special implementation of the list. Can you please file a new issue and ping me on it, thank you. In the meantime as a workaround you can usee the settings in json via F1 > Preferences: Open Settings (JSON). @joanmarie just uses latest insiders bulid like you, but she is talking about the Explorer tree and you are talking about Settings (which is the only one that is a bit special)

@LeonarddeR the current strcuture is A > B > C > All Children. Currently the parent node A has the role. We can also set the role on the C which directly contains all Children. Should the role then be set both on A and C or only on C. This is not so elegant since all the other aria attirbutes are set on A and we would like to keep it that way if possible.

@LeonarddeR
Copy link

LeonarddeR commented Mar 11, 2020

note: I need to carefully look at the examples I wrote here, I don't think the nesting is correct. I don't have the time to do that yet.

@isidorn the problem is not the a>b>c structure and where the role is, but that all children are at the same level in the dom.

So, it's now this, right:

<div aria-level="1">parent</div>
<div aria-level="2">child</div>
<div aria-level="3">grand child</div>

It should be something among the lines of:

<div aria-level="1">parent</div>
<div role="group">
	<div aria-level="2">child</div>
	<div role="group">
		<div aria-level="3">grand child</div>
	</div>
</div>
```

May be something with aria-owns could work, see https://tink.uk/using-the-aria-owns-attribute/ . It should be used very carefully, but this might be a valid use case.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 11, 2020

@LeonarddeR oh that we definetly can not change. That is simply how our tree is imlemented - it uses the list and the list is not aware of this. Why can not the screen reader only use the aria level attributes, why does it need the actual HTML structure to be in a tree like form?

@ajborka
Copy link

ajborka commented Mar 11, 2020 via email

@LeonarddeR
Copy link

So, one entry looks as follows:

			<div class="monaco-list-row focused" role="treeitem" data-index="0" data-last-element="false"
				aria-setsize="2" aria-posinset="1" id="list_id_3_0" aria-selected="false" aria-label="Child1"
				aria-level="1" aria-expanded="true" draggable="true" style="top: 0px; height: 22px; line-height: 22px;">
				...
			</div>

Would it be possible to add an additional div within every monaco-list-row or monaco-tl-contents div with a role of group that has aria-owns containing a space separated list with IDs of direct descendants? This way, we might be able to convince the accessibility tree that there's a hierarchie.

If you don't understand what I mean, I can work out an example in HTML to explain this.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 12, 2020

@LeonarddeR I got what you mean. For that I have created a new feature request #92564
Let's continue the discussion there.
Thanks!

@bkoray
Copy link

bkoray commented Apr 23, 2020

Hello,

Would it be possible to make it so that screen reader announces selected ones as opposed to "not selected" ones?

Right now in order to hear collapsed / expanded status of a folder and hear the level I'm in, I am having to listen to "not selected" in each file / folder even though I am not trying to select anything. It would be much more useful if it were to announce "selected" when something is selected as opposed to announcing "not selected" on every file.

See part of my speech history with NVDA below:

data not selected collapsed level 2
dev not selected collapsed level 2
etc not selected expanded level 2
level 3 alternatives not selected collapsed

Version: 1.44.1587431043-insiders (user setup)
Commit: c359b95aa96eb0ec5a5e6cee2c3f4a4978e24963
Date: 2020-04-21T01:44:28.769Z
Electron: 7.1.11
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18362

@isidorn
Copy link
Contributor Author

isidorn commented Apr 24, 2020

@bkoray hello! This is up to the NVDA screen reader. Can you please file an issue against them?
The only way to fix this on the VS Code side is to "lie" and say that focused elements are also selected, which is incorrect.

@LeonarddeR
Copy link

Last time I tested, it wasn't possible to get a selection count from IAccessible2 in the explorer, but interesting enough, now it seems possible.

However, either I don't undrstand the logic correctly, or something is broken:

Steps to reproduce

  1. Open the file explorer
  2. Focus a not selected item
  3. Press shift+down arrow. Observe that that particular entry is selected.
  4. Press up arrow. NVDA says not selected for the first entry. I assume this is expected, as pressing arrows without shift normally undoes selection
  5. Press down arrow. Observe that the just selected item still seems selected. I would not expect so.

If my findings are really how VS Code should behave, I think we really need a description of how keyboard navigation behaves in these list/tree view controls, if there isn't any.

@isidorn
Copy link
Contributor Author

isidorn commented Apr 24, 2020

@LeonarddeR correct it behaves just like that.
Even though this behavior might look strange this is how we VS Code lists and trees behave from day 1. @joaomoreno can exaplain deeper on the exact reason why and if there are open issues where users complain about this. Aslo do we have this documented somehwere? I think not.

@LeonarddeR
Copy link

Well, in that case I understand the strong desire to know how many items are selected and possibly even which items are.

@isidorn
Copy link
Contributor Author

isidorn commented Aug 18, 2021

The further action planned here, and the upstream issue in Chrome is fixed, so we should soon pick it up and then screen readers should announce how many items are selected in our lists.
Thus closing

@isidorn isidorn closed this as completed Aug 18, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2021
@isidorn isidorn added the verified Verification succeeded label Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug list-widget List widget issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants