MenuBar: do not focus on hover #607

gabalafou · 2023-07-06T14:03:11Z

When the MenuBar class was created, it was designed so that whenever a user hovers over an item in the menu with a mouse, the active index would change to the index of the item being hovered. The setter for the active index also had a side effect that would update the DOM in order to apply the active CSS class to the item being hovered so that clients consuming this class could style the item (just like the :hover pseudo-class).

Later, in an attempt to make the Lumino menu bar more accessible, tabindex=0 values were added to the menubar items via #187. With the menu bar items keyboard-focusable (but with no CSS styling to indicate it), it made sense to try to synchronize the active index with the item focussed.

However, this had one unfortunate side effect, which was that whenever a user moved their mouse over a menu bar item, the app would move browser focus from anywhere else in the app onto that item.

This PR is an attempt to fix that issue and to put the menu bar's focus management on better footing.

Originally I refactored the menu bar class to simply not change activeIndex on mousemove, but I realized that that would probably break a lot of downstream code. For years, since the beginning it seems, the convention of the Lumino menu bar has been that if an item in the menu bar is hovered, then Lumino will apply the lm-mod-active class. In the interest of not breaking people's code, I decided to keep that convention.

gabalafou

Left some inline comments

packages/default-theme/style/menubar.css

gabalafou · 2023-07-07T09:08:06Z

packages/widgets/src/menubar.ts

-    // Update focus to new active index
-    if (
-      this._activeIndex >= 0 &&
-      this.contentNode.childNodes[this._activeIndex]
-    ) {
-      (this.contentNode.childNodes[this._activeIndex] as HTMLElement).focus();
-    }
-


This block of code was added via #187.

However, this code presents two problems for me. For one, I have found side effects in setters to be problematic. Another, bigger problem is that it causes :focus-visible styling to be activated after page load even if the user hasn't touched the keyboard.** As far as I can tell, all browsers right now have this behavior (why, I don't know). Here it happens because node.focus() gets called when this.activeIndex is changed during a mousedown event. It makes it impossible for me to get the kind of behavior that I want for :focus-visible, which is to only show the focus ring when the user is using the keyboard instead of the mouse to interact with the page—in other words, to match the behavior that you get in a Windows file menu bar.

There's another issue, too, that I try to address explicitly in this PR by being more explicit about the definition of activeIndex. It's very hard to make activeIndex always consistent with focus. The consistency we had before this PR and which we continue to have after this PR is:

If an item node at index i in the menu bar has focus, then activeIndex is equal to i.

This is assured by the onfocus handler attached to each item node. But the reverse was (and is) not always true. If activeIndex is equal to i (not -1), the item node at index i may or may not be focussed, depending on whether the menu at i is open.

** You can verify this behavior for yourself. Go to the ReadTheDocs Lumino DockPanel example, and before pressing any keys or clicking anywhere else, use your mouse to move and click around the menu bar. You should see your browser's default focus ring appearing even though you've only been using your mouse, and even though the MenuBar doesn't have any HTML elements that get a focus ring by default.

gabalafou · 2023-07-07T09:48:57Z

packages/widgets/src/menubar.ts

@@ -648,7 +662,6 @@ export class MenuBar extends Widget {

    // Stop the propagation of the event. Immediate propagation is
    // also stopped so that an open menu does not handle the event.
-    event.preventDefault();


If we always call preventDefault() on mousedown, it prevents the node from receiving browser focus unless we call node.focus() ourselves, but if we call focus on mousedown we cause current browsers to activate :focus-visible stylings. In other words, if we call focus on mousedown, then a focus ring appears around the items in the menu bar (on initial page load) even if the user hasn't touched the keyboard. I have a code pen that demos this behavior.

packages/widgets/src/menubar.ts

gabalafou · 2023-07-07T11:41:19Z

packages/widgets/src/menubar.ts

@@ -408,7 +422,7 @@ export class MenuBar extends Widget {
   */
  protected onActivateRequest(msg: Message): void {
    if (this.isAttached) {
-      this.activeIndex = 0;
+      this._focusItem(0);


Instead of having focus happen as a side-effect of setting activeIndex, I rewrote this code to rely on the fact that the item's onfocus handler sets the active index.

Note: if menubar.activate() is called on page load or after a mouse event, this will cause :focus-visible styling to be applied.

gabalafou · 2023-07-07T11:53:43Z

packages/widgets/src/menubar.ts

-      this.activeIndex = -1;
-      this.node.blur();


There is no way currently to focus the menu bar node, so there is no way to blur it either.

The escape key has not been able to blur the menu bar node since #465. Before that PR, the menu bar node could be focussed (because tabindex). But after that PR, the menu bar node is just a div with no tabindex value, so it cannot ever be focussed.

gabalafou · 2023-07-07T12:09:06Z

packages/widgets/src/menubar.ts

-  private _evtMouseLeave(event: MouseEvent): void {
-    // Reset the active index if there is no open menu.
-    if (!this._childMenu) {
+  private _evtFocusOut(event: FocusEvent): void {


The MenuBar class was originally written to handle mouse and keyboard interaction; however, it really wasn't concerned with browser focus. For example, in the original version, you could use the left and right arrow keys to "move" from one item to another in the menu bar, but browser focus did not move when you pressed those keys.

There was no way to even put browser focus on individual item nodes in the menu bar until #187. As a result, it made sense to set the active index to -1 when the mouse left the menu bar. But now that the active index is closely tied to focus, to be consistent we shouldn't reset the active index just because the mouse leaves the menu bar.

However, we can safely reset the active index if the menu bar loses focus.

gabalafou · 2023-07-27T12:37:50Z

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

@@ -489,7 +513,10 @@ describe('@lumino/widgets', () => {

      context('keydown', () => {
        it('should bail on Tab', () => {
-          let event = new KeyboardEvent('keydown', { keyCode: 9 });
+          let event = new KeyboardEvent('keydown', {
+            cancelable: true,


If you don't set cancelable to true then it's pointless to check event.preventDefault because it will always be false.

I searched the codebase for defaultPrevented and fortunately there were only a few places, all fixed in this PR.

marthacryan · 2023-08-09T21:38:55Z

Thank you so much for working on this - and for the super thorough descriptions, research, and codepens! It really helps to have all the context with this stuff cause it can be sort of subtle. I haven't tested this locally but as the person that added a lot of the changes that you're addressing in this PR, I approve! I also might recommend someone that's worked a bit more with Lumino checks this out too (@afshin) cause it's been a while since I made changes to Lumino.

fcollonval · 2023-08-10T10:22:18Z

Hey @gabalafou would you mind running:

yarn api
yarn lint

To fix the CI?

gabalafou · 2023-08-16T09:35:22Z

@fcollonval done! But running yarn api changed a bunch of API doc files that have nothing to do with the changes in this PR, not sure what's going on...

fcollonval

Hey @gabalafou

Thanks for pushing on this. I rebased (this was the reason of so many api doc changes) and added the menubar to the doc to ease testing.

i only have one comment otherwise it looks good to me.

packages/widgets/src/menubar.ts

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

fcollonval

Thanks @gabalafou

github-actions bot assigned gabalafou Jul 6, 2023

gabalafou commented Jul 7, 2023

View reviewed changes

gabalafou force-pushed the no-focus-on-hover branch from 97424bf to 8e91f4b Compare July 27, 2023 11:52

gabalafou marked this pull request as ready for review July 27, 2023 15:00

gabalafou mentioned this pull request Jul 27, 2023

Team update - Jul 26, 2023 Quansight-Labs/jupyter-a11y-mgmt#229

Closed

gabalafou commented Jul 27, 2023

View reviewed changes

gabalafou requested review from marthacryan and afshin August 1, 2023 11:36

This was referenced Aug 1, 2023

Focus visible test Quansight-Labs/jupyter-a11y-testing#33

Open

Fix focus issues in Lumino menu bar Quansight-Labs/jupyter-a11y-mgmt#225

Open

fcollonval added the bug Something isn't working label Aug 10, 2023

gabalafou and others added 11 commits August 17, 2023 17:51

wip

e941c37

do not allow empty menu to be active

50954e9

remove underline

c4b4a38

newline

bb793c6

return test line

60c3f00

fix prevent default

ce2286e

reduce repeated comments

06a82c2

missed a spot

43540b1

move some comments around

4bf5a0b

yarn api && yarn lint

eb8750b

Add menubar example to doc

888038f

fcollonval force-pushed the no-focus-on-hover branch from bc50bd9 to 888038f Compare August 17, 2023 16:01

fcollonval requested changes Aug 17, 2023

View reviewed changes

packages/widgets/src/menubar.ts Outdated Show resolved Hide resolved

gabalafou and others added 2 commits August 17, 2023 11:19

Update packages/widgets/src/menubar.ts

e515980

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

Fix CI

cc19217

fcollonval approved these changes Sep 14, 2023

View reviewed changes

fcollonval merged commit 9938dff into jupyterlab:main Sep 14, 2023
15 of 16 checks passed

gabalafou mentioned this pull request Feb 18, 2024

File browser items in focus state draw a rectangle around the itemText node, which disappears when hovering on any menu jupyterlab/jupyterlab#14678

Closed

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MenuBar: do not focus on hover #607

MenuBar: do not focus on hover #607

gabalafou commented Jul 6, 2023 •

edited

Loading

gabalafou left a comment

gabalafou Jul 7, 2023

gabalafou Jul 7, 2023

gabalafou Jul 7, 2023

gabalafou Jul 7, 2023

gabalafou Jul 7, 2023

gabalafou Jul 27, 2023

marthacryan commented Aug 9, 2023

fcollonval commented Aug 10, 2023

gabalafou commented Aug 16, 2023 •

edited

Loading

fcollonval left a comment

fcollonval left a comment

MenuBar: do not focus on hover #607

MenuBar: do not focus on hover #607

Conversation

gabalafou commented Jul 6, 2023 • edited Loading

gabalafou left a comment

Choose a reason for hiding this comment

gabalafou Jul 7, 2023

Choose a reason for hiding this comment

gabalafou Jul 7, 2023

Choose a reason for hiding this comment

gabalafou Jul 7, 2023

Choose a reason for hiding this comment

gabalafou Jul 7, 2023

Choose a reason for hiding this comment

gabalafou Jul 7, 2023

Choose a reason for hiding this comment

gabalafou Jul 27, 2023

Choose a reason for hiding this comment

marthacryan commented Aug 9, 2023

fcollonval commented Aug 10, 2023

gabalafou commented Aug 16, 2023 • edited Loading

fcollonval left a comment

Choose a reason for hiding this comment

fcollonval left a comment

Choose a reason for hiding this comment

gabalafou commented Jul 6, 2023 •

edited

Loading

gabalafou commented Aug 16, 2023 •

edited

Loading