feat(cdk-experimental/menu): add functionality to navigate a Menu and MenuBar with a keyboard #19962

andy9775 · 2020-07-13T18:30:05Z

The aria spec for Menu and MenuBar describes a robust set of instructions for navigating the Menu
using a keyboard. This implements the full aria spec for vertical and horizontal Menus along with
support for left-to-right and right-to-left layouts. Some liberty is taken with handling keyboard
events with modifier keys (home/end and escape) in order to match the existing mat-menu
implementation for those keys.

jelbourn

First pass of comments, got to go walk the dog again- will take another look tomorrow

jelbourn · 2020-07-14T00:17:29Z

src/cdk-experimental/menu/menu-stack.ts

+  close(): Observable<MenuStackItem> {
+    return this._close.asObservable();
+  }


Suggested change

close(): Observable<MenuStackItem> {

return this._close.asObservable();

}

readonly close: Observable<MenuStackItem> = this._close.asObservable();

Could close and empty be properties? It looks like they never change

jelbourn · 2020-07-14T00:45:52Z

src/cdk-experimental/menu/menu-panel.ts

@@ -18,6 +19,9 @@ export class CdkMenuPanel {
  /** Reference to the child menu component */
  _menu?: Menu;

+  /** Keep track of open Menus. */
+  _menuStack: MenuStack = new MenuStack();


I got confused at first because this looked like this was always creating a new stack; it wasn't clear that the trigger would overwrite this stack upon the panel being bound to the trigger.

Is it possible to not instantiate a new stack here and instead have a comment that explains that the stack would be set by the trigger upon opening?

jelbourn · 2020-07-14T00:55:10Z

src/cdk-experimental/menu/menu-item.ts

+
+  /** Get the MenuStack from the parent menu. */
+  private _getMenuStack() {
+    return this._parentMenu._menuStack;


I would add a comment here explaining that this needs to be a function (rather than a property set during construction) because the menu doesn't have a reference to its stack at creation time.

jelbourn · 2020-07-14T00:58:05Z

src/cdk-experimental/menu/menu-panel.ts

@@ -26,5 +30,8 @@ export class CdkMenuPanel {
   */
  _registerMenu(child: Menu) {
    this._menu = child;
+
+    this._menu._menuStack = this._menuStack;


I would add a comment here that explains that, while we'd ideally like to inject the stack into the menu, this isn't currently possible because the menu's injector can't be affected by the code stamping out the panel template.

jelbourn · 2020-07-14T00:59:56Z

src/cdk-experimental/menu/menu-stack.ts

+import {Subject, Observable} from 'rxjs';
+
+/** Events to emit as specified by the caller once the MenuStack is empty. */
+export type FocusNext = 'nextItem' | 'previousItem' | 'currentItem' | undefined;


Could this be a const enum? We typically only use string literal types like this when the value would be coming from a template binding or other user-based input.

jelbourn · 2020-07-14T01:01:25Z

src/cdk-experimental/menu/menu-bar.ts

+      case HOME:
+      case END:
+        if (!hasModifierKey(event)) {
+          event.preventDefault();
+          event.keyCode === HOME ? this.focusFirstItem('keyboard') : this.focusLastItem('keyboard');
+        }
+        break;
+


FYI we just support for home and end in ListKeyManager this past week

jelbourn · 2020-07-14T01:12:08Z

@crisbeto I'd love to get your input on this approach; I've had my head in the problem for too long

crisbeto · 2020-07-14T15:17:59Z

src/cdk-experimental/menu/menu-bar.ts

@@ -20,18 +35,169 @@ import {CDK_MENU, Menu} from './menu-interface';
  selector: '[cdkMenuBar]',
  exportAs: 'cdkMenuBar',
  host: {
+    '(keydown)': '_handleKeyEvent($event)',
+    '(focus)': 'focusFirstItem()',


If we're redirecting focus anyway, couldn't we remove the tabindex from this this and let the user tab into the first item directly?

yeah, I didn't want to pollute this PR with too much tab focus logic. I'll have a follow on which sets the tabindex of the first menu item in the menu bar to 0 and handles changes as well.

Considering this further, the current implementation will set focus on the first non-disabled item and keeps focus on the MenuBar if all elements are disabled which is the expected behaviour. Any changes would essentially do the same. The only difference being that the tab index of the first menu item would be 0 rather than -1 as is here. Thoughts?

crisbeto · 2020-07-14T15:20:16Z

src/cdk-experimental/menu/menu-bar.ts

+        ) {
+          event.preventDefault();
+
+          const prevIsOpen = this._keyManager.activeItem?.isMenuOpen();


It looks like you're repeating this._keyManager.activeItem in almost all of the switch cases. Maybe move it out into a variable at the top?

crisbeto · 2020-07-14T15:20:58Z

src/cdk-experimental/menu/menu-bar.ts

+  /** Subscribe to the MenuStack close and empty observables. */
+  private _subscribeToMenuStack() {
+    this._menuStack.close().subscribe((item: MenuStackItem) => this._closeOpenMenu(item));
+    this._menuStack.empty().subscribe((event: FocusNext) => this._toggleOpenMenu(event));


It doesn't look like these are unsubscribing.

crisbeto · 2020-07-14T15:22:17Z

src/cdk-experimental/menu/menu-bar.ts

+    if (item === this._keyManager.activeItem?.getMenu()) {
+      this._keyManager.activeItem.getMenuTrigger()?.closeMenu();
+      this._keyManager.setFocusOrigin('keyboard');
+      this._keyManager.setActiveItem(this._keyManager.activeItem);


Repeating this._keyManager won't minify with anything aside from Closure compiler. If you have it multiple times within the same function, it's better to save it in a variable.

crisbeto · 2020-07-14T15:26:09Z

src/cdk-experimental/menu/menu-item-trigger.ts

+    if (!this.isMenuOpen()) {
+      this.opened.next();
+
+      this._overlayRef = this._overlay.create(this._getOverlayConfig());


This will recreate the overlay every time you open it. You can reuse it like this:

if (!this._overlayRef) { this._overlayRef = this._overlay.create(this._getOverlayConfig()); } this._overlayRef.attach(this._getPortal());

crisbeto · 2020-07-14T15:30:30Z

src/cdk-experimental/menu/menu-item-trigger.ts

+      case RIGHT_ARROW:
+        if (this._isParentVertical()) {
+          event.preventDefault();
+          if (this._directionality.value === 'ltr') {


The _directionality should be decorated with @Optional, because we aren't guaranteed for it to exist. Also generally we format direction checks like this, because LTR is the most common case and it handles undefined values better:

if (direction === 'rtl') { // do RTL stuff } else { // do LTR stuff }

crisbeto · 2020-07-14T15:31:57Z

src/cdk-experimental/menu/menu-item.ts

+  /** Get the label for this element which is required by the FocusableOption interface. */
+  getLabel(): string {
+    // TODO(andy): implement a more robust algorithm for determining nested text
+    return this._elementRef.nativeElement.innerText;


innerText will trigger a reflow, because it has some logic where it excludes hidden text. We should use textContent here.

crisbeto · 2020-07-14T15:51:14Z

src/cdk-experimental/menu/menu-stack.ts

+ */
+export class MenuStack {
+  /** All MenuStackItems tracked by this MenuStack. */
+  private readonly _elements: MenuStackItem[] = [];


I think that using a stack/array that pretends to be a stack here will run into the same issue as when we tried to keep track of menu items ourselves in mat-menu. If a menu pushes itself onto the stack, gets destroyed by an ngIf and then gets re-added, the stack will be out of order. E.g. if we have [1, 2, 3], we remove 2 and then we re-add it, the stack will have [1, 3, 2], whereas the DOM will still be [1, 2, 3].

That being said, I'm having a hard time visualizing when the registration happens. If it's only while a stack of menus is open, we'll probably be fine.

Hmm. Not sure if I follow. If we remove 2 that means we closed 2 and closing 2 should close all submenus. Hence [1,2,3] would result in [1] after removing 2.

Yeah, I think that if you ensure any menu closing also closes all of its sub-menus, this should be okay. (Unless there's something a user might do that I'm not imagining)

Was already merged in before I had a chance to address this. I'll address with a follow on pr.

crisbeto · 2020-07-14T15:52:27Z

src/cdk-experimental/menu/menu-stack.ts

+  closeAll(focusNext?: FocusNext) {
+    if (this._elements.length) {
+      while (this._elements.length) {
+        const menuStackItem = this._elements.pop();


It looks like you're repeating a bit of the logic from closeLatest. Could it be reduced to this?

while (this._elements.length) { this.closeLatest(); }

Long story short, no. It has to do with the Subject which results in loop

crisbeto · 2020-07-14T15:52:43Z

src/cdk-experimental/menu/menu-stack.ts

+   * not emit if the stack was initially empty.
+   */
+  closeAll(focusNext?: FocusNext) {
+    if (this._elements.length) {


This check isn't necessary since the loop won't start if the array is empty.

yeah, but I don't want to emit if the stack was initially empty. Of course I could do something like

const shouldEmit = this._elements.length > 0; while(...) {...} if (shouldEmit) {this._empty.next(focusNext)}

But the existing approach seemed cleaner IMO, but let me know what you think

teflonwaffles · 2020-07-14T15:52:53Z

src/cdk-experimental/menu/menu-bar.spec.ts

+          .map(e => e.nativeElement)
+          .slice(0, 2); // menu bar has the first 2 menu items
+
+        if (nativeMenus.length > 0) {


It would be best to avoid any conditional logic when setting up a unit test suite, if possible.

teflonwaffles · 2020-07-14T15:58:08Z

src/cdk-experimental/menu/menu-bar.spec.ts

+        function openShareMenu() {
+          openFileMenu();
+
+          if (nativeMenus.length === 1) {


If openShareMenu() will be called with both nativeMenus.length === 1 and nativeMenus.length !== 1, could you create separate versions of the function and call the relevant function in each scenario (instead of relying on an if statement)>

teflonwaffles · 2020-07-14T16:25:53Z

src/cdk-experimental/menu/menu-item.spec.ts

-    button = fixture.debugElement.query(By.directive(CdkMenuItem)).injector.get(CdkMenuItem);
-    nativeButton = fixture.debugElement.query(By.directive(CdkMenuItem)).nativeElement;
-  });
+  describe('without enclosing elements', () => {


I thought this meant, without elements around the menu item. Could you reword this to be clearer (eg. 'with no complex inner elements', to match the other suite).

teflonwaffles · 2020-07-14T16:32:00Z

src/cdk-experimental/menu/menu-stack.ts

+  private readonly _empty: Subject<FocusNext> = new Subject();
+
+  /** Observable which emits the MenuStackItem which has been requested to close. */
+  close(): Observable<MenuStackItem> {


We have an internal guideline to use typing to restrict the type when exposing a subject as an observable, instead of explicitly calling asObservable(). @jelbourn , is this also true in the CDK codebase, or do you prefer to call asObservable()?

Yeah, we could do that here; that is probably better

Was already merged in before I had a chance to address, will include fix in a follow on pr.

… MenuBar with a keyboard The aria spec for Menu and MenuBar describes a robust set of instructions for navigating the Menu using a keyboard. This implements the full aria spec for vertical and horizontal Menus along with support for left-to-right and right-to-left layouts. Some liberty is taken with handling keyboard events with modifier keys (home/end and escape) in order to match the existing mat-menu implementation for those keys.

andy9775 · 2020-07-14T18:18:03Z

@teflonwaffles @jelbourn @crisbeto Feedback should be addressed.

crisbeto

LGTM

jelbourn

LGTM

… MenuBar with a keyboard (angular#19962) The aria spec for Menu and MenuBar describes a robust set of instructions for navigating the Menu using a keyboard. This implements the full aria spec for vertical and horizontal Menus along with support for left-to-right and right-to-left layouts. Some liberty is taken with handling keyboard events with modifier keys (home/end and escape) in order to match the existing mat-menu implementation for those keys.

angular-automatic-lock-bot · 2020-08-15T00:07:53Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

andy9775 requested a review from jelbourn as a code owner July 13, 2020 18:30

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 13, 2020

andy9775 requested a review from teflonwaffles July 13, 2020 18:30

andy9775 force-pushed the cdk-menu-key-handling branch from dc1d361 to c359611 Compare July 13, 2020 18:41

jelbourn reviewed Jul 14, 2020

View reviewed changes

jelbourn requested a review from crisbeto July 14, 2020 01:11

crisbeto reviewed Jul 14, 2020

View reviewed changes

teflonwaffles reviewed Jul 14, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-key-handling branch from c359611 to 134f627 Compare July 14, 2020 18:17

crisbeto approved these changes Jul 14, 2020

View reviewed changes

jelbourn approved these changes Jul 14, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jul 14, 2020

andrewseguin merged commit ffc6f4b into angular:master Jul 14, 2020

andy9775 deleted the cdk-menu-key-handling branch July 14, 2020 23:16

andy9775 mentioned this pull request Jul 14, 2020

refactor(cdk-experimental/menu) use typing to expose a subject as observable and skip method call #19978

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 15, 2020

feat(cdk-experimental/menu): add functionality to navigate a Menu and MenuBar with a keyboard #19962

feat(cdk-experimental/menu): add functionality to navigate a Menu and MenuBar with a keyboard #19962

Uh oh!

Conversation

andy9775 commented Jul 13, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy9775 Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy9775 commented Jul 14, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Aug 15, 2020

andy9775 Jul 14, 2020 •

edited

Loading