microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.67k stars 29.06k forks source link

List should announce how many items are selected #91061

Closed isidorn closed 3 years ago

isidorn commented 4 years ago
  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 commented 4 years ago

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 commented 4 years ago

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

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:

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 commented 4 years ago

@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 commented 4 years ago

@isidorn Wanna look into this?

isidorn commented 4 years ago

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

pawelurbanski commented 4 years ago

If attributes will be set correctly screen reader will do this job. It is announcing the change of state from unselected to selected. The number of selected item is not an issue, therefore do not use alerts to announce it. I guess that the use case will be to select a handful of items. If there will be the need for more, selecting all will be more frequent action or going the container route. For example using a folder or expanded node of a tree to select it and the objects contained to perform the action.

isidorn commented 4 years ago

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 https://github.com/microsoft/vscode/pull/92019 Docs for reference https://www.w3.org/WAI/PF/aria/states_and_properties#aria-multiselectable

pawelurbanski commented 4 years ago

What would be a few example use cases? Maybe we don’t need such precise behavior? For example: if the case is to select a few items to perform some batch action, we could try to do a bit of a hack and apply the: aria-checked attribute or aria-selected for that matter. The user would get very precise feedback about the items to which the action would be applied. The shortcut rule of thumb is that if some item was spoken and additionally displayed on a NBraille display if one is using it, it is something with focus – all the other states are additional information.

On a side note: I do not rcall if the number of selected items is announced in regular contexts such as: Windows explorer list with files selected and so on – will check with different NVDA settings when it comes to verbosity.

LeonarddeR commented 4 years ago

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 commented 4 years ago

cc @zersiax

isidorn commented 4 years ago

@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 commented 4 years ago

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:

joanmarie commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

isidorn commented 4 years ago

@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 commented 4 years ago

When it comes to NVDA and Orca it comes to the matter of verbosity. Hearing that you selected 3 items, which you already did does not make sense. Getting information that X and Y are selected when you scroll through some list of items can be helpful indeed. The case is when you cherry pick just a few files or commits in a branch.

LeonarddeR commented 4 years ago

In a list, NVDA considers the selected state redundant, as it is the default. Therefore, it only announces the unselected state. This is expected behavior.

joanmarie commented 4 years ago

When selection follows focus, Orca doesn't say "selected" because, as @leonardder just stated, that's the default/expected state. But if you use Ctrl+Arrow to move within a (native) tree then the focused item is not selected. Since that's not the expected thing, Orca will then say "not selected." If you use Ctrl+Space to toggle the selection of the current item, Orca will announce that.

I personally think that announcing the number of items selected after every selection change would cause Orca users to complain. HOWEVER, I do think it makes sense to add that information to Orca's whereAmI command output. That is already done when the native file explorer is in icon view rather than list/tree view. So I'll make that change generically, which will hopefully then JustWork(tm) with VSCode. Will this address your concerns?

isidorn commented 4 years ago

@leonardder ok that makes sense. Since VS Code has concepts of focus and selection, however since all actions are exectued on focused and selected things I think vscode should not be 100% honest here and we will continue setting aria-selected on focused items. However we will also do that on selected items. If we do not do that NVDA would always announce not selected when user moves focus in the list (which is the default up / down behavior)

@joanmarie the change and your suggestion makes sense. I think users would not complain if you announce it only when selection is larger than 1. But that is just mine assumption.

Thank you both for your help and I hope it is ok if I ping you in the future on other accessibility issues users face in VS Code.

joanmarie commented 4 years ago

@isidorn: I could also make it a preference. Orca has a number of preferences in this general ballpark e.g. announcing child position ("3 of 7", "4 of 7", etc.) during navigation. The impression I get is that most users don't want to hear that, but some do, so it's a preference which is disabled by default.

I'll see about making both of these changes in the next few days. Note that the whereAmI change can (likely) go into 3.36.x (which is permafrozen with respect to features, string changes, and UI changes) because I'm pretty sure I already have a localized string which will apply to this situation. But the new preference which I just mentioned in this comment will only be able to be committed to Orca master.

And thank you for working on accessibility in VS Code in general, and ensuring it works with Orca in particular. You rock!!

pawelurbanski commented 4 years ago

I hope you won’t mind another comment. I figured out a few scenarios that take into account current behavior of VS Code, NVDa and Orca:

  1. The user is scrolling through the items.

    • Focus without announcing the item as selected shall be enough,
    • It is obvious what item is in context,
  2. The user is scrolling to pick some items.

    • Adding the aria-selected attribute shall make the screen reader announce the fact of selecting the item,
    • Few items later the user does the same to select something,
    • He or she decides to unselect the first item so we remove the aria-selected attribute and the change shall be announced,
    • The number of items is of a secondary nature and as the others said it could annoy the users,

The best case to imagine this is adding changed files to the staging area before making a commit.

isidorn commented 4 years ago

Thank you very much for the feedback, I have merged in the PR https://github.com/microsoft/vscode/pull/92019 Thus now both selected and focused items in vscode list and trees will have aria-selected = true. Other items will have aria-selceted = false List and tree will also have multiselectable when they support multi selection.

Let's see how this behaves in practice. I do not think there will be noticable user changes, unless they set the Orca prefences which @joanmarie is about to push

@pawelurbanski feedback is always welcome. Yes, your 1. point should be covered we will not spam the user in saying things are selected. NVDA and Orca treat this as deafult behavior. Hopefully point 2 should also be covered. We shall see.

@joanmarie thanks for compliments :) No hurries in releasing these changes, whenever they land is good.

LeonarddeR commented 4 years ago

If we do not do that NVDA would always announce not selected when user moves focus in the list (which is the default up / down behavior)

I don't think that's a problem if the item itself is not selected. If up and down arrow focus another entry without selecting it, I'd like to know the unselected state. However, in the explorer, up and down both select and focus, right? So why should announce NVDA unselected in this case?

I think users would not complain if you announce it only when selection is larger than 1. But that is just mine assumption.

This is what NVDA tried to implement in https://github.com/nvaccess/nvda/pull/8879, where it works for tables. May be we can expand this behaviour.

joanmarie commented 4 years ago

@isidorn BTW, if you are in a list and press Orca+Shift+Up, does Orca present the selected item count and items? (That command speaks the selected text when you're in text content, but when you're not it looks for selected items.)

isidorn commented 4 years ago

@joanmarie I am actually having an issue triggering the Orca key. I am using a VM and a small apple keyboard that does not have a lot of buttons. I changed the orca modifier key to be ctrl, shit. However when I press ctrl+shift+up not gets announced, both in vs code and native explorer. Thus I think my orca key does not work in my vm with my keyboard. Any alternatives?

@leonardder no, in the explorer up and down only focuses and does not select. Expanding on the table selection behavior makes sense imho.

LeonarddeR commented 4 years ago

@leonardder no, in the explorer up and down only focuses and does not select.

Yet, when I focus an item in the explorer with the up and down arrows and press enter on it, it is opened/expanded. This somehow suggests that it was selected.

However, it looks like there isn't a selection indeed. I observe this behavior:

  1. In a folder with item 1, item 2 and item 3, focus item 1. It should be unselected according to what you said
  2. Hold shift and press down arrow twice. Focus is at item 3. Item 2 and item 3 are selected.
  3. When pressing delete, item 2 and 3 are deleted, item 1 stays around.

I think this behavior is really confusing, and it at least differs from Windows Explorer where focus = select. I wonder whether the underlying issue is more an usability issue than an accessibility issue. May be there's a good reason for the why this was designed?

isidorn commented 4 years ago

All actions are performed on focus, unles focus is part of a larger selection then actions are performed on the slection. We thought this bheavior is intutive and changing that now will break too much muscle memory of our users so unfortunetly that is not an option. And yes it is different from Windows Explorer. As to why we originaly diverged focus and selectino @joaomoreno would be the best to answer.

LeonarddeR commented 4 years ago

All actions are performed on focus, unles focus is part of a larger selection then actions are performed on the slection. We thought this bheavior is intutive and changing that now will break too much muscle memory of our users so unfortunetly that is not an option. And yes it is different from Windows Explorer.

Ah, this clarifies a lot for me.

SO, let me try to summarize things here:

Current state as of today

  1. Treeview has treeview role with the aria-multiselectable role if applicable
  2. Treeview items have role treeviewitem
  3. Position info is not exposed by NVDA. Is the group role used to wrap al children of a leaf?
  4. Focused items are reported as selected, even though they aren't. This has several drawbacks:
    1. People expect the shift+down arrow trick to select all items including the first item. This is currently not the case.
    2. Non contiguous selection is likely to be impossible with a screen reader

Proposal

I propose to tie aria-selected to selection only. While this has the drawback of not selected being reported for every item, it much more closely exposes the current state of the focused item. May be it can be an accessibility option that can be toggled? I would prefer more verbosity over in accurateness.

joanmarie commented 4 years ago

So I just built VS Code. Is the list/tree in question the file explorer?

If so, I see that selection does not follow focus. In addition, I've not yet found the keystroke to toggle the selection of the current item. What is it?

Regardless there seem to be a number of issues that are making it hard for Orca to do the right thing in this particular tree (e.g. AtkSelection claiming 0 selected items; the selected state being missing from items which I think I've selected, i.e. with Shift+Arrow).

Whether or not these are Chromium bugs or VS Code bugs remains to be determined, but the missing state and selection-interface issue are things I'm seeing using the Accerciser accessibility inspector (i.e. independent of Orca).

Where would be the best place to discuss these issues? In this bug? In spinoff bugs but this repo? (For stuff we determine to be Chromium issues, I'll of course file bugs there.)

pawelurbanski commented 4 years ago

I have an idea for the case of selecting separate items. While it is a hack, but what if we set the: aria-checked attribute? It would matter to screen users only – I guess that there is no CSS with some attribute selector that would change the look of the selected item. While it sounds like a hack Windows explorer allows to enable slection with checkboxes as an option. Yes, I know that it is a hack and not the clean way to do it, but we do not spend that much time in three views such as the file explorer or SCM view.

jvesouza commented 4 years ago

@joanmarie Please check bug #90893. If I'm not mistaken, there is information on how to select an item using a keystroke.

joanmarie commented 4 years ago

As an aside (given the bug title/summary): Right now Orca cannot announce how many items are selected (in a performant manner) because of https://bugs.chromium.org/p/chromium/issues/detail?id=1058961.

As another aside: In your tree, the element in between the treeitems and the tree appears to be a div without any ARIA role. As you will see in https://w3c.github.io/aria/#treeitem, treeitem can be in a group or a tree. So perhaps applying the group role to that intervening div? Won't fix the aforementioned bug, but would be correct.

isidorn commented 4 years ago

@leonardder we can change that aria-selected is set only on selected elements. I will push that right away and you can try vscode insiders next week to see how it behaves. In case users complain we can always revert this

@joanmarie we use the same list and tree implementation across vs code. And yes the Explorer is the most prominent example.

  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. Can you maybe create a new issue on the VS Code side and ping me @isidorn and then we usualy create a bug on the Chromium side and link from our repository. Please note that VS Code usualy lags 2 Chrome versions behind since we are build on top of Electron
  2. Thanks for linking issue 1058961 , due to that I will reopen this issue and mark it as upstream so users can see why this is not working
  3. The doc says contained in, so I think we are good. Even though the direct parent of a treeitem is not a tree, one of its ancestors is a tree and it contains the treeitem just not directly. Unless I am understadning the docs wrongly?
joanmarie commented 4 years ago

"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". https://github.com/w3c/aria/issues/748

joanmarie commented 4 years ago
  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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@leonardder great thanks!

LeonarddeR commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@isidorn https://github.com/isidorn : Screen readers depend on the browser's accessibility tree to convey structural information to a screen reader. This is why the HTML for the tree must be different than what you currently use. Just using a list which attempts to use structured nodes like a tree wont work. I assume it is the same with the settings tree, but that will get covered in another bug.

On Wed, Mar 11, 2020 at 8:34 AM Isidor Nikolic notifications@github.com wrote:

@leonardder https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/microsoft/vscode/issues/91061#issuecomment-597607079, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACN663TN2WHLMJUQUEI6HLLRG6AO7ANCNFSM4KYK6CNQ .

LeonarddeR commented 4 years ago

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.