Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/cdk-experimental/menu/menu-bar.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,27 @@ describe('MenuBar', () => {

expect(document.activeElement).toEqual(fileMenuNativeItems[1]);
}));

it('should handle keyboard actions if initial menu is opened programmatically', () => {
fixture.debugElement
.queryAll(By.directive(CdkMenuItem))[0]
.injector.get(CdkMenuItem)
.getMenuTrigger()!
.openMenu();
detectChanges();
fixture.debugElement
.queryAll(By.directive(CdkMenuItem))[2]
.injector.get(CdkMenuItem)
.getMenuTrigger()!
.openMenu();
detectChanges();

fileMenuNativeItems[0].focus();
dispatchKeyboardEvent(fileMenuNativeItems[0], 'keydown', TAB);
detectChanges();

expect(nativeMenus.length).toBe(0);
});
});
});

Expand Down
41 changes: 35 additions & 6 deletions src/cdk-experimental/menu/menu-bar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ import {
import {Directionality} from '@angular/cdk/bidi';
import {FocusKeyManager, FocusOrigin} from '@angular/cdk/a11y';
import {LEFT_ARROW, RIGHT_ARROW, UP_ARROW, DOWN_ARROW, ESCAPE, TAB} from '@angular/cdk/keycodes';
import {takeUntil, mergeAll, mapTo, startWith, mergeMap, switchMap} from 'rxjs/operators';
import {Subject, merge} from 'rxjs';
import {CdkMenuGroup} from './menu-group';
import {CDK_MENU, Menu} from './menu-interface';
import {CdkMenuItem} from './menu-item';
import {MenuStack, MenuStackItem, FocusNext} from './menu-stack';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';

/**
* Directive applied to an element which configures it as a MenuBar by setting the appropriate
Expand Down Expand Up @@ -64,6 +64,9 @@ export class CdkMenuBar extends CdkMenuGroup implements Menu, AfterContentInit,
@ContentChildren(CdkMenuItem, {descendants: true})
private readonly _allItems: QueryList<CdkMenuItem>;

/** The Menu Item which triggered the open submenu. */
private _openItem?: CdkMenuItem;

constructor(readonly _menuStack: MenuStack, @Optional() private readonly _dir?: Directionality) {
super();
}
Expand All @@ -72,6 +75,7 @@ export class CdkMenuBar extends CdkMenuGroup implements Menu, AfterContentInit,
super.ngAfterContentInit();

this._setKeyManager();
this._subscribeToMenuOpen();
this._subscribeToMenuStack();
}

Expand Down Expand Up @@ -163,12 +167,13 @@ export class CdkMenuBar extends CdkMenuGroup implements Menu, AfterContentInit,
* Close the open menu if the current active item opened the requested MenuStackItem.
* @param item the MenuStackItem requested to be closed.
*/
private _closeOpenMenu(item: MenuStackItem) {
private _closeOpenMenu(menu: MenuStackItem) {
const trigger = this._openItem;
const keyManager = this._keyManager;
if (item === keyManager.activeItem?.getMenu()) {
keyManager.activeItem.getMenuTrigger()?.closeMenu();
if (menu === trigger?.getMenuTrigger()?.getMenu()) {
trigger.getMenuTrigger()?.closeMenu();
keyManager.setFocusOrigin('keyboard');
keyManager.setActiveItem(keyManager.activeItem);
keyManager.setActiveItem(trigger);
}
}

Expand Down Expand Up @@ -207,6 +212,30 @@ 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.
*/
private _subscribeToMenuOpen() {
const exitCondition = merge(this._allItems.changes, this._destroyed);
this._allItems.changes
.pipe(
startWith(this._allItems),
mergeMap((list: QueryList<CdkMenuItem>) =>
list
.filter(item => item.hasMenu())
.map(item => item.getMenuTrigger()!.opened.pipe(mapTo(item), takeUntil(exitCondition)))
),
mergeAll(),
switchMap((item: CdkMenuItem) => {
this._openItem = item;
return item.getMenuTrigger()!.closed;
}),
takeUntil(this._destroyed)
)
.subscribe(() => (this._openItem = undefined));
}

ngOnDestroy() {
super.ngOnDestroy();
this._destroyed.next();
Expand Down
41 changes: 36 additions & 5 deletions src/cdk-experimental/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ import {
hasModifierKey,
} from '@angular/cdk/keycodes';
import {Directionality} from '@angular/cdk/bidi';
import {take, takeUntil} from 'rxjs/operators';
import {take, takeUntil, startWith, mergeMap, mapTo, mergeAll, switchMap} from 'rxjs/operators';
import {merge} from 'rxjs';
import {CdkMenuGroup} from './menu-group';
import {CdkMenuPanel} from './menu-panel';
import {Menu, CDK_MENU} from './menu-interface';
Expand Down Expand Up @@ -81,6 +82,9 @@ export class CdkMenu extends CdkMenuGroup implements Menu, AfterContentInit, OnI
@ContentChildren(CdkMenuItem, {descendants: true})
private readonly _allItems: QueryList<CdkMenuItem>;

/** The Menu Item which triggered the open submenu. */
private _openItem?: CdkMenuItem;

/**
* A reference to the enclosing parent menu panel.
*
Expand All @@ -106,6 +110,7 @@ export class CdkMenu extends CdkMenuGroup implements Menu, AfterContentInit, OnI

this._completeChangeEmitter();
this._setKeyManager();
this._subscribeToMenuOpen();
this._subscribeToMenuStack();
}

Expand Down Expand Up @@ -227,12 +232,13 @@ export class CdkMenu extends CdkMenuGroup implements Menu, AfterContentInit, OnI
* Close the open menu if the current active item opened the requested MenuStackItem.
* @param item the MenuStackItem requested to be closed.
*/
private _closeOpenMenu(item: MenuStackItem) {
private _closeOpenMenu(menu: MenuStackItem) {
const keyManager = this._keyManager;
if (item === keyManager.activeItem?.getMenu()) {
keyManager.activeItem.getMenuTrigger()?.closeMenu();
const trigger = this._openItem;
if (menu === trigger?.getMenuTrigger()?.getMenu()) {
trigger.getMenuTrigger()?.closeMenu();
keyManager.setFocusOrigin('keyboard');
keyManager.setActiveItem(keyManager.activeItem);
keyManager.setActiveItem(trigger);
}
}

Expand All @@ -259,6 +265,31 @@ export class CdkMenu extends CdkMenuGroup implements Menu, AfterContentInit, OnI
}
}

// TODO(andy9775): remove duplicate logic between menu an menu bar
/**
* 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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const exitCondition = merge(this._allItems.changes, this.closed);
this._allItems.changes
.pipe(
startWith(this._allItems),
mergeMap((list: QueryList<CdkMenuItem>) =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above comment?

list
.filter(item => item.hasMenu())
.map(item => item.getMenuTrigger()!.opened.pipe(mapTo(item), takeUntil(exitCondition)))
),
mergeAll(),
switchMap((item: CdkMenuItem) => {
this._openItem = item;
return item.getMenuTrigger()!.closed;
}),
takeUntil(this.closed)
)
.subscribe(() => (this._openItem = undefined));
}

/** Return true if this menu has been configured in a horizontal orientation. */
private _isHorizontal() {
return this.orientation === 'horizontal';
Expand Down