MenuBar: do not focus on hover

.github/workflows/tests.yml

@@ -117,7 +117,7 @@ jobs:
       - uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1
       - uses: jupyterlab/maintainer-tools/.github/actions/check-links@v1
         with:
-          ignore_links: "./api/index.html examples/accordionpanel/index.html examples/datagrid/index.html examples/dockpanel/index.html https://mybinder.org/v2/gh/jupyterlab/lumino/.*"
+          ignore_links: "./api/index.html examples/accordionpanel/index.html examples/datagrid/index.html examples/dockpanel/index.html examples/menubar/index.html https://mybinder.org/v2/gh/jupyterlab/lumino/.*"
 
   check_release:
     runs-on: ubuntu-latest

docs/source/conf.py

@@ -34,7 +34,7 @@
 
 
 HERE = osp.abspath(osp.dirname(__file__))
-EXAMPLES = ["accordionpanel", "datagrid", "dockpanel"]
+EXAMPLES = ["accordionpanel", "datagrid", "dockpanel", "menubar"]
 
 # -- General configuration ------------------------------------------------
 
@@ -50,7 +50,6 @@
     'sphinx.ext.intersphinx',
     'sphinx.ext.mathjax',
     'sphinx_copybutton',
-
 ]
 
 myst_enable_extensions = ["html_image"]

docs/source/examples.rst

@@ -13,3 +13,5 @@ Rendered static examples
 `DataGrid <examples/datagrid/index.html>`_
 
 `DockPanel <examples/dockpanel/index.html>`_
+
+`MenuBar <examples/menubar/index.html>`_

packages/default-theme/style/menubar.css

@@ -44,3 +44,12 @@
   border-right: 1px solid #c0c0c0;
   box-shadow: 0px 0px 6px rgba(0, 0, 0, 0.2);
 }
+
+.lm-MenuBar-item:focus-visible {
+  outline: 2px solid #333;
+  outline-offset: -2px;
+}
+
+.lm-MenuBar-item[aria-disabled='true'] {
+  color: rgba(0, 0, 0, 0.37);
+}

packages/widgets/src/menubar.ts

@@ -145,6 +145,11 @@ export class MenuBar extends Widget {
       value = -1;
     }
 
+    // An empty menu cannot be active
+    if (value > -1 && this._menus[value].items.length === 0) {
+      value = -1;
+    }
+
     // Bail early if the index will not change.
     if (this._activeIndex === value) {
       return;
@@ -153,19 +158,6 @@ export class MenuBar extends Widget {
     // Update the active index.
     this._activeIndex = value;
 
-    // Update the focus index.
-    if (value !== -1) {
-      this._tabFocusIndex = value;
-    }
-
-    // Update focus to new active index
-    if (
-      this._activeIndex >= 0 &&
-      this.contentNode.childNodes[this._activeIndex]
-    ) {
-      (this.contentNode.childNodes[this._activeIndex] as HTMLElement).focus();
-    }
-
     // Schedule an update of the items.
     this.update();
   }
@@ -370,8 +362,8 @@ export class MenuBar extends Widget {
       case 'mousemove':
         this._evtMouseMove(event as MouseEvent);
         break;
-      case 'mouseleave':
-        this._evtMouseLeave(event as MouseEvent);
+      case 'focusout':
+        this._evtFocusOut(event as FocusEvent);
         break;
       case 'contextmenu':
         event.preventDefault();
@@ -387,7 +379,7 @@ export class MenuBar extends Widget {
     this.node.addEventListener('keydown', this);
     this.node.addEventListener('mousedown', this);
     this.node.addEventListener('mousemove', this);
-    this.node.addEventListener('mouseleave', this);
+    this.node.addEventListener('focusout', this);
     this.node.addEventListener('contextmenu', this);
   }
 
@@ -398,7 +390,7 @@ export class MenuBar extends Widget {
     this.node.removeEventListener('keydown', this);
     this.node.removeEventListener('mousedown', this);
     this.node.removeEventListener('mousemove', this);
-    this.node.removeEventListener('mouseleave', this);
+    this.node.removeEventListener('focusout', this);
     this.node.removeEventListener('contextmenu', this);
     this._closeChildMenu();
   }
@@ -408,7 +400,7 @@ export class MenuBar extends Widget {
    */
   protected onActivateRequest(msg: Message): void {
     if (this.isAttached) {
-      this.activeIndex = 0;
+      this._focusItemAt(0);
     }
   }
 
@@ -443,9 +435,11 @@ export class MenuBar extends Widget {
     for (let i = 0; i < length; ++i) {
       content[i] = renderer.renderItem({
         title: menus[i].title,
-        active: i === activeIndex && menus[i].items.length !== 0,
+        active: i === activeIndex,
         tabbable: i === tabFocusIndex,
+        disabled: menus[i].items.length === 0,
         onfocus: () => {
+          this._tabFocusIndex = i;
           this.activeIndex = i;
         }
       });
@@ -482,7 +476,9 @@ export class MenuBar extends Widget {
           title: this._overflowMenu.title,
           active: length === activeIndex && menus[length].items.length !== 0,
           tabbable: length === tabFocusIndex,
+          disabled: menus[length].items.length === 0,
           onfocus: () => {
+            this._tabFocusIndex = length;
             this.activeIndex = length;
           }
         });
@@ -502,7 +498,9 @@ export class MenuBar extends Widget {
               title: menu.title,
               active: false,
               tabbable: length === tabFocusIndex,
+              disabled: menus[length].items.length === 0,
               onfocus: () => {
+                this._tabFocusIndex = length;
                 this.activeIndex = length;
               }
             });
@@ -582,31 +580,38 @@ export class MenuBar extends Widget {
 
     // Enter, Space, Up Arrow, Down Arrow
     if (kc === 13 || kc === 32 || kc === 38 || kc === 40) {
+      // The active index may have changed (for example, user hovers over an
+      // item with the mouse), so be sure to use the focus index.
+      this.activeIndex = this._tabFocusIndex;
+      if (this.activeIndex !== this._tabFocusIndex) {
+        // Bail if the setter refused to set activeIndex to tabFocusIndex
+        // because it means that the item at tabFocusIndex cannot be opened (for
+        // example, it has an empty menu)
+        return;
+      }
       this.openActiveMenu();
       return;
     }
 
     // Escape
     if (kc === 27) {
       this._closeChildMenu();
-      this.activeIndex = -1;
-      this.node.blur();
+      this._focusItemAt(this.activeIndex);
       return;
     }
 
-    // Left Arrow
-    if (kc === 37) {
-      let i = this._activeIndex;
+    // Left or Right Arrow
+    if (kc === 37 || kc === 39) {
+      let direction = kc === 37 ? -1 : 1;
+      let start = this._tabFocusIndex + direction;
       let n = this._menus.length;
-      this.activeIndex = i === 0 ? n - 1 : i - 1;
-      return;
-    }
-
-    // Right Arrow
-    if (kc === 39) {
-      let i = this._activeIndex;
-      let n = this._menus.length;
-      this.activeIndex = i === n - 1 ? 0 : i + 1;
+      for (let i = 0; i < n; i++) {
+        let index = (n + start + direction * i) % n;
+        if (this._menus[index].items.length) {
+          this._focusItemAt(index);
+          return;
+        }
+      }
       return;
     }
 
@@ -631,8 +636,10 @@ export class MenuBar extends Widget {
       this.openActiveMenu();
     } else if (result.index !== -1) {
       this.activeIndex = result.index;
+      this._focusItemAt(this.activeIndex);
     } else if (result.auto !== -1) {
       this.activeIndex = result.auto;
+      this._focusItemAt(this.activeIndex);
     }
   }
 
@@ -648,7 +655,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();
     event.stopPropagation();
     event.stopImmediatePropagation();
 
@@ -673,6 +679,9 @@ export class MenuBar extends Widget {
       this._closeChildMenu();
       this.activeIndex = index;
     } else {
+      // If we don't call preventDefault() here, then the item in the menu
+      // bar will take focus over the menu that is being opened.
+      event.preventDefault();
       const position = this._positionForMenu(index);
       Menu.saveWindowData();
       // Begin DOM modifications.
@@ -737,15 +746,28 @@ export class MenuBar extends Widget {
   }
 
   /**
-   * Handle the `'mouseleave'` event for the menu bar.
+   * Handle the `'focusout'` event for the menu bar.
    */
-  private _evtMouseLeave(event: MouseEvent): void {
-    // Reset the active index if there is no open menu.
-    if (!this._childMenu) {
+  private _evtFocusOut(event: FocusEvent): void {
+    // Reset the active index if there is no open menu and the menubar is losing focus.
+    if (!this._childMenu && !this.node.contains(event.relatedTarget as Node)) {
       this.activeIndex = -1;
     }
   }
 
+  /**
+   * Focus an item in the menu bar.
+   *
+   * #### Notes
+   * Does not open the associated menu.
+   */
+  private _focusItemAt(index: number): void {
+    const itemNode = this.contentNode.childNodes[index] as HTMLElement | void;
+    if (itemNode) {
+      itemNode.focus();
+    }
+  }
+
   /**
    * Open the child menu at the active index immediately.
    *
@@ -776,7 +798,8 @@ export class MenuBar extends Widget {
       document.addEventListener('mousedown', this, true);
     }
 
-    // Ensure the menu bar is updated and look up the item node.
+    // Update the tab focus index and ensure the menu bar is updated.
+    this._tabFocusIndex = this.activeIndex;
     MessageLoop.sendMessage(this, Widget.Msg.UpdateRequest);
 
     // Get the positioning data for the new menu.
@@ -880,9 +903,12 @@ export class MenuBar extends Widget {
     this.update();
   }
 
-  // Track the index of the item that is currently focused. -1 means nothing focused.
+  // Track the index of the item that is currently focused or hovered. -1 means nothing focused or hovered.
   private _activeIndex = -1;
-  // Track which item can be focused using the TAB key. Unlike _activeIndex will always point to a menuitem.
+  // Track which item can be focused using the TAB key. Unlike _activeIndex will
+  // always point to a menuitem. Whenever you update this value, it's important
+  // to follow it with an "update-request" message so that the `tabindex`
+  // attribute on each menubar item gets properly updated.
   private _tabFocusIndex = 0;
   private _forceItemsPosition: Menu.IOpenOptions;
   private _overflowMenuOptions: IOverflowMenuOptions;
@@ -947,6 +973,15 @@ export namespace MenuBar {
      */
     readonly tabbable: boolean;
 
+    /**
+     * Whether the item is disabled.
+     *
+     * #### Notes
+     * A disabled item cannot be active.
+     * A disabled item cannot be focussed.
+     */
+    readonly disabled?: boolean;
+
     readonly onfocus?: (event: FocusEvent) => void;
   }
 
@@ -986,7 +1021,7 @@ export namespace MenuBar {
         {
           className,
           dataset,
-          tabindex: data.tabbable ? '0' : '-1',
+          ...(data.disabled ? {} : { tabindex: data.tabbable ? '0' : '-1' }),
           onfocus: data.onfocus,
           ...aria
         },
@@ -1033,7 +1068,7 @@ export namespace MenuBar {
       if (data.title.className) {
         name += ` ${data.title.className}`;
       }
-      if (data.active) {
+      if (data.active && !data.disabled) {
         name += ' lm-mod-active';
       }
       return name;
@@ -1058,7 +1093,11 @@ export namespace MenuBar {
      * @returns The aria attributes object for the item.
      */
     createItemARIA(data: IRenderData): ElementARIAAttrs {
-      return { role: 'menuitem', 'aria-haspopup': 'true' };
+      return {
+        role: 'menuitem',
+        'aria-haspopup': 'true',
+        'aria-disabled': data.disabled ? 'true' : 'false'
+      };
     }
 
     /**