From d7ebb5120c959020281d97967baff1b0c2785dc7 Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 27 Apr 2020 17:03:49 +0200 Subject: [PATCH] fix(menu): clicks on disabled item closing the menu We have some logic to prevent clicks on disabled items from closing the menu. The problem is that browsers swallow clicks and don't fire their event listeners for disabled `button` nodes, but not any non-disabled child nodes. In #16696 we made it consistent so clicks don't land on any of the button's elements, but these changes fix the underlying issue by binding the event both on the root `button`, and a wrapper `span` that's around the content. This way we're guaranteed to hit at least one of the listeners. These changes also move the event outside the `NgZone` since it doesn't trigger any changes in the view. Fixes #19173. --- .../mdc-menu/menu-item.html | 2 +- src/material-experimental/mdc-menu/menu.scss | 6 -- .../mdc-menu/menu.spec.ts | 17 +++++ src/material/menu/menu-item.html | 2 +- src/material/menu/menu-item.ts | 63 ++++++++++++++----- src/material/menu/menu.scss | 9 --- src/material/menu/menu.spec.ts | 17 +++++ tools/public_api_guard/material/menu.d.ts | 9 +-- 8 files changed, 87 insertions(+), 38 deletions(-) diff --git a/src/material-experimental/mdc-menu/menu-item.html b/src/material-experimental/mdc-menu/menu-item.html index 3092e914580e..72655e266cbe 100644 --- a/src/material-experimental/mdc-menu/menu-item.html +++ b/src/material-experimental/mdc-menu/menu-item.html @@ -1,4 +1,4 @@ - +
diff --git a/src/material-experimental/mdc-menu/menu.scss b/src/material-experimental/mdc-menu/menu.scss index 949db115c9fb..6cbf537a4c6d 100644 --- a/src/material-experimental/mdc-menu/menu.scss +++ b/src/material-experimental/mdc-menu/menu.scss @@ -53,12 +53,6 @@ text-decoration: none; &[disabled] { - // Usually every click inside the menu closes it, however some browsers will stop events - // when the user clicks on a disabled item, **except** when the user clicks on a non-disabled - // child node of the disabled button. This is inconsistent because some regions of a disabled - // button will still cause the menu to close and some won't (see #16694). We make the behavior - // more consistent by disabling pointer events and allowing the user to click through. - pointer-events: none; cursor: default; } diff --git a/src/material-experimental/mdc-menu/menu.spec.ts b/src/material-experimental/mdc-menu/menu.spec.ts index 59c316d51a4f..f1ba38e30075 100644 --- a/src/material-experimental/mdc-menu/menu.spec.ts +++ b/src/material-experimental/mdc-menu/menu.spec.ts @@ -569,6 +569,23 @@ describe('MDC-based MatMenu', () => { expect(items.every(item => item.getAttribute('role') === 'menuitem')).toBe(true); }); + it('should prevent the default action when clicking on a disabled item', () => { + const fixture = createComponent(SimpleMenu, [], [FakeIcon]); + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + + const item = overlayContainerElement.querySelector('.mat-mdc-menu-item[disabled]')!; + const itemEvent = dispatchFakeEvent(item, 'click'); + fixture.detectChanges(); + expect(itemEvent.defaultPrevented).toBe(true); + + const contentWrapper = item.querySelector('span')!; + const wrapperEvent = dispatchFakeEvent(contentWrapper, 'click'); + fixture.detectChanges(); + expect(wrapperEvent.defaultPrevented).toBe(true); + }); + it('should be able to set an alternate role on the menu items', () => { const fixture = createComponent(MenuWithCheckboxItems); fixture.detectChanges(); diff --git a/src/material/menu/menu-item.html b/src/material/menu/menu-item.html index a025ff57a01e..53a7b2c4d003 100644 --- a/src/material/menu/menu-item.html +++ b/src/material/menu/menu-item.html @@ -1,4 +1,4 @@ - +
diff --git a/src/material/menu/menu-item.ts b/src/material/menu/menu-item.ts index 469feb9027a8..9b6d21df94d4 100644 --- a/src/material/menu/menu-item.ts +++ b/src/material/menu/menu-item.ts @@ -18,6 +18,9 @@ import { Optional, Input, HostListener, + NgZone, + AfterViewInit, + ViewChild, } from '@angular/core'; import { CanDisable, CanDisableCtor, @@ -57,11 +60,14 @@ const _MatMenuItemMixinBase: CanDisableRippleCtor & CanDisableCtor & typeof MatM templateUrl: 'menu-item.html', }) export class MatMenuItem extends _MatMenuItemMixinBase - implements FocusableOption, CanDisable, CanDisableRipple, OnDestroy { + implements FocusableOption, CanDisable, CanDisableRipple, AfterViewInit, OnDestroy { /** ARIA role for the menu item. */ @Input() role: 'menuitem' | 'menuitemradio' | 'menuitemcheckbox' = 'menuitem'; + /** Reference to the element wrapping the projected content. */ + @ViewChild('content') _content: ElementRef | undefined; + private _document: Document; /** Stream that emits when the menu item is hovered. */ @@ -80,9 +86,11 @@ export class MatMenuItem extends _MatMenuItemMixinBase private _elementRef: ElementRef, @Inject(DOCUMENT) document?: any, private _focusMonitor?: FocusMonitor, - @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel) { + @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel, + private _ngZone?: NgZone) { // @breaking-change 8.0.0 make `_focusMonitor` and `document` required params. + // @breaking-change 11.0.0 make `_ngZone` a required parameter. super(); if (_focusMonitor) { @@ -110,6 +118,15 @@ export class MatMenuItem extends _MatMenuItemMixinBase this._focused.next(this); } + ngAfterViewInit() { + // @breaking-change 11.0.0 Remove null check for `_ngZone`. + if (this._ngZone) { + this._ngZone.runOutsideAngular(() => this._bindDisabledClickEvents()); + } else { + this._bindDisabledClickEvents(); + } + } + ngOnDestroy() { if (this._focusMonitor) { this._focusMonitor.stopMonitoring(this._elementRef); @@ -119,6 +136,11 @@ export class MatMenuItem extends _MatMenuItemMixinBase this._parentMenu.removeItem(this); } + this._elementRef.nativeElement.removeEventListener('click', this._preventDisabledClicks); + if (this._content) { + this._content.nativeElement.removeEventListener('click', this._preventDisabledClicks); + } + this._hovered.complete(); this._focused.complete(); } @@ -133,20 +155,6 @@ export class MatMenuItem extends _MatMenuItemMixinBase return this._elementRef.nativeElement; } - /** Prevents the default element actions if it is disabled. */ - // We have to use a `HostListener` here in order to support both Ivy and ViewEngine. - // In Ivy the `host` bindings will be merged when this class is extended, whereas in - // ViewEngine they're overwritten. - // TODO(crisbeto): we move this back into `host` once Ivy is turned on by default. - // tslint:disable-next-line:no-host-decorator-in-concrete - @HostListener('click', ['$event']) - _checkDisabled(event: Event): void { - if (this.disabled) { - event.preventDefault(); - event.stopPropagation(); - } - } - /** Emits to the hover stream. */ // We have to use a `HostListener` here in order to support both Ivy and ViewEngine. // In Ivy the `host` bindings will be merged when this class is extended, whereas in @@ -160,7 +168,8 @@ export class MatMenuItem extends _MatMenuItemMixinBase /** Gets the label to be used when determining whether the option should be focused. */ getLabel(): string { - const element: HTMLElement = this._elementRef.nativeElement; + const element: HTMLElement = this._content ? + this._content.nativeElement : this._elementRef.nativeElement; const textNodeType = this._document ? this._document.TEXT_NODE : 3; let output = ''; @@ -180,6 +189,26 @@ export class MatMenuItem extends _MatMenuItemMixinBase return output.trim(); } + /** Binds the click events that prevent the default actions while disabled. */ + private _bindDisabledClickEvents() { + // We need to bind this event both on the root node and the content wrapper, because browsers + // won't dispatch events on disabled `button` nodes, but they'll still be dispatched if the + // user interacts with a non-disabled child of the button. This means that can get regions + // inside a disabled menu item where clicks land and others where they don't. + this._elementRef.nativeElement.addEventListener('click', this._preventDisabledClicks); + if (this._content) { + this._content.nativeElement.addEventListener('click', this._preventDisabledClicks); + } + } + + /** Prevents the default click action if the menu item is disabled. */ + private _preventDisabledClicks = (event: Event) => { + if (this.disabled) { + event.preventDefault(); + event.stopPropagation(); + } + } + static ngAcceptInputType_disabled: BooleanInput; static ngAcceptInputType_disableRipple: BooleanInput; } diff --git a/src/material/menu/menu.scss b/src/material/menu/menu.scss index afcd241fc5ee..d737530018a8 100644 --- a/src/material/menu/menu.scss +++ b/src/material/menu/menu.scss @@ -44,15 +44,6 @@ $mat-menu-submenu-indicator-size: 10px !default; @include mat-menu-item-base(); position: relative; - &[disabled] { - // Usually every click inside the menu closes it, however some browsers will stop events - // when the user clicks on a disabled item, **except** when the user clicks on a non-disabled - // child node of the disabled button. This is inconsistent because some regions of a disabled - // button will still cause the menu to close and some won't (see #16694). We make the behavior - // more consistent by disabling pointer events and allowing the user to click through. - pointer-events: none; - } - @include cdk-high-contrast(active, off) { &.cdk-program-focused, &.cdk-keyboard-focused, diff --git a/src/material/menu/menu.spec.ts b/src/material/menu/menu.spec.ts index 017fb1e1a66d..9cd0338ad3de 100644 --- a/src/material/menu/menu.spec.ts +++ b/src/material/menu/menu.spec.ts @@ -569,6 +569,23 @@ describe('MatMenu', () => { expect(items.every(item => item.getAttribute('role') === 'menuitem')).toBe(true); }); + it('should prevent the default action when clicking on a disabled item', () => { + const fixture = createComponent(SimpleMenu, [], [FakeIcon]); + fixture.detectChanges(); + fixture.componentInstance.trigger.openMenu(); + fixture.detectChanges(); + + const item = overlayContainerElement.querySelector('.mat-menu-item[disabled]')!; + const itemEvent = dispatchFakeEvent(item, 'click'); + fixture.detectChanges(); + expect(itemEvent.defaultPrevented).toBe(true); + + const contentWrapper = item.querySelector('span')!; + const wrapperEvent = dispatchFakeEvent(contentWrapper, 'click'); + fixture.detectChanges(); + expect(wrapperEvent.defaultPrevented).toBe(true); + }); + it('should be able to set an alternate role on the menu items', () => { const fixture = createComponent(MenuWithCheckboxItems); fixture.detectChanges(); diff --git a/tools/public_api_guard/material/menu.d.ts b/tools/public_api_guard/material/menu.d.ts index a53cc451b3e7..f6aa54c20335 100644 --- a/tools/public_api_guard/material/menu.d.ts +++ b/tools/public_api_guard/material/menu.d.ts @@ -98,25 +98,26 @@ export interface MatMenuDefaultOptions { yPosition: MenuPositionY; } -export declare class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOption, CanDisable, CanDisableRipple, OnDestroy { +export declare class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOption, CanDisable, CanDisableRipple, AfterViewInit, OnDestroy { + _content: ElementRef | undefined; readonly _focused: Subject; _highlighted: boolean; readonly _hovered: Subject; _parentMenu?: MatMenuPanel | undefined; _triggersSubmenu: boolean; role: 'menuitem' | 'menuitemradio' | 'menuitemcheckbox'; - constructor(_elementRef: ElementRef, document?: any, _focusMonitor?: FocusMonitor | undefined, _parentMenu?: MatMenuPanel | undefined); - _checkDisabled(event: Event): void; + constructor(_elementRef: ElementRef, document?: any, _focusMonitor?: FocusMonitor | undefined, _parentMenu?: MatMenuPanel | undefined, _ngZone?: NgZone | undefined); _getHostElement(): HTMLElement; _getTabIndex(): string; _handleMouseEnter(): void; focus(origin?: FocusOrigin, options?: FocusOptions): void; getLabel(): string; + ngAfterViewInit(): void; ngOnDestroy(): void; static ngAcceptInputType_disableRipple: BooleanInput; static ngAcceptInputType_disabled: BooleanInput; static ɵcmp: i0.ɵɵComponentDefWithMeta; - static ɵfac: i0.ɵɵFactoryDef; + static ɵfac: i0.ɵɵFactoryDef; } export declare class MatMenuModule {