fix(aria-expanded) on submenues add aria prop #79775

gbmeow · 2019-08-25T23:19:08Z

@isidorn I am trying to figure out how to solve this aria-expended issue.
Can you advise me that we would try to add/remove the following?
on
<a class="action-menu-item monaco-submenu-item" aria-expended="true">

<li class="action-item focused" role="presentation">
	<a class="action-menu-item monaco-submenu-item" role="menuitem" tabindex="0" aria-checked="false" aria-haspopup="true" aria-posinset="6" aria-setsize="18" style="color: rgb(255, 255, 255); background-color: rgb(9, 71, 113);" *aria-expended="true"*>
		<span class="menu-item-check" role="none" style="background-color: rgb(255, 255, 255);"></span>
		<span class="action-label" aria-label="Open Recent">Open Recent</span>
		<span class="submenu-indicator" aria-hidden="true" style="background-color: rgb(255, 255, 255);"></span>
	</a>
</li>
```

isidorn · 2019-08-26T10:03:00Z

@georgebatalinski thanks for the PR. Currently we are in Endgame week which means we are doing testing and do not take new features - thus assigning this to September.

On the best location where we would tackle the expanded State, @sbatten would be the best since he owns the menus.
@sbatten let me know if you do not have time and I can also review / tackle this next milestone

gbmeow · 2019-08-26T13:25:56Z

@sbatten would you be able to give me some context around how the menu is structured?

this.container
this.element
this.item

I am working on trying to figure out if there is no conflict with these on the a element
aria-checked="false" aria-haspopup="true" aria-expended="true"

sbatten · 2019-08-26T18:23:03Z

@georgebatalinski If I understand correctly, this should only be set to true when the submenu is open. It looks like this PR adds it when the element is created which means that it will always be true.

I'll gladly explain about the structure to help out here. Firstly, the section you are modifying is a class called SubmenuMenuActionViewItem. An instance of this is responsible for rendering a single item in a menu that can be selected to open a submenu. e.g. Preferences in the File menu.

this.element points to the dom element which belongs to the greater menu and is generic to any entry such as the separators or normal menu entries like Save All. this.item is a child of that element and has the text for Preferences and the > submenu indicator. The submenu that gets created is a descendant of this.element in the dom and a sibling of this.item. I think that means that this.item is the correct element to have attribute aria-expanded and if so should be updated when the submenu is created and destroyed. This happens at the following locations respectively.

vscode/src/vs/base/browser/ui/menu/menu.ts

Line 699 in d93c8c4

private createSubmenu(selectFirstItem = true): void {

vscode/src/vs/base/browser/ui/menu/menu.ts

Line 687 in d93c8c4

private cleanupExistingSubmenu(force: boolean): void {

Also notice that you typed aria-expended in the PR but it should be aria-expanded.

gbmeow · 2019-08-27T22:27:15Z

@sbatten this is amazing context - thank you so much. I will update it

gbmeow · 2019-08-29T19:48:47Z

Tests completed:

Check that aria-expanded is used on elements a or button (** not sure if this is required)
Check that the toggling action correctly changes the state of the aria-expanded attribute
(https://www.w3.org/WAI/GL/wiki/Using_aria-expanded_to_indicate_the_state_of_a_collapsible_element)

@isidorn is it correct for an element to have both aria-checked and aria-expended?

<a class="action-menu-item monaco-submenu-item" role="menuitem" aria-checked="false" aria-expended="true" ...>

isidorn · 2019-08-30T09:15:19Z

@georgebatalinski according to my knowledge, aria-checked is used for elements which have a checkmark (toggle state). In theory an element can have both the aria-checked and aria-expanded, however I am not sure what element in vscode falls in this category? I am not aware of any menu entry that is both a checkmark and is a submenu.

From your comment I am assuming that you added the aria-expanded to every element. However I would only expect that elements whic have submenus have the aria-expanded property. I appologise if I did not mention this before.

Example for aria-checked element should be File > Auto Save
Example aria-expanded element should be File > Open Recent

gbmeow · 2019-08-31T00:05:59Z

@isidorn I will review this tomorrow, thank you for the input on aria-checked menu elements 🚀

gbmeow · 2019-09-03T23:34:37Z

I am still working on figuring out which elements have the aria-checked on them
I will spend some time debugging the action aspect of the menu
in order to confirm if only the relevant menu items, get the aria-checked

will update soon (after fixing the build errors on my machine from yarn :)

isidorn · 2019-09-04T08:19:25Z

@georgebatalinski great thanks
If you can not fix those yarn errors let us know (I faced something similar on my win machine last week)

gbmeow · 2019-09-17T00:53:34Z

@isidorn this PR is ready to go - I had sometime today (so started investigating my yarn issue)
I will created another PR for the aria-checked, but it should not interfere anyway, did not want to hold you up

gbmeow · 2019-09-17T02:25:28Z

gyp ERR! stack Error: spawn C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\MSBuild\15.0\Bin\MSBuild.exe ENOENT

isidorn · 2019-09-17T08:30:18Z

@georgebatalinski I think you forgot to push your latest changes to this PR, since the last change I see is from 19 days ago.

gbmeow · 2019-09-18T17:57:04Z

@isidorn the PR has been ready since then (19 days ago),
I was just investigating something else (outside of this PR)

I am still having yarn issues - trying to get it to work via
https://gist.github.com/noseratio/d81888eda77620e526c7eb2c3b75cba8

isidorn · 2019-09-19T15:39:21Z

Looks good to me. Though leaving final review to @sbatten since he owns the menus.
Note that I did not verify that this works - since I have issues building on Windows as well.

@jeanp413

credit to @jeanp413 for calling out this duplication

msftclas · 2019-10-17T22:33:44Z

All CLA requirements met.

sbatten

noticed there were still 2 places we weren't setting back to false so I updated those

sbatten · 2019-10-17T22:41:16Z

src/vs/base/browser/ui/menu/menu.ts

@@ -796,13 +790,7 @@ class SubmenuMenuActionViewItem extends BaseMenuActionViewItem {
 			this.submenuDisposables.add(this.parentData.submenu.onDidCancel(() => {
 				this.parentData.parent.focus();

-				if (this.parentData.submenu) {


@jeanp413 credit to you for calling this out in your PR. In this PR it became necessary to fix without further duplicating code.

isidorn · 2019-10-18T07:54:07Z

@georgebatalinski can you please sign the contributer license agreement and we will merge this PR in.
The bot mentions it [here])(#79775 (comment))

sbatten · 2019-10-28T18:24:18Z

@georgebatalinski pinging for CLA signing

sbatten · 2019-11-13T22:35:25Z

@georgebatalinski are you still available to sign the CLA?

gbmeow · 2019-11-26T03:13:51Z

@sbatten yes - I will sign it

gbmeow · 2019-11-26T15:02:09Z

@sbatten and @isidorn thank you very much - I am printing this PR :) so much amazing context around how the menu system works 👍

@jeanp413

* fix(aria-expended) on submenues add aria prop * (aria-expanded) on sub menu creation ⭐ * add missing places for setting to false credit to @jeanp413 for calling out this duplication

fix(aria-expended) on submenues add aria prop

ea11bd0

gbmeow mentioned this pull request Aug 25, 2019

Menu: Proper role and state is not announced to the screen reader users. #77075

Closed

RMacfarlane assigned sbatten Aug 26, 2019

isidorn self-assigned this Aug 26, 2019

isidorn added this to the September 2019 milestone Aug 26, 2019

(aria-expanded) on sub menu creation ⭐

7a2adef

gbmeow changed the title ~~fix(aria-expended) on submenues add aria prop~~ fix(aria-expanded) on submenues add aria prop Aug 29, 2019

isidorn approved these changes Sep 19, 2019

View reviewed changes

isidorn removed their assignment Sep 19, 2019

sbatten self-requested a review September 23, 2019 18:25

mjbvz modified the milestones: September 2019, October 2019 Oct 10, 2019

sbatten added 2 commits October 17, 2019 14:59

Merge branch 'master' into pr/georgebatalinski/79775

f63c4cf

add missing places for setting to false

a2aa8de

credit to @jeanp413 for calling out this duplication

sbatten approved these changes Oct 17, 2019

View reviewed changes

sbatten added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues menus Menu items and widget issues labels Oct 17, 2019

sbatten reviewed Oct 17, 2019

View reviewed changes

alexdima modified the milestones: October 2019, November 2019 Nov 6, 2019

sbatten merged commit 1d51b43 into microsoft:master Nov 26, 2019

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aria-expanded) on submenues add aria prop #79775

fix(aria-expanded) on submenues add aria prop #79775

gbmeow commented Aug 25, 2019 •

edited

Loading

isidorn commented Aug 26, 2019

gbmeow commented Aug 26, 2019

sbatten commented Aug 26, 2019

gbmeow commented Aug 27, 2019

gbmeow commented Aug 29, 2019

isidorn commented Aug 30, 2019 •

edited

Loading

gbmeow commented Aug 31, 2019

gbmeow commented Sep 3, 2019

isidorn commented Sep 4, 2019

gbmeow commented Sep 17, 2019

gbmeow commented Sep 17, 2019

isidorn commented Sep 17, 2019

gbmeow commented Sep 18, 2019

isidorn commented Sep 19, 2019 •

edited

Loading

msftclas commented Oct 17, 2019 •

edited

Loading

sbatten left a comment

sbatten Oct 17, 2019

isidorn commented Oct 18, 2019

sbatten commented Oct 28, 2019

sbatten commented Nov 13, 2019

gbmeow commented Nov 26, 2019

gbmeow commented Nov 26, 2019

fix(aria-expanded) on submenues add aria prop #79775

fix(aria-expanded) on submenues add aria prop #79775

Conversation

gbmeow commented Aug 25, 2019 • edited Loading

isidorn commented Aug 26, 2019

gbmeow commented Aug 26, 2019

sbatten commented Aug 26, 2019

gbmeow commented Aug 27, 2019

gbmeow commented Aug 29, 2019

isidorn commented Aug 30, 2019 • edited Loading

gbmeow commented Aug 31, 2019

gbmeow commented Sep 3, 2019

isidorn commented Sep 4, 2019

gbmeow commented Sep 17, 2019

gbmeow commented Sep 17, 2019

isidorn commented Sep 17, 2019

gbmeow commented Sep 18, 2019

isidorn commented Sep 19, 2019 • edited Loading

msftclas commented Oct 17, 2019 • edited Loading

sbatten left a comment

Choose a reason for hiding this comment

sbatten Oct 17, 2019

Choose a reason for hiding this comment

isidorn commented Oct 18, 2019

sbatten commented Oct 28, 2019

sbatten commented Nov 13, 2019

gbmeow commented Nov 26, 2019

gbmeow commented Nov 26, 2019

gbmeow commented Aug 25, 2019 •

edited

Loading

isidorn commented Aug 30, 2019 •

edited

Loading

isidorn commented Sep 19, 2019 •

edited

Loading

msftclas commented Oct 17, 2019 •

edited

Loading