fix(cdk-experimental/menu): do not allow two separate triggers to open the same menu #20300
Conversation
googlebot
added
the
cla: yes
| @@ -140,7 +148,7 @@ export class CdkMenuItemTrigger implements OnDestroy { | |||
| */ | |||
| _toggleOnMouseEnter() { | |||
| const menuStack = this._getMenuStack(); | |||
| if (!menuStack.isEmpty() && !this.isMenuOpen()) { | |||
| if (!menuStack?.isEmpty() && !this.isMenuOpen()) { | |||
There was a problem hiding this comment.
The first part of this expression is a little hard to parse. Is there a way to make it clearer?
| @@ -206,10 +214,10 @@ export class CdkMenuItemTrigger implements OnDestroy { | |||
| // that means that the parent menu is a menu bar since we don't put the menu bar on the | |||
| // stack | |||
| const isParentMenuBar = | |||
| !menuStack.closeSubMenuOf(this._parentMenu) && menuStack.peek() !== this._parentMenu; | |||
| !menuStack?.closeSubMenuOf(this._parentMenu) && menuStack?.peek() !== this._parentMenu; | |||
There was a problem hiding this comment.
Could you verify that your calls to methods on menuStack? will work in an expected way if menuStack is not defined?
There was a problem hiding this comment.
In the instance that there is no menustack there's nothing to do and the ? prevents the methods from being called.
| @@ -108,6 +110,10 @@ export class CdkContextMenuTrigger implements OnDestroy { | |||
| return this._menuPanel; | |||
| } | |||
| set menuPanel(panel: CdkMenuPanel) { | |||
| // If the provided panel already has a stack, that means it already has a trigger configured | |||
| if (panel._menuStack && isDevMode()) { | |||
There was a problem hiding this comment.
I don't think it makes a difference, but conventionally the isDevMode check goes first
| @@ -108,6 +110,10 @@ export class CdkContextMenuTrigger implements OnDestroy { | |||
| return this._menuPanel; | |||
| } | |||
| set menuPanel(panel: CdkMenuPanel) { | |||
| // If the provided panel already has a stack, that means it already has a trigger configured | |||
| if (panel._menuStack && isDevMode()) { | |||
There was a problem hiding this comment.
Note that the isDevMode calls will become lint errors with #20146. I suspect that your changes will land first though.
There was a problem hiding this comment.
ya, I saw that and wasn't sure how to address it. I think I'll issue a follow up PR after it lands and I'll add a TODO for now
| }); | ||
|
|
||
| it('should throw an error if a trigger in a submenu references its parent menu', () => { | ||
| expect(createComponent(TriggerOpensItsMenu)).toThrowError(); |
There was a problem hiding this comment.
Should we be testing for the specific error that we're expecting here? E.g. .toThrowError(/CdkMenuPanel is already referenced by different CdkMenuTrigger/).
| @@ -125,7 +125,7 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, OnDestroy | |||
| } | |||
|
|
|||
| // don't set the tabindex if there are no open sibling or parent menus | |||
| if (!event || (event && !this._getMenuStack().isEmpty())) { | |||
| if (!event || (event && !this._getMenuStack()?.isEmpty())) { | |||
There was a problem hiding this comment.
I think the event && here is redundant since it's assumed by the !event being true before.
andy9775
force-pushed
the
cdk-menu-fix-double-trigger-for-menu
branch
from
e4ed617
to
be0bf4a
Compare
|
@jelbourn @teflonwaffles @crisbeto feedback should be addressed |
…n the same menu Fixes the error where two different menu triggers open the same menu which leads to open menus not closing out on hover and other inconsistent behaviour. Opt to throw and error and not allow two triggers to share a menu.
andy9775
force-pushed
the
cdk-menu-fix-double-trigger-for-menu
branch
from
be0bf4a
to
8eb7939
Compare
jelbourn
added
action: merge
|
@andy9775: Is this going to affect the Material Menu and Material Menu Trigger as well? We are actually using two different triggers for the same menu with with a purpose. We have the classic tre-dot-menu but also show the menu when using right-click, only then we want the menu to appear where user is clicking. Since the easiest way to get the menu position to be where you click at is to place a trigger there, we have another trigger for that. With only one trigger we would have to move the dots to where the mouse is. Which obviously doesn't look that good. |
|
@JonWallsten this pr only impacts the cdk-experimental/menu package and not material. |
|
@JonWallsten in terms of the future- I want to attempt to land a change in Angular that would make this restriction unnecessary before changing the interns of |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes the error where two different menu triggers open the same menu which leads to open menus not
closing out on hover and other inconsistent behaviour. Opt to throw and error and not allow two
triggers to share a menu.