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

Editor title being read out once suggestion is accepted #89098

Open
isidorn opened this issue Jan 22, 2020 · 23 comments
Open

Editor title being read out once suggestion is accepted #89098

isidorn opened this issue Jan 22, 2020 · 23 comments
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug macos Issues with VS Code on MAC/OS X upstream Issue identified as 'upstream' component related (exists outside of VS Code) windows VS Code on Windows issues
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Jan 22, 2020

found by @pawelurbanski

  1. Open the suggest widget
  2. Accept a suggestion
  3. Editor aria label is begin read out 🐛

The issue I believe is that the focus moves back to the editor which makes ScreenReaders read out the editor aria label. I am not sure how to tackle this and am open to suggestions @pawelurbanski

Assigning to January so I look into fixing this week.
Can reproduce both with NVDA and VoiceOver on mac.

@isidorn isidorn added bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues labels Jan 22, 2020
@isidorn isidorn added this to the January 2020 milestone Jan 22, 2020
@isidorn isidorn self-assigned this Jan 22, 2020
@isidorn
Copy link
Contributor Author

isidorn commented Jan 22, 2020

@pawelurbanski tried this out again and the issue is as I have suspecte. The activedescendant gets set to the suggest the screen reader nicely reads this. Once the user accepts a suggestion the activedescendent is unset, thus the focus is again in the editor -> making the screen reader read that editor aria label.
So my question to you is if you know how to make the screen reader ignore a focus change (activedescendent change)?

@pawelurbanski
Copy link
Contributor

I’ve just pulled the changes from Master – there is no change in behavior, or I eed to merge locally a specific branch.

I managed to confirm the following:

  1. The main window title consists of the current file name in the editor and the VSC edition label.
  • The file name is very likely connected to the verbosity setting of file names and path in the editor group, which makes sense.
  1. After creating a few files with new editors associated:
  • The correct file name is announced when switching editors by using Ctrl+Tab,
  • I get feedback following the focus announcing correct file names,
  1. If we could stick to the bbehavior of 1.41 where the main window title gets the file name, we would not have to use the aria-label on the editor, since it is already announced with focus changes when switching editors.
  2. This functionality is already present and the aria-label was an addition to provide detailed information.
  3. I can still overwrite the 'name' proprty, which corresponds to the main window title in the add-on to reduce the verbosity, namely not read what is now inserted in the aria-label.
  4. As for focus management and
    As for the focus and activedescendent:
  5. Focus is the basic mechanism to get handles to controls.
  • You could detect getting focus after completion and set the main window title to an empty string, which would very likely prevent the screen reader from reading too much.
  • It would read the title in othe useful cases, such as the one when switching editors by pressing Ctrl+Tab.
  1. The aria-activedescendent property is something I am not that familiar with. It seems to me that it is recently implemented correctly by the browsers.
  • Since it is applied to a pop-up menu, I keep getting announcements when inside the editor that it is an editor and has pop-up.
  • The current word in the editor being a property or function, must have this property attached for it can have other properties and methods.
  • I need to investigate how to avoid reading it all the time, since the editor contents is the most important.

@pawelurbanski
Copy link
Contributor

Clarification after further testing:

    1. In both 1.41 and the current development 1.42 the name of the file opened in the editor is read aloud. In addition to that every time the editor gets focus the information about the editor picker and group is added, which sounds very chatty...

Possible logic:

  1. Provide just the file name or path and file name according to the settings,
  2. If there is no editor group just skip this information,
  3. If there is an editor group - just announce its name,
  4. The announcement about being in the editor picker is unnecessary.

The aria-label on the editor overwrites the defaults and it seems that it prevents the add-ons to screen readers overwrite its value as a result. It prevents the add-on to disable reading the name of the editor when getting the focus back to the textarea field.
Addressign the case when the user does not use any plugins, we can check if the focus was obtained after completion and set the name of the textarea to an empty string.
I guess we do not need to label the editor in such a strong way. Every user working on some text or code knows where he or she is focused. You can obviously scrol up or down to check the contents and so on.

@isidorn
Copy link
Contributor Author

isidorn commented Jan 22, 2020

Great, thanks a lot for the feedback and ideas.
I was discussing something similar with @alexdima and we came to a similar conclusion.

  • There is duplication between the VS Code title and the Editor aria label. And one of them should get dumped. We were thinking of making the VS Code title not contain the editor name when a screen reader is detected

Also:

  • If there is only one editor group - we should not read this information. I agree and we planned to do this
  • Just anouncing the name of the editor group would not work, since the name is 1 and I think that is not clear enough
  • I do not get the eidtor picker announcment. Can you please clarify this? What is exactly being read out?

We also though of introducing a workaround that the suggest widget clears the editor aria label before the suggestion is accepted and sets it back afterwards. This is a hack workardoun and are open for better ideas on how to solve this issue.

@pawelurbanski
Copy link
Contributor

pawelurbanski commented Jan 22, 2020 via email

@isidorn
Copy link
Contributor Author

isidorn commented Jan 23, 2020

  1. I do not get "picker" read out. Do you have some special setup? When I swithc between Untitled editors I get the following read out Untitled-2 editor, Editor Group 2 edit has auto complete multi line
  2. We can update the group name to be just group. However group is a term which the Mac Voice Over uses and we got feedback from @ivanfetch that this can be confusing and ambigous
  3. We will try to make the VS Code title shorter so Screen readers are not so chatty
  4. We would handle the focus in such a way that the Suggest Widget would clear the Editor aria label and with a timeout set it back to the previous state. That way only the focus from the suggest would not read it out. It is a hack, but I have no better ideas how to solve this.

@pawelurbanski
Copy link
Contributor

pawelurbanski commented Jan 23, 2020 via email

@isidorn
Copy link
Contributor Author

isidorn commented Jan 23, 2020

For 3) I have created this issue #89175
For this issue I would look into trying to fix 4), that editor title is being read out on suggest accept.

As for a code poitner for 4, I think we should simply here do our magic hack. setAriaOptions should take another parameter ariaLabel and we should temporarily clear it.
I can look into creating a PR for this, or if you would like to let me know so I leave it up to you. Thanks

@isidorn
Copy link
Contributor Author

isidorn commented Mar 20, 2020

TL DR: once a suggestion is accepted NVDA and VoiceOver reads the editor title.
Suggest box has role listbox, and suggest items have role option.
This issue is that NVDA and VoiceOver should not read the editor title once the suggestion is accepted.
Orca works as expected here.

To put it in HTML screen reader terms: if element A has focus and has activeDescendent set to element B and element B is a listbox on closing of B arialabel of A should not be re-read.

Created NVDA issue nvaccess/nvda#10887

@isidorn isidorn added upstream Issue identified as 'upstream' component related (exists outside of VS Code) macos Issues with VS Code on MAC/OS X windows VS Code on Windows issues labels Mar 20, 2020
@isidorn isidorn modified the milestones: March 2020, On Deck Mar 20, 2020
@MarcoZehe
Copy link
Contributor

Question or wild idea: Is the suggestions popup an accessibility child of the editor, or somewhere else in the accessibility hierarchy? If it were part of the editor's sub tree, e. g. via aria-owns on the editor pointing to the ID of the suggestions popup container. This would put it and all its children into the editor element's accessibility sub tree (not DOM sub tree). NVDA and some other AT have a habit of rereading a hierarchical chain of elements when focus changes. So if focus only changed from a child to a parent once aria-activedescendant is cleared, that might reduce verbosity.

However, if you are already aria-owning the suggestions popup into the editor accessibility sub tree, then please ignore me. :-)

@isidorn
Copy link
Contributor Author

isidorn commented Mar 24, 2020

@MarcoZehe great suggestion to use aria-owns. I have just tried it out and unfortunetly this does not fix the issue both with NVDA and VoiceOver.
We can push this aria-owns change either way if you think it makes sense. Since currently we only set the activedescendent and the suggest widget is not an actual DOM child of our textArea which has the focus while a user is typing.

@MarcoZehe
Copy link
Contributor

No, if it doesn't fix the issue, no need to include it. Sometimes it does make sense, but it is nothing that should be used just because one can. Only adds complexity which in this case isn't needed.

@isidorn
Copy link
Contributor Author

isidorn commented Mar 24, 2020

@MarcoZehe ok, thanks for the idea anyways.

@pawelurbanski
Copy link
Contributor

pawelurbanski commented Mar 24, 2020 via email

@LeonarddeR
Copy link

@isidorn I really dig the aria-owns idea. In NVDA, it should already help for braille actually. We could also consider making NVDA less verbose by default if focus moves from a child to a parent. I guess there is some stuff we can ignore in that case, such as states and label.
Would you be able to provide a test build with this in place?

@LeonarddeR
Copy link

LeonarddeR commented Feb 5, 2022

Actually, here is some interesting info about aria-activedescendant that also mentions aria-owns and aria-controls. I think from this section, option 3 suboption 1 applies to VS Code. That means aria-controls should be implemented on the text area pointing at the suggestions list box. See also this documentation from the ARIA 1.2 best practices which explicitly states that aria-controls should be implemented correctly. it has a structure that is somehow similar to VS Code.

  Now what I'd really love in Chromium is a possibility to get an IAccessible2 releation from the editor to the active descendant, similar to how you can get the controllerFor relation that points at the element referenced with aria-controls. Unfortunately, neither Chromium nor Firefox implement such a relation. @MarcoZehe do you have an idea where I could propose such a thing? As far as I know, whenever screen reader focus is overridden with aria-activedescendant, there's no way for the screen reader to get the dom focus, which is still the editor.

@MarcoZehe
Copy link
Contributor

Now what I'd really love in Chromium is a possibility to get an IAccessible2 releation from the editor to the active descendant, similar to how you can get the controllerFor relation that points at the element referenced with aria-controls. Unfortunately, neither Chromium nor Firefox implement such a relation. @MarcoZehe do you have an idea where I could propose such a thing?

There are several parts to this question. First you'd need to find out if either the IAccessible2 or ATK standards already know of such a relation you ask for. If not, you'd need to file bugs against the appropriate standards bodies to implement these.
Once these are agreed upon, the next step is to take this to the ARIA working group within the W3C to adjust the mappings for these attributes to include these relations.
Once that's done, file bugs in the Chromium and Firefox bug trackers to implement these relations.
And finall, get JAWS, NVDA, and Orca to support these relations.

Note, though, that I no longer work for Mozilla, and am not part of the ARIA working group any more, so I can only advise on how things are done, but am not in any position to help drive this forward any more than you are.

@isidorn
Copy link
Contributor Author

isidorn commented Feb 7, 2022

We currently do not set neither aria-owns or aria-controls on the VS Code side but we could easily add it.
@joanmarie can maybe help from the Chromium side of things regarding Leon's comment.

@joanmarie
Copy link

@isidorn Unless I am misunderstanding, that comment suggests that aria-controls should be set (i.e. by VSCode/authors). I'm pretty sure that Chromium already has proper implementation for both aria-owns and aria-controls.

@isidorn
Copy link
Contributor Author

isidorn commented Feb 7, 2022

@joanmarie yeah I misunderstood, sorry for the ping. I got it once I read the comment on the NVDA side.

So the following should be done:

  • Every content widget should set the id on its dom element to it's content widget id
  • Once a contentWidget is shown (or registered) the editor needs to set aria-controls on the textArea pointing to the id of the widget

@hediet @alexdima @jrieken what do you think? Is this reasonable? We can also scale it down to the suggest widget, but I thought a more general content widget approach would work better.

Gist of the issue we are trying to solve: on editor content widget close, screen reader should not re-read editor aria label and content.

@LeonarddeR
Copy link

Ugh, I tried to write a prototype for NVDA based on the ARIA example, but it turns out to be harder than I thought. Aria-controls points to the suggestions list, but as soon as that list disappears, there is no way using IAccessible2 to get a reference to that list any more. To put it into steps:

  1. Editor has focus. IA2 controllerFor relation yields no results
  2. Type something
  3. Suggestions list pops up, aria-activedescendant is set and therefore focus goes to an item in the list
  4. Suggestion is chosen or dismissed
  5. NVDA tries to check the IA2 relation controllerFor on the editor to see whether focus came from a controlled object, but as the list was already dismissed, it yields no results again.

Another approach is checking the controlled by relation on every focus, and if that applies, cache the controlled by until it gets focus again. I assume this can be pretty costly though and therefore I'm a bit reluctant to do this.

@isidorn
Copy link
Contributor Author

isidorn commented Feb 9, 2022

@LeonarddeR thanks for providing the details. Let us know how that investigation continues. We are open to do the changes required on the VS Code side if this can improve the overall experience.

@isidorn isidorn removed the important Issue identified as high-priority label Dec 5, 2022
@akash07k
Copy link

Any update on this issue?

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 macos Issues with VS Code on MAC/OS X upstream Issue identified as 'upstream' component related (exists outside of VS Code) windows VS Code on Windows issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants