feat(cdk-experimental/menu): add the ability to open/close menus on mouse click and hover #20118

andy9775 · 2020-07-28T17:15:47Z

Add the ability to open/close a menu when a user clicks a menu trigger and when a user hovers over
menu items. Additionally, keep track of hovered menu items and sync them with the FocusKeyManager
allowing a user to continue with a keyboard where they left off with their mouse.

teflonwaffles · 2020-07-28T20:00:08Z

src/cdk-experimental/menu/focus-mouse-manager.spec.ts

+    const spy = jasmine.createSpy('mouse enter spy');
+    focusManager.mouseEntered.subscribe(spy);
+
+    expect(fixture.componentInstance.showThird).toBeFalse();


I'm not sure this adds anything, since it's clear in the test host setup that this starts out false. (Also, it doesn't look like it's being updated in response to anything under test.)

It's more of a sanity check before the test, but I see how that's not clear. I'll make a change to make that more obvious.

The functionality that's being tested here is that when a new element is added, the focus-manager updates and picks it up.

teflonwaffles · 2020-07-28T20:15:38Z

src/cdk-experimental/menu/focus-mouse-manager.ts

+
+  constructor(private readonly _elements: QueryList<T>) {}
+
+  /**


Currently, a new observable is generated every time the method is called. Is this strictly necessary, or can the same purpose be served by using a single observable instance?

If a new observable is required every time, you should explain why in the documentation. (I would also rename it to '_getNewMouseEnterObservable()' to make this clear.)

Good point. It was part of the construction of the observable so it was only called once. However with Jeremys comment below it'll just end up as a function so it should be addressed

teflonwaffles · 2020-07-28T20:23:39Z

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

@@ -81,6 +83,11 @@ export class CdkMenuItem implements FocusableOption {
    this._elementRef.nativeElement.focus();
  }

+  // In Ivy the `host` metadata will be merged, whereas in ViewEngine it is overridden. In order
+  // to avoid double event listeners, we need to use `HostListener`. Once Ivy is the default, we
+  // can move this back into `host`.


Include a formal 'TODO' to do this?

This is the standard comment we use throughout the library; we have a backlog item to clean them all up when we can get to deleting ViewEngine

jelbourn · 2020-07-28T22:38:49Z

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

+    this._mouseManager = new FocusMouseManager(this._allItems);
+    this._mouseManager.mouseEntered.pipe(takeUntil(this._destroyed)).subscribe(item => {
+      if (this._hasOpenSubmenu()) {
+        this._keyManager.setActiveItem(item);
+      }
+    });


We can run this block in runOutsideAngular to avoid extra change detections

jelbourn · 2020-07-28T23:31:32Z

src/cdk-experimental/menu/focus-mouse-manager.ts

+ * FocusMouseManager keeps track of the element under mouse focus. It emits the element on an
+ * observable when an element is moused into.


After our conversation, I realized this could just be a function rather than a class since it doesn't have any state and just has a single method

/** * Gets a stream of pointer (mouse) entries into the given items. * This should typically run outside the Angular zone. */ function getItemPointerEntries<T extends FocusableElement>(items: QueryList<T>): Observable<T> { return this._elements.changes.pipe( startWith(this._elements), mergeMap((list: QueryList<T>) => list.map(element => fromEvent(element._elementRef.nativeElement, 'mouseenter').pipe( mapTo(element), takeUntil(this._elements.changes) ) ) ), mergeAll() ); }

jelbourn · 2020-07-28T23:36:24Z

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

@@ -81,6 +83,11 @@ export class CdkMenuItem implements FocusableOption {
    this._elementRef.nativeElement.focus();
  }

+  // In Ivy the `host` metadata will be merged, whereas in ViewEngine it is overridden. In order
+  // to avoid double event listeners, we need to use `HostListener`. Once Ivy is the default, we
+  // can move this back into `host`.


This is the standard comment we use throughout the library; we have a backlog item to clean them all up when we can get to deleting ViewEngine

jelbourn · 2020-07-28T23:39:24Z

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

+
+    /** Mock mouse events required to open the file menu. */
+    function openFileMenu() {
+      dispatchEvent(menuBarNativeItems[0], createMouseEvent('mouseenter'));


Rather than doing dispatchEvent with createMouseEvent, there's a convenient dispatchMouseEvent helper too

andy9775 · 2020-07-29T12:43:10Z

@jelbourn @teflonwaffles Feedback should be addressed PTAL

jelbourn

LGTM

jelbourn · 2020-07-29T18:11:12Z

src/cdk-experimental/menu/focus-mouse-manager.ts

@@ -0,0 +1,40 @@
+/**


I think I would change the name of this file to something like item-pointer-entries.ts

jelbourn · 2020-07-29T18:13:39Z

You can add the "merge-ready" label when ready

…ouse click and hover Add the ability to open/close a menu when a user clicks a menu trigger and when a user hovers over menu items. Additionally, keep track of hovered menu items and sync them with the FocusKeyManager allowing a user to continue with a keyboard where they left off with their mouse.

angular-automatic-lock-bot · 2020-08-29T00:11:46Z

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 28, 2020 17:15

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

andy9775 requested a review from teflonwaffles July 28, 2020 17:15

teflonwaffles reviewed Jul 28, 2020

View reviewed changes

jelbourn reviewed Jul 28, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-mouse-handling branch from aec83c0 to 600bf0e Compare July 29, 2020 12:29

andy9775 requested review from jelbourn and teflonwaffles July 29, 2020 12:43

jelbourn added target: minor This PR is targeted for the next minor release merge safe labels Jul 29, 2020

jelbourn approved these changes Jul 29, 2020

View reviewed changes

jelbourn added the lgtm label Jul 29, 2020

andy9775 force-pushed the cdk-menu-mouse-handling branch from 600bf0e to 5e70e3b Compare July 29, 2020 18:59

andy9775 added the action: merge The PR is ready for merge by the caretaker label Jul 29, 2020

jelbourn merged commit cdbf2c1 into angular:master Jul 29, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdk-experimental/menu): add the ability to open/close menus on mouse click and hover #20118

feat(cdk-experimental/menu): add the ability to open/close menus on mouse click and hover #20118

andy9775 commented Jul 28, 2020

teflonwaffles Jul 28, 2020

andy9775 Jul 29, 2020

teflonwaffles Jul 28, 2020

andy9775 Jul 29, 2020

teflonwaffles Jul 28, 2020

jelbourn Jul 28, 2020

jelbourn Jul 28, 2020

jelbourn Jul 28, 2020

jelbourn Jul 28, 2020

jelbourn Jul 28, 2020

andy9775 commented Jul 29, 2020

jelbourn left a comment

jelbourn Jul 29, 2020

andy9775 Jul 29, 2020

jelbourn commented Jul 29, 2020

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

		* FocusMouseManager keeps track of the element under mouse focus. It emits the element on an
		* observable when an element is moused into.

feat(cdk-experimental/menu): add the ability to open/close menus on mouse click and hover #20118

feat(cdk-experimental/menu): add the ability to open/close menus on mouse click and hover #20118

Conversation

andy9775 commented Jul 28, 2020

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

andy9775 commented Jul 29, 2020

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn commented Jul 29, 2020

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