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

Terminal tab should surface split information in aria label #122332

Closed
isidorn opened this issue Apr 27, 2021 · 8 comments · Fixed by #122823
Closed

Terminal tab should surface split information in aria label #122332

isidorn opened this issue Apr 27, 2021 · 8 comments · Fixed by #122823
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 terminal General terminal issues that don't fall under another label terminal-tabs verified Verification succeeded
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Apr 27, 2021

Testing #122242

Have multiple terminal tabs open. In one tab have a couple of split terminals. Visually they look like they belong to a tree, however this information is not conveyed to the screen reader. Here are some ideas on how to improve this:

  • Put aria-level = 2 for elements that are inside the tree. Currently all elements have an aria-level = 1. If you prefer to leave it like that then the role of the whole thing should be a list and not a tree
  • Modify aria label of an element in case he is part of a split
@isidorn isidorn added the accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues label Apr 27, 2021
@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2021

We're converting this into a list next iteration #121745, I think we need to indicate to the screen reader that it's a split though. Not sure how the special characters are read out now.

@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug terminal General terminal issues that don't fall under another label terminal-tabs labels Apr 27, 2021
@Tyriar Tyriar added this to the April 2021 milestone Apr 27, 2021
@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2021

@isidorn hmm, should we be surfacing the fact that it's a split terminal to the screen reader at all? I think it's purely a UI feature so it shouldn't make a difference if you can't see the UI?

@Tyriar Tyriar added the under-discussion Issue is under discussion for relevance, priority, approach label Apr 27, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Apr 27, 2021

@Tyriar oh great that you moving to the list. You should have a correct role list then.
I would still convey the group number as part of the aria label, since we do the same for editors in editor groups. It is useful for users to know how things are grouped on the screen in case there are navigation actions like switch tab, or move focus inside a terminal tab. Just make sure that the group label is at the end of the list element aria label so it is easy to skip.

Special character is not read out now because it is not part of the aria label.

@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2021

Good point 👍

@Tyriar Tyriar changed the title Terminal tab list does not convey tree structure to screen reader Terminal tab should surface split information in aria label Apr 27, 2021
@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2021

Scoped this issue down, conversion to list will be handled in #121745

@Tyriar Tyriar closed this as completed in f1c66b6 Apr 27, 2021
@Tyriar Tyriar removed the under-discussion Issue is under discussion for relevance, priority, approach label Apr 27, 2021
@isidorn
Copy link
Contributor Author

isidorn commented Apr 29, 2021

I see the code changes which look good, however when I try this out the group is not applied to the aria label, it is the same as before.

I have a ZSH, one tab with one terminal, and another tabs with two terminals. And all their labels are just zsh

@isidorn isidorn reopened this Apr 29, 2021
@isidorn isidorn added the verification-found Issue verification failed label Apr 29, 2021
@meganrogge
Copy link
Contributor

inspect

When I inspect a split terminal tab label, it has the correct aria label.

@isidorn
Copy link
Contributor Author

isidorn commented Apr 30, 2021

Oh but you are setting the aria-label on the wrong HTML element label-name
You should use the List AccessibilityProvider so the aria-label is properly set on the monaco-list-row element. It has to be on that element, since that element has focus and that is what the Screen Reader will read.
So I think it should be fixed here
https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/terminal/browser/terminalTabsWidget.ts#L67

@Tyriar Tyriar added the candidate Issue identified as probable candidate for fixing in the next release label Apr 30, 2021
@Tyriar Tyriar modified the milestones: April 2021, May 2021 Apr 30, 2021
@Tyriar Tyriar removed the candidate Issue identified as probable candidate for fixing in the next release label Apr 30, 2021
meganrogge added a commit that referenced this issue May 3, 2021
@meganrogge meganrogge removed the verification-found Issue verification failed label Jun 2, 2021
@isidorn isidorn added the verified Verification succeeded label Jun 4, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2021
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 insiders-released Patch has been released in VS Code Insiders terminal General terminal issues that don't fall under another label terminal-tabs verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@isidorn @Tyriar @meganrogge and others