Improve the menubar accessibility #465

scmmmh · 2022-11-14T14:27:20Z

This PR will improve accessibility issues with the menubar component, bringing it closer to being WCAG2.1 compliant. In particular it will

Set the tabindex properties as specified;
Handle the focus setting as specified, both when focus first moves into the menubar and also when focus moves out of it;

This is my first bit of work with the lumino codebase, so all comments, particularly around architecture are much appreciated.

I haven't added any tests yet, that is the next step.

welcome · 2022-11-14T14:27:23Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

scmmmh · 2022-11-14T19:40:43Z

I've added tests and the PR is now ready for review.

gabalafou

I'm so stoked to see this PR, thanks @scmmmh!

Is there any way we could correct the asymmetry between tabbing forward versus backward (shift-tabbing)? Right now, what I'm seeing with your changes is that when I tab through the example page, it focuses on the skip link first, then the File menu item, then the text area. But starting from the text area and shift-tabbing backwards, I get: File menu item, then the entire menubar, then the skip link. Tabbing forward, the user does not see the menubar receive focus, but tabbing backwards they do. Does my description make sense? I can make a video if it's not clear, just lemme know! 😃

I think this is a step in the right direction, but ultimately I want to make the Lumino menubar behave more like the ARIA example menubar. There are some differences between that implementation and the Lumino menubar after this PR, which I would like to correct, for example:

Pressing the escape key takes the user out of the menubar in Lumino, whereas in the ARIA example, it closes any open menus but keeps them in the menubar. (Tab key is how to get out.)
Pressing the tab key while inside an open Lumino menu does nothing, whereas in the ARIA example it navigates you out of the menubar.
When you leave the Lumino menubar it forgets the last menu item that you were on, whereas the ARIA example remembers.

And so on and so forth.

But I think this PR incrementally moves in a direction aligned with a future better UX, so I don't think it needs to be held back by any of those outstanding discrepancies.

The only reason I'm not approving it right now is because I have some questions about the code. I also cannot review the version numbering stuff in the package.json file. @afshin could you take a look at that or ping somebody who knows how to review it?

gabalafou · 2022-11-15T18:15:42Z

packages/widgets/tests/src/menubar.spec.ts

+          bar.dispose();
+        });
+
+        it('should loose focus on tab key', () => {


Suggested change

it('should loose focus on tab key', () => {

it('should lose focus on tab key', () => {

gabalafou · 2022-11-15T18:17:28Z

packages/widgets/tests/src/menubar.spec.ts

+          bar.dispose();
+        });
+
+        it('should loose focus on shift-tab key', () => {


Suggested change

it('should loose focus on shift-tab key', () => {

it('should lose focus on shift-tab key', () => {

or is "loose" the British way of spelling it?

😆 No, this is just the one spelling mistake that I continue to make. I've always misspelled that and it looks like I always will.

packages/widgets/src/menubar.ts

gabalafou · 2022-11-15T18:28:52Z

examples/example-menubar/src/index.ts

+    node.appendChild(h1);
+    const label = document.createElement('label');
+    label.appendChild(
+      document.createTextNode('A textarea to demonstrate the tab handling.')


Could you add some more prose explaining the inclusion of the textarea on the page? I'm afraid that in the future without a better explanation, it's not going to be quite clear why the textarea is on the page, or what purpose it serves.

I'll add a comment. It is basically there so that there is some content that can be tabbed to. Otherwise the tab out of the menubar takes you into the browser UI and that can often be a bit harder to see where it has gone (I guess an a11y issue in itself).

examples/example-menubar/src/index.ts

gabalafou · 2022-11-15T19:11:22Z

packages/widgets/src/menubar.ts

    if (kc === 9) {
+      if (!event.shiftKey) {
+        this.activeIndex = -1;


Sorry, even with the comment I'm still not quite sure I get what this line is doing.

I thought if it's not resetting the activeIndex, then I should be able to do the following on the example page:

Tab to the menu bar

Right arrow key over to Edit (it's highlighted now)

Click on the text area input ("Edit" is still highlighted)

Press Shift + Tab and...
Expected behavior: "Edit" remains highlighted and if I press Enter it should open the Edit menu.
Actual behavior: the File menu items becomes highlighted and if I press Enter it opens the File menu.

gabalafou · 2022-11-15T19:15:50Z

packages/widgets/src/menubar.ts

+  /**
+   * Handle the `'focus'` event for the menu bar.
+   */
+  private _evtFocus(event: FocusEvent): void {


Why do we wish to reset the active menu item when the menu bar receives focus?

I would like to make the Lumino menubar behave more like the example menubar in the ARIA Authoring Practices Guide.

This is caused by the interpretation of what it means for the "menubar to receive focus", as I put in the main reply. I'll switch to the interpretation used in the example, which will also make the code much simpler.

gabalafou · 2022-11-15T19:30:21Z

packages/widgets/tests/src/menubar.spec.ts

+          expect(
+            bar.contentNode.contains(document.activeElement) &&
+              bar.contentNode !== document.activeElement
+          ).to.equal(true);


I'm not sure this test actually tests that the menu bar loses focus. Perhaps the test should be renamed, or perhaps these 4 lines should be modified and moved below line 803 (the event dispatch).

Suggested change

expect(

bar.contentNode.contains(document.activeElement) &&

bar.contentNode !== document.activeElement

).to.equal(true);

expect(bar.node.contains(document.activeElement)).toBe(false);

remembering the odd property that a node.contains itself, so document.activeElement.contains(document.activeElement) will always be true I think.

packages/widgets/tests/src/menubar.spec.ts

gabalafou · 2022-11-15T19:32:23Z

packages/widgets/tests/src/menubar.spec.ts

+            bar.contentNode.contains(document.activeElement) &&
+              bar.contentNode !== document.activeElement
+          ).to.equal(true);
+          let event = new KeyboardEvent('keydown', { keyCode: 9 });


Did you forget the shift modifier?

Yes. I was fiddling with getting the tab behaviour to work in the test and in the process of that removed too much :-).

scmmmh · 2022-11-16T09:41:19Z

Thanks for all the detailed feedback. I'll iterate the code.

The main reason for the divergence from the menubar example is that I was following the spec, which says "when a menubar receives focus, keyboard focus is placed on the first item". Looking at the example and your comments, it has become clear to me that there is an ambiguity here, whether the "menubar" is the actual HTML element with the role="menubar" or the component as a whole. In the first interpretation the HTML element must have tabindex="0" to be focusable. As a result you have this slightly different behaviour tabbing forward and backward and also a lot of the code I have is to handle the resulting edge cases. In the second interpretation, none of that is necessary and only the menuitems are focusable. Since that is the interpretation used by the example, I'll rework the code to follow that one. It will make the code a good bit simpler, so that'll be nice.

I agree that the long-term aim is to make the menubar work exactly like the ARIA menubar and this is just a first, incremental step towards that. I just wanted to take some first steps in the codebase and get some feedback, before I start making bigger changes.

scmmmh · 2022-11-16T11:05:07Z

If somebody could add a label, then that would reduce the number of failure e-mails I receive and would be much appreciated :-).

blink1073 · 2022-11-16T11:23:17Z

If somebody could add a label, then that would reduce the number of failure e-mails I receive and would be much appreciated :-).

Done, and thanks for working on this!

scmmmh · 2022-11-16T12:31:49Z

Done, and thanks for working on this!

Happy to help. I'm responsible for accessibility in my department, so every issue I fix is a complaint that doesn't come to me 😃

gabalafou

This is so great! I really love the direction this is going in!

When trying this out locally, I noticed that if I get rid of the focus index and also get rid of the line that resets the active index, I think I get the same behavior minus the inconsistency I mention in one of my inline comments.

As such, I'm marking my review as "request changes"—not because I'm sure the code needs to change but because I have questions. Thank you for your patience with the review!

examples/example-menubar/package.json

packages/widgets/src/menubar.ts

gabalafou · 2022-11-18T03:15:52Z

packages/widgets/src/menubar.ts

    if (kc === 9) {
+      this.activeIndex = -1;


This introduces an inconsistency that makes me uncomfortable. The active index gets reset when you tab out of the menubar but not when you click out. On the example page, if you tab to the File menu, then click on the button, the File menu stays shaded. But if you tab to the File menu, then tab to the button, the File menu loses its shading.

I can live with this inconsistency for now, but we will need to address it in the future.

Co-authored-by: Frédéric Collonval <[email protected]>

gabalafou

I would be okay with merging this code as-is, but I left some inline comments with some suggested changes.

gabalafou · 2022-11-22T15:20:48Z

packages/widgets/src/menubar.ts

@@ -772,6 +783,11 @@ export namespace MenuBar {
     */
    readonly active: boolean;

+    /**
+     * The index of the item in the list of items.


I think this description was meant for a different variable? focusable is a boolean, not an index.

While we're at it, could we rename this variable? To be, focusable means "able to be focussed" but I think what this variable really means is more like "should be made focusable" (by setting tabindex to 0).

What do we think about makeFocusable or enableFocus or enableTabIndex or...?

packages/widgets/src/menubar.ts

scmmmh · 2022-11-22T15:52:09Z

I've updated the code in line with your comments.

scmmmh · 2022-11-24T11:19:20Z

Do I need to do anything to make the API changes test pass?

blink1073 · 2022-11-24T13:42:32Z

Do I need to do anything to make the API changes test pass?

I believe you need to call yarn run api and then commit the changes.

fcollonval

Thanks a lot @scmmmh and thanks @gabalafou for the review

welcome · 2022-11-28T10:57:15Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

fcollonval · 2022-11-28T10:57:47Z

@meeseeksdev please backport to 1.x

lumberbot-app · 2022-11-28T10:57:53Z

Oops, something went wrong applying the patch ... Please have a look at my logs.

scmmmh · 2022-11-28T11:03:29Z

Thanks a lot @scmmmh and thanks @gabalafou for the review

Happy to help. More soon, I hope.

Mark

…ity)

* Backport #465 on branch 1.x (Improve the menubar accessibility) * Fix tests * Fix tests requiring bubbling

Bring keyboard handling closer in line with WCAG2.1

601c14b

Code cleanup

41bcc6f

github-actions bot assigned scmmmh Nov 14, 2022

scmmmh added 6 commits November 14, 2022 14:54

Fix focusing the menubar on event

29d2e8b

Make the tests check that the activeElement is a descendant

4231171

More work on the tests

588348d

Fixed the focus / tab tests

ad90278

Added missing test for the space activation

fc2c5f9

Cleanup the example

f4d6525

scmmmh marked this pull request as ready for review November 14, 2022 19:40

scmmmh changed the title ~~WIP: Improve the menubar accessibility~~ Improve the menubar accessibility Nov 14, 2022

krassowski mentioned this pull request Nov 14, 2022

Make focus visible (mostly CSS) jupyterlab/jupyterlab#13415

Merged

gabalafou self-requested a review November 15, 2022 18:13

gabalafou requested changes Nov 15, 2022

View reviewed changes

scmmmh added 7 commits November 16, 2022 10:07

Switch to the ARIA menubar example interaction pattern

0720b26

Set the activeIndex in onActivateRequest

4fe3e21

Remove old code

480267b

Code cleanup

8eb88b2

Remove test for scenario that cannot occur

44fe991

Remember the last focused menu item

0a5ebb4

Test fix

8e6551e

blink1073 added the enhancement New feature or request label Nov 16, 2022

Fix license header job

ff5d5f1

gabalafou mentioned this pull request Nov 17, 2022

Team update - Nov 16, 2022 Quansight-Labs/jupyter-a11y-mgmt#179

Closed

gabalafou requested changes Nov 18, 2022

View reviewed changes

Update examples/example-menubar/package.json

cdc8143

Co-authored-by: Frédéric Collonval <[email protected]>

gabalafou approved these changes Nov 22, 2022

View reviewed changes

scmmmh added 2 commits November 22, 2022 15:45

Small rewordings

7ef2870

Small doc changes

acac897

gabalafou approved these changes Nov 23, 2022

View reviewed changes

Widgets API update

89c792a

fcollonval approved these changes Nov 28, 2022

View reviewed changes

fcollonval merged commit 14c3380 into jupyterlab:main Nov 28, 2022

fcollonval added this to the 1.x milestone Nov 28, 2022

scmmmh deleted the improve-menubar-accessibility branch November 28, 2022 11:55

fcollonval added a commit to fcollonval/lumino that referenced this pull request Nov 28, 2022

Backport jupyterlab#465 on branch 1.x (Improve the menubar accessibil…

f32db8d

…ity)

fcollonval mentioned this pull request Nov 28, 2022

Backport #465 on branch 1.x (Improve the menubar accessibility) #476

Merged

fcollonval added a commit that referenced this pull request Nov 28, 2022

Backport #465 on branch 1.x (Improve the menubar accessibility) (#476)

1a0bdf9

* Backport #465 on branch 1.x (Improve the menubar accessibility) * Fix tests * Fix tests requiring bubbling

krassowski mentioned this pull request Jan 11, 2023

ARIA MenuBar keyboard interaction improvements #477

Draft

This was referenced Jan 12, 2023

Team update - Jan 11, 2023 Quansight-Labs/jupyter-a11y-mgmt#189

Closed

Focus not always visible on JupyterLab initial page Quansight-Labs/jupyter-a11y-mgmt#167

Closed

gabalafou mentioned this pull request Jul 7, 2023

MenuBar: do not focus on hover #607

Merged

github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the menubar accessibility #465

Improve the menubar accessibility #465

scmmmh commented Nov 14, 2022

welcome bot commented Nov 14, 2022

scmmmh commented Nov 14, 2022

gabalafou left a comment •

edited

Loading

gabalafou Nov 15, 2022

gabalafou Nov 15, 2022

scmmmh Nov 16, 2022

gabalafou Nov 15, 2022

scmmmh Nov 16, 2022

gabalafou Nov 15, 2022

gabalafou Nov 15, 2022

scmmmh Nov 16, 2022

gabalafou Nov 15, 2022

gabalafou Nov 15, 2022

scmmmh Nov 16, 2022

scmmmh commented Nov 16, 2022

scmmmh commented Nov 16, 2022

blink1073 commented Nov 16, 2022

scmmmh commented Nov 16, 2022

gabalafou left a comment

gabalafou Nov 18, 2022

gabalafou Nov 22, 2022

gabalafou left a comment

gabalafou Nov 22, 2022

scmmmh commented Nov 22, 2022

scmmmh commented Nov 24, 2022

blink1073 commented Nov 24, 2022

fcollonval left a comment

welcome bot commented Nov 28, 2022

fcollonval commented Nov 28, 2022

lumberbot-app bot commented Nov 28, 2022

scmmmh commented Nov 28, 2022

	it('should loose focus on tab key', () => {
	it('should lose focus on tab key', () => {

	it('should loose focus on shift-tab key', () => {
	it('should lose focus on shift-tab key', () => {

Improve the menubar accessibility #465

Improve the menubar accessibility #465

Conversation

scmmmh commented Nov 14, 2022

welcome bot commented Nov 14, 2022

scmmmh commented Nov 14, 2022

gabalafou left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scmmmh commented Nov 16, 2022

scmmmh commented Nov 16, 2022

blink1073 commented Nov 16, 2022

scmmmh commented Nov 16, 2022

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scmmmh commented Nov 22, 2022

scmmmh commented Nov 24, 2022

blink1073 commented Nov 24, 2022

fcollonval left a comment

Choose a reason for hiding this comment

welcome bot commented Nov 28, 2022

fcollonval commented Nov 28, 2022

lumberbot-app bot commented Nov 28, 2022

scmmmh commented Nov 28, 2022

gabalafou left a comment •

edited

Loading