fix(menu): unable to open same sub-menu from different triggers and not picking up indirect descendant items #10132

crisbeto · 2018-02-24T15:48:27Z

Reworks the relationship between the menu items and the menu panel to allow for the items to be indirect descendants of the menu, while also allowing for mat-menu instances to be declared inside of other mat-menu instances without having the root mat-menu pick up all of the descendant items.
Adds the ability to pass in an array to the ListKeyManager, in addition to a QueryList.
Fixes not being able to re-use the same sub-menu between multiple sub-menu triggers.

Fixes #9969.
Fixes #9987.

jelbourn · 2018-02-26T19:32:34Z

src/cdk/a11y/key-manager/list-key-manager.ts

-        if (newIndex > -1 && newIndex !== this._activeItemIndex) {
-          this._activeItemIndex = newIndex;
+  constructor(private _items: QueryList<T> | T[]) {
+    if (_items instanceof QueryList) {


Add comment that explains why items can be either a QueryList or an array?

jelbourn · 2018-02-26T19:42:34Z

src/lib/menu/menu-directive.ts

+   * Registers a menu item with the menu.
+   * @docs-private
+   */
+  addItem(item: MatMenuItem) {


Does needing to call these methods constitute a breaking change for people implementing their own menu panel?

I don't think so. We never required the items (the QueryList we used before to collected the items) as a part of the MatMenuPanel and we didn't support custom menu panels for nested menus either.

jelbourn · 2018-02-26T19:43:17Z

src/lib/menu/menu-directive.ts

@@ -294,6 +300,30 @@ export class MatMenu implements OnInit, AfterContentInit, MatMenuPanel, OnDestro
    }
  }

+  /**
+   * Registers a menu item with the menu.


Expand to talk about why this registration is necessary?

jelbourn · 2018-02-26T20:05:30Z

src/lib/menu/menu-trigger.ts

@@ -140,6 +143,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
              @Optional() @Self() private _menuItemInstance: MatMenuItem,
              @Optional() private _dir: Directionality,
              // TODO(crisbeto): make the _focusMonitor required when doing breaking changes.
+              // @deletion-target 6.0.0


Is deletion target the right tag?

That's what we've been using in other places to mark that a parameter should become required. deletion-target might not be the best naming for it though.

jelbourn · 2018-02-26T20:06:39Z

src/lib/menu/menu-trigger.ts

+      // Since we might have multiple competing triggers for the same menu (e.g. a sub-menu
+      // with different data and triggers), we have to delay it by a tick to ensure that
+      // it won't be closed immediately after it is opened.
+      .pipe(filter(active => active === this._menuItemInstance), delay(0))


Will this delay trigger multiple change detections if you quickly hover over multiple menu items?

We're triggering change detections anyway, because we have a mouseenter listener on each menu item.

crisbeto

Addressed the feedback @jelbourn.

crisbeto · 2018-02-26T20:34:40Z

src/lib/menu/menu-directive.ts

+   * Registers a menu item with the menu.
+   * @docs-private
+   */
+  addItem(item: MatMenuItem) {


I don't think so. We never required the items (the QueryList we used before to collected the items) as a part of the MatMenuPanel and we didn't support custom menu panels for nested menus either.

crisbeto · 2018-02-26T20:36:26Z

src/lib/menu/menu-trigger.ts

@@ -140,6 +143,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
              @Optional() @Self() private _menuItemInstance: MatMenuItem,
              @Optional() private _dir: Directionality,
              // TODO(crisbeto): make the _focusMonitor required when doing breaking changes.
+              // @deletion-target 6.0.0


That's what we've been using in other places to mark that a parameter should become required. deletion-target might not be the best naming for it though.

crisbeto · 2018-02-26T20:37:27Z

src/lib/menu/menu-trigger.ts

+      // Since we might have multiple competing triggers for the same menu (e.g. a sub-menu
+      // with different data and triggers), we have to delay it by a tick to ensure that
+      // it won't be closed immediately after it is opened.
+      .pipe(filter(active => active === this._menuItemInstance), delay(0))


We're triggering change detections anyway, because we have a mouseenter listener on each menu item.

jelbourn

LGTM

jelbourn · 2018-02-27T21:40:08Z

Just needs rebase

josephperrott · 2018-05-01T21:41:25Z

@crisbeto Looks like this has a couple lint errors.

…ot picking up indirect descendant items * Reworks the relationship between the menu items and the menu panel to allow for the items to be indirect descendants of the menu, while also allowing for `mat-menu` instances to be declared inside of other `mat-menu` instances without having the root `mat-menu` pick up all of the descendant items. * Adds the ability to pass in an array to the `ListKeyManager`, in addition to a `QueryList`. * Fixes not being able to re-use the same sub-menu between multiple sub-menu triggers. Fixes angular#9969. Fixes angular#9987.

crisbeto · 2018-05-01T21:53:20Z

Fixed @josephperrott.

angular-automatic-lock-bot · 2019-09-08T21:02:19Z

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.}

crisbeto added the pr: needs review label Feb 24, 2018

crisbeto assigned jelbourn Feb 24, 2018

crisbeto requested review from devversion, jelbourn and kara as code owners February 24, 2018 15:48

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 24, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from a18d631 to a7a2ae6 Compare February 25, 2018 09:58

jelbourn reviewed Feb 26, 2018

View reviewed changes

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from a7a2ae6 to 8643ddd Compare February 26, 2018 20:38

crisbeto commented Feb 26, 2018

View reviewed changes

jelbourn approved these changes Feb 27, 2018

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Feb 27, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch 2 times, most recently from b0b7139 to d798784 Compare March 3, 2018 17:02

crisbeto added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs rebase labels Mar 3, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from d798784 to c2a6a5d Compare March 11, 2018 02:05

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from c2a6a5d to dd5c23c Compare March 26, 2018 18:05

josephperrott added pr: lgtm and removed pr: lgtm labels Mar 26, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch 2 times, most recently from 3f40dbc to 087ac4d Compare March 29, 2018 14:23

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from 087ac4d to 0df86e8 Compare April 8, 2018 12:50

crisbeto mentioned this pull request Apr 12, 2018

Template issue when creating dynamic 3-level nested menu #10815

Closed

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from 0df86e8 to 9b32c13 Compare April 14, 2018 13:11

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from 9b32c13 to 2c33108 Compare April 24, 2018 23:08

andrewseguin added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 26, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch 2 times, most recently from 642bd06 to e71f2de Compare May 1, 2018 19:14

josephperrott added pr: needs cleanup / tests and removed action: merge The PR is ready for merge by the caretaker presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels May 1, 2018

crisbeto force-pushed the 9987/nested-menu-multi-trigger branch from e71f2de to 9657ebe Compare May 1, 2018 21:52

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs cleanup / tests labels May 1, 2018

josephperrott merged commit 5393bfe into angular:master May 2, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

fix(menu): unable to open same sub-menu from different triggers and not picking up indirect descendant items #10132

fix(menu): unable to open same sub-menu from different triggers and not picking up indirect descendant items #10132

Uh oh!

Conversation

crisbeto commented Feb 24, 2018

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!

crisbeto 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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Feb 27, 2018

Uh oh!

josephperrott commented May 1, 2018

Uh oh!

crisbeto commented May 1, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!