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

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

Merged
merged 4 commits into from Nov 26, 2019

Conversation

@georgebatalinski
Copy link
Contributor

georgebatalinski commented Aug 25, 2019

@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

This comment has been minimized.

Copy link
Contributor

isidorn commented Aug 26, 2019

@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

@isidorn isidorn self-assigned this Aug 26, 2019
@isidorn isidorn added this to the September 2019 milestone Aug 26, 2019
@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Aug 26, 2019

@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

This comment has been minimized.

Copy link
Member

sbatten commented Aug 26, 2019

@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.

image

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.

private createSubmenu(selectFirstItem = true): void {

private cleanupExistingSubmenu(force: boolean): void {

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

Copy link

brightest-dazes left a comment

I agree must be done in an efficient amount of time

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Aug 27, 2019

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

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

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Aug 29, 2019

Tests completed:

  1. Check that aria-expanded is used on elements a or button (** not sure if this is required)
  2. 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

This comment has been minimized.

Copy link
Contributor

isidorn commented Aug 30, 2019

@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

Screenshot 2019-08-30 at 11 14 25

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Aug 31, 2019

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

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Sep 3, 2019

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

This comment has been minimized.

Copy link
Contributor

isidorn commented Sep 4, 2019

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

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Sep 17, 2019

@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

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Sep 17, 2019

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

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Sep 17, 2019

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

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Sep 18, 2019

@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

This comment has been minimized.

Copy link
Contributor

isidorn commented Sep 19, 2019

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.

@isidorn isidorn removed their assignment Sep 19, 2019
@sbatten sbatten self-requested a review Sep 23, 2019
@mjbvz mjbvz modified the milestones: September 2019, October 2019 Oct 10, 2019
sbatten added 2 commits Oct 17, 2019
credit to @jeanp413 for calling out this duplication
@msftclas

This comment has been minimized.

Copy link

msftclas commented Oct 17, 2019

CLA assistant check
All CLA requirements met.

Copy link
Member

sbatten left a comment

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

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

if (this.parentData.submenu) {

This comment has been minimized.

Copy link
@sbatten

sbatten Oct 17, 2019

Member

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

@isidorn

This comment has been minimized.

Copy link
Contributor

isidorn commented Oct 18, 2019

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

@sbatten

This comment has been minimized.

Copy link
Member

sbatten commented Oct 28, 2019

@georgebatalinski pinging for CLA signing

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

This comment has been minimized.

Copy link
Member

sbatten commented Nov 13, 2019

@georgebatalinski are you still available to sign the CLA?

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Nov 26, 2019

@sbatten yes - I will sign it

@georgebatalinski

This comment has been minimized.

Copy link
Contributor Author

georgebatalinski commented Nov 26, 2019

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

@sbatten sbatten merged commit 1d51b43 into microsoft:master Nov 26, 2019
2 checks passed
2 checks passed
VS Code #20191017.102 succeeded
Details
license/cla All CLA requirements met.
Details
devrsi0n pushed a commit to devrsi0n/vscode that referenced this pull request Nov 27, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.