fix(cdk-experimental/menu): fix bug preventing keyboard event handling if opened programmatically #20004

andy9775 · 2020-07-16T13:45:20Z

If a menu is opened programmatically, subsequent keyboard events to close it out are not handled.
This fixes the issue by tracking the open sub-menu regardless of how it was opened up.

crisbeto · 2020-07-16T14:32:46Z

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

+          detectChanges();
+
+          fileMenuNativeItems[0].focus();
+          dispatchKeyboardEvent(document.activeElement!, 'keydown', TAB);


This could result in test flakes since browsers won't move focus in some cases on the CI (primarily if the window is out of focus). It might be better if you send the event directly to fileMenuNativeItems[0].

crisbeto · 2020-07-16T14:33:48Z

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

+        item
+          .getMenuTrigger()!
+          .closed.pipe(take(1), takeUntil(this._destroyed))
+          .subscribe(() => (this._openItem = undefined));


Subscribing inside of a subscription is a bit of anti-pattern in rxjs since you can end up with multiple concurrent subscriptions. It might be better if you did this with a switchMap and only subscribed once at the end.

crisbeto · 2020-07-16T14:34:27Z

src/cdk-experimental/menu/menu.ts

+        item
+          .getMenuTrigger()!
+          .closed.pipe(take(1), takeUntil(this.closed))
+          .subscribe(() => (this._openItem = undefined));


Same note here about subscribing inside a subscription.

teflonwaffles · 2020-07-16T19:40:20Z

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

@@ -207,6 +212,33 @@ export class CdkMenuBar extends CdkMenuGroup implements Menu, AfterContentInit,
    return this.orientation === 'horizontal';
  }

+  /**
+   * Subscribe to the menu trigger's open events in order to track the trigger which opened the menu
+   * and stop tracking it when the menu is closed closed.


typo: double 'closed'

teflonwaffles · 2020-07-16T19:42:40Z

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

+            .map(item =>
+              item
+                .getMenuTrigger()!
+                .opened.pipe(mapTo(item), takeUntil(merge(this._allItems.changes, this._destroyed)))


Here too, is it possible to unnest some of these nested operators? I've tended to have odd behavior with this sort of thing. (Should there be a persistent object-level observable merging this._allItems.changes and this._destroyed?)

teflonwaffles · 2020-07-16T19:44:59Z

src/cdk-experimental/menu/menu.ts

+    this._allItems.changes
+      .pipe(
+        startWith(this._allItems),
+        mergeMap((list: QueryList<CdkMenuItem>) =>


Similar to above comment?

andy9775 · 2020-07-16T21:09:15Z

@crisbeto @teflonwaffles feedback should be addressed.

jelbourn

LGTM

jelbourn · 2020-07-20T23:11:55Z

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

+          fixture.debugElement
+            .queryAll(By.directive(CdkMenuItem))[0]
+            .injector.get(CdkMenuItem)
+            .getMenuTrigger()!
+            .openMenu();


nit: +4 indent for line overflow, so this would be e.g.

Suggested change

fixture.debugElement

.queryAll(By.directive(CdkMenuItem))[0]

.injector.get(CdkMenuItem)

.getMenuTrigger()!

.openMenu();

fixture.debugElement

.queryAll(By.directive(CdkMenuItem))[0]

.injector.get(CdkMenuItem)

.getMenuTrigger()!

.openMenu();

jelbourn · 2020-07-20T23:13:48Z

src/cdk-experimental/menu/menu.ts

+   * Subscribe to the menu trigger's open events in order to track the trigger which opened the menu
+   * and stop tracking it when the menu is closed.
+   */
+  private _subscribeToMenuOpen() {


Not strictly necessary for this PR, but it would be good to cut down on the duplication between menu and menubar here. Maybe just add a TODO for now

yeah, totally agree. Once everything is finalized I think pulling the duplicated logic out into an abstract Menu class is the way to go.

jelbourn · 2020-07-20T23:15:46Z

You can add the merge-ready label when ready

…g if opened programmatically If a menu is opened programmatically, subsequent keyboard events to close it out are not handled. This fixes the issue by tracking the open sub-menu regardless of how it was opened up.

…g if opened programmatically (#20004) If a menu is opened programmatically, subsequent keyboard events to close it out are not handled. This fixes the issue by tracking the open sub-menu regardless of how it was opened up. (cherry picked from commit 7303780)

angular-automatic-lock-bot · 2020-08-21T00:08:57Z

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 16, 2020 13:45

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

andy9775 requested a review from teflonwaffles July 16, 2020 13:45

crisbeto reviewed Jul 16, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-fix-programatic-open branch 2 times, most recently from e2b3c2c to 7e6f75c Compare July 16, 2020 16:33

teflonwaffles reviewed Jul 16, 2020

View reviewed changes

andy9775 force-pushed the cdk-menu-fix-programatic-open branch from 7e6f75c to adf11f3 Compare July 16, 2020 21:08

andy9775 force-pushed the cdk-menu-fix-programatic-open branch from adf11f3 to 39d06ee Compare July 16, 2020 21:31

jelbourn approved these changes Jul 20, 2020

View reviewed changes

jelbourn added target: patch This PR is targeted for the next patch release lgtm labels Jul 20, 2020

andy9775 force-pushed the cdk-menu-fix-programatic-open branch from 39d06ee to da11787 Compare July 21, 2020 00:11

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

wagnermaciel merged commit 7303780 into angular:master Jul 21, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cdk-experimental/menu): fix bug preventing keyboard event handling if opened programmatically #20004

fix(cdk-experimental/menu): fix bug preventing keyboard event handling if opened programmatically #20004

andy9775 commented Jul 16, 2020

crisbeto Jul 16, 2020

crisbeto Jul 16, 2020

crisbeto Jul 16, 2020

teflonwaffles Jul 16, 2020

teflonwaffles Jul 16, 2020

teflonwaffles Jul 16, 2020

andy9775 commented Jul 16, 2020

jelbourn left a comment

jelbourn Jul 20, 2020

jelbourn Jul 20, 2020

andy9775 Jul 21, 2020

jelbourn commented Jul 20, 2020

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

fix(cdk-experimental/menu): fix bug preventing keyboard event handling if opened programmatically #20004

fix(cdk-experimental/menu): fix bug preventing keyboard event handling if opened programmatically #20004

Conversation

andy9775 commented Jul 16, 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

andy9775 commented Jul 16, 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

Choose a reason for hiding this comment

jelbourn commented Jul 20, 2020

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