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

Breakpoint checkboxes have wrong role #52390

Closed
AccessibilityTestingTeam-TCS opened this issue Jun 20, 2018 · 18 comments · Fixed by #59236
Closed

Breakpoint checkboxes have wrong role #52390

AccessibilityTestingTeam-TCS opened this issue Jun 20, 2018 · 18 comments · Fixed by #59236
Assignees
Labels
a11ymas Issue from accessibility team accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues macos Issues with VS Code on MAC/OS X verified Verification succeeded
Milestone

Comments

@AccessibilityTestingTeam-TCS

Environment Details:
VSCode Version : 1.24.0
OS Version : HighSierra10.13.5

Additional Details:
MAS Violated: MAS 1.3.1
ScreenReader : VoiceOver

Repro Steps:

  1. Launch VS Code while VoiceOver is ON.
  2. Navigate to Activity Bar and select "View Debug " button.
  3. Navigate to "All Exceptions" and "User Handled Exceptions" checkboxes in "Breakpoints" section.

Actual:
Screenreader reads the tick boxes as list boxes. The role of the "All Exceptions" and "User Handled Exceptions" controls is incorrect.

Expected:
Screenreader should read the proper role of the controls as tickboxes so that proper action can be performed by the user.

Recommendations:
Provide proper role to the "All Exceptions" and "User Handled Exceptions" controls as tick boxes.(checkboxes)
or,
Refer below link which is repository of bug fixes code snippets:
https://microsoft.sharepoint.com/teams/msenable/mas/pages/browse-fixes.aspx

User Impact:
The screenreader users will not be able to know the correct role of the controls to perform appropriate actions on the controls present on the screen.

MAS Reference:
MAS 1.3.1 - https://microsoft.sharepoint.com/teams/msenable/_layouts/15/WopiFrame.aspx?sourcedoc={54f28d1f-a2d1-4dcd-84e1-5c9b87e8aba4}

Attachment For Reference:
A11y_VSCode_ViewDebug_VoiceOver_TickboxAsListBox.pptx

Does this issue occur when all extensions are disabled?: Yes

@weinand weinand assigned isidorn and unassigned weinand Jun 20, 2018
@weinand weinand added debug Debug viewlet, configurations, breakpoints, adapter issues accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues labels Jun 20, 2018
@isidorn isidorn added this to the July 2018 milestone Jun 20, 2018
@isidorn isidorn changed the title [Accessibility]A11y_VSCode_ViewDebug_ScreenReader: VoiceOver reads the tick box as list box. Breakpoint checkboxes have wrong role Jul 4, 2018
@isidorn isidorn added the bug Issue identified by VS Code Team member as probable bug label Jul 4, 2018
@isidorn
Copy link
Contributor

isidorn commented Aug 2, 2018

For this to be done the list would have to support an option to pass in the aria role similar as to how it currently allow the aria-lebel to be passed.
Since from the breakpoints view I can not style the appropriate element.
@joaomoreno thoughts?

@isidorn isidorn modified the milestones: July 2018, August 2018 Aug 2, 2018
@joaomoreno
Copy link
Member

Yeah we probably need that.

@isidorn
Copy link
Contributor

isidorn commented Aug 2, 2018

@joaomoreno would you like a PR or you want to look into it?

@joaomoreno
Copy link
Member

joaomoreno commented Aug 6, 2018

PR!

@cleidigh
Copy link
Contributor

cleidigh commented Aug 7, 2018

@isidorn
FYI
I believe the breakpoint checkboxes exist within a tree wiget. Since this uses role=tree the active descendents must have role=treeitem

There seems to the some ambiguity and inconsistency for the support of interactive elements,
within a aria tree. I have found two issues while working with the new settings editor,
the original element roles get superseded by the tree item and input box labels
do not get read correctly by the screen reader. This is my experience on Windows with NVDA.
Electron 2.x may play a role in this problem.

Ping me if you run into similar issues addressing this and want some more details.
I will also try to identify some of the accessibility issues to pick up some slack for August.

Related: #52862

@isidorn
Copy link
Contributor

isidorn commented Aug 24, 2018

@joaomoreno currently the list just has the option to pass the aria label for the whole list, not for every element.
Should we add an option aria-role which would get applied for every item, or each item would get pinged for the aria-role. In this case we would need to introduce some AccessibilityProvider the same way tree has one.
Also a pointer on where the list actually renders and would change the aria-role would be helpful.

@cleidigh
Copy link
Contributor

cleidigh commented Aug 24, 2018

I am doing a lot of CCing since various issues involve various people but they're all related:

@isidorn
@joaomoreno
@bpasero
@roblourens
I'm going to make an overall issue for this. I have been doing a lot of playing around working on the setting side . It has revealed several similar issues. I also found a great resource that includes some important information that the official specs are slightly ambiguous.
http://whatsock.com/training/matrices/

Both list and tree widgets have several issues associated with this.

  • Both only use role=tree as the top level aria role . This alters descendent roles which can distort output
  • Widget rows are always role=treeitem
  • Each tree item also has the attributes for level ,position and position in set to help the reader
  • My research and experimentation shows that editable/interactive elements are not clearly supported
    within a aria tree. Both descendent element roles and their labeling become unpredictable despite
    some ambiguity in the specs. I found it was implicit trees are always read-only/non- editable

as I said all do an issue for this, but here's my suggestions:

  • Allow the tree widget to accept an option for the main role
  • Form based roles as top-level will not require any HTML structure change
  • Use above for list also
  • Investigate changing the main tree operation to use the newer treegrid role/structure
    This would require HTML structure change using a table structure (grid probably small tweaks), this is explicitly editable
    and should be able to act like both or tree , list and even grid
  • Determine if list widget should always be one-dimensional, with the ability to have intractable elements
    I have an experimental PR/not cleaned up yet for my solution for settings to get both roles and labels correct for the reader. For now were going to do this all within settings, but it makes sense to consider better solutions within the widgets.
    Settings: Fix aria labels using role=form. Fixes: #54836 #57127 (comment)

Trying to coerce the current tree shows a bunch of inconsistencies even if certain things work sometimes
when the spec would imply they're not appropriate the reader still try to do their job.

@isidorn
Copy link
Contributor

isidorn commented Aug 29, 2018

@cleidigh thanks for the reply, I have just discussed this with @joaomoreno and we have come up with a solution pretty similar to what you are prosposing - so we are on the same page.

  1. We will add a getAriaRole to the IAccessibilityProvider of the Tree
  2. We will add a IAccessibilityProvider to the List which can be passed as an option when the list is created. The IAccessiblityProvider will be able to pass the role the same way as for the tree

Those are the first steps. After we do that we can see what is also missing.
Anyways, we will not do this in endgame -> september

@isidorn isidorn modified the milestones: August 2018, September 2018 Aug 29, 2018
@cleidigh
Copy link
Contributor

@isidorn

Sounds good, some thoughts:

  • IMHO the main widget role option should be an enum (tree, form, section, landmark)
    arbitrary roles could cause problems
  • the "branch/row" roles should be derived from the main role above, again arbitrary could cause problems

If a main role does not support particular attributes, they should be removed within the widget automatically
e.g. in a form no levels, position et cetera

Is this something you are doing or would you like me to pick it up for September since I've worked on this already?

@roblourens
Copy link
Member

@cleidigh do we need to get rid of the role now that we have a nicer way to do it? I tested with NVDA and it actually seems ok like this, not sure why.

@cleidigh
Copy link
Contributor

@roblourens
I'll take a look at this to see if any changes are necessary.

@cleidigh
Copy link
Contributor

@roblourens
I commented in the PR because I think were still missing the ability to set the role of the tree widget itself.
#59236

Unless I missing something, we can now set the role of the individual items but not the tree itself, although it looks like you can for a pure list. Whack me if I have missed something.

I can change to use the new getAriaRole callback until we see if anyone objects to me adding the role

@isidorn
Copy link
Contributor

isidorn commented Sep 25, 2018

After investigation and testing I had to revert the PR. Reason being that the VoiceOver on mac only properly reads out things for the combination of tree / treeitem roles. For list / listitem roles and for checkbox it is silent all the time.
Using checkbox on windows also keeps nvda silent.
Thus it seems it does not make sense to customize these roles since it breaks screen reader behavior
Due to this -> reopening this issue

@isidorn isidorn reopened this Sep 25, 2018
@isidorn isidorn modified the milestones: September 2018, October 2018 Sep 25, 2018
@cleidigh
Copy link
Contributor

@isidorn
cc: @roblourens
you ran into several of the things I did when working on the new settings editor which added some different ARIA requirements. I got things to work for all input types with a few games. You can see these working on both Windows and OS X. As I mentioned previously I found that there are plenty of inconsistencies in how the readers deal with input elements that are descendents of widget roles such as tree. After experimenting with a lot of different combinations I got each of the inputs to work. In the case of settings I used a primary role of form but I also had to change some of the individual element roles including checkbox.

I am more handicapped on OS X so I was not yet able to make an example there, but I just have to do similar things to what we did with settings. I think I can get this to work in the same fashion. I will play with it for October.

Check out the new settings editor with voiceover...

@isidorn
Copy link
Contributor

isidorn commented Sep 26, 2018

@cleidigh got it. Feel free to do a PR and ping me on it and we can look into it in October

@chrmarti chrmarti added the a11ymas Issue from accessibility team label Oct 2, 2018
@isidorn
Copy link
Contributor

isidorn commented Oct 29, 2018

Moving to november, not something for endgame

@roblourens
Copy link
Member

The role and 'isChecked' seem to be seen by VoiceOver, but I don't hear it read the set size or posInSet, dunno if that's an issue.

@roblourens roblourens added the verified Verification succeeded label Jul 31, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 22, 2019
@isidorn isidorn added macos Issues with VS Code on MAC/OS X windows VS Code on Windows issues and removed windows VS Code on Windows issues labels Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11ymas Issue from accessibility team accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues macos Issues with VS Code on MAC/OS X verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants