fix(material/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.

Author: crisbeto

src/material-experimental/mdc-menu/menu-item.html

@@ -1,5 +1,5 @@
 <ng-content select="mat-icon"></ng-content>
-<span class="mdc-list-item__primary-text"><ng-content></ng-content></span>
+<span class="mdc-list-item__primary-text mat-mdc-menu-item-content" #content><ng-content></ng-content></span>
 <div class="mat-mdc-menu-ripple" matRipple
      [matRippleDisabled]="disableRipple || disabled"
      [matRippleTrigger]="_getHostElement()"

src/material-experimental/mdc-menu/menu-item.ts

@@ -14,6 +14,7 @@ import {
   ElementRef,
   Optional,
   ChangeDetectorRef,
+  NgZone,
 } from '@angular/core';
 import {
   MAT_RIPPLE_GLOBAL_OPTIONS,
@@ -64,8 +65,9 @@ export class MatMenuItem extends BaseMatMenuItem {
     @Optional() @Inject(MAT_RIPPLE_GLOBAL_OPTIONS)
       globalRippleOptions?: RippleGlobalOptions,
     @Optional() @Inject(ANIMATION_MODULE_TYPE) animationMode?: string,
+    ngZone?: NgZone,
     changeDetectorRef?: ChangeDetectorRef) {
-    super(elementRef, document, focusMonitor, parentMenu, changeDetectorRef);
+    super(elementRef, document, focusMonitor, parentMenu, ngZone, changeDetectorRef);
 
     this._noopAnimations = animationMode === 'NoopAnimations';
     this._rippleAnimation = globalRippleOptions?.animation || {

src/material-experimental/mdc-menu/menu.scss

@@ -60,12 +60,6 @@ mat-menu {
   min-height: map.get($height-config, default);
 
   &[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;
 
     // This is the same as `mdc-list-mixins.list-disabled-opacity` which
@@ -107,6 +101,11 @@ mat-menu {
   }
 }
 
+.mat-mdc-menu-item-content {
+  display: flex;
+  align-items: center;
+}
+
 .mat-mdc-menu-item-submenu-trigger {
   @include menu-common.item-submenu-trigger(mdc-list-variables.$side-padding);
 }

src/material-experimental/mdc-menu/menu.spec.ts

@@ -639,6 +639,23 @@ describe('MDC-based MatMenu', () => {
     expect(items.every(item => item.getAttribute('role') === 'menuitemcheckbox')).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 not change focus origin if origin not specified for menu items', fakeAsync(() => {
     const fixture = createComponent(MenuWithCheckboxItems);
     fixture.detectChanges();

src/material/menu/menu-item.html

@@ -1,4 +1,4 @@
-<ng-content></ng-content>
+<span #content><ng-content></ng-content></span>
 <div class="mat-menu-ripple" matRipple
      [matRippleDisabled]="disableRipple || disabled"
      [matRippleTrigger]="_getHostElement()">

src/material/menu/menu-item.ts

@@ -18,8 +18,10 @@ import {
   Optional,
   Input,
   HostListener,
+  NgZone,
   AfterViewInit,
   ChangeDetectorRef,
+  ViewChild,
 } from '@angular/core';
 import {
   CanDisable,
@@ -62,6 +64,9 @@ export class MatMenuItem extends _MatMenuItemBase
   /** ARIA role for the menu item. */
   @Input() role: 'menuitem' | 'menuitemradio' | 'menuitemcheckbox' = 'menuitem';
 
+  /** Reference to the element wrapping the projected content. */
+  @ViewChild('content') _content: ElementRef<HTMLElement> | undefined;
+
   /** Stream that emits when the menu item is hovered. */
   readonly _hovered: Subject<MatMenuItem> = new Subject<MatMenuItem>();
 
@@ -83,13 +88,15 @@ export class MatMenuItem extends _MatMenuItemBase
     @Inject(DOCUMENT) _document?: any,
     private _focusMonitor?: FocusMonitor,
     @Inject(MAT_MENU_PANEL) @Optional() public _parentMenu?: MatMenuPanel<MatMenuItem>,
+    private _ngZone?: NgZone,
     /**
      * @deprecated `_changeDetectorRef` to become a required parameter.
      * @breaking-change 14.0.0
      */
-    private _changeDetectorRef?: ChangeDetectorRef) {
+     private _changeDetectorRef?: ChangeDetectorRef) {
 
     // @breaking-change 8.0.0 make `_focusMonitor` and `document` required params.
+    // @breaking-change 11.0.0 make `_ngZone` a required parameter.
     super();
 
     if (_parentMenu && _parentMenu.addItem) {
@@ -115,6 +122,13 @@ export class MatMenuItem extends _MatMenuItemBase
       // mouse or touch interaction.
       this._focusMonitor.monitor(this._elementRef, false);
     }
+
+    // @breaking-change 11.0.0 Remove null check for `_ngZone`.
+    if (this._ngZone) {
+      this._ngZone.runOutsideAngular(() => this._bindDisabledClickEvents());
+    } else {
+      this._bindDisabledClickEvents();
+    }
   }
 
   ngOnDestroy() {
@@ -126,6 +140,11 @@ export class MatMenuItem extends _MatMenuItemBase
       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();
   }
@@ -140,20 +159,6 @@ export class MatMenuItem extends _MatMenuItemBase
     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
@@ -188,6 +193,26 @@ export class MatMenuItem extends _MatMenuItemBase
     this._changeDetectorRef?.markForCheck();
   }
 
+  /** 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;
 }

src/material/menu/menu.scss

@@ -49,15 +49,6 @@ mat-menu {
   @include menu-common.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 a11y.high-contrast(active, off) {
     $outline-width: 1px;
 

src/material/menu/menu.spec.ts

@@ -639,6 +639,23 @@ describe('MatMenu', () => {
     expect(items.every(item => item.getAttribute('role') === 'menuitemcheckbox')).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 not change focus origin if origin not specified for menu items', fakeAsync(() => {
     const fixture = createComponent(MenuWithCheckboxItems);
     fixture.detectChanges();

tools/public_api_guard/material/menu.md

@@ -198,9 +198,9 @@ export class _MatMenuDirectivesModule {
 // @public
 export class MatMenuItem extends _MatMenuItemBase implements FocusableOption, CanDisable, CanDisableRipple, AfterViewInit, OnDestroy {
     constructor(_elementRef: ElementRef<HTMLElement>,
-    _document?: any, _focusMonitor?: FocusMonitor | undefined, _parentMenu?: MatMenuPanel<MatMenuItem> | undefined,
+    _document?: any, _focusMonitor?: FocusMonitor | undefined, _parentMenu?: MatMenuPanel<MatMenuItem> | undefined, _ngZone?: NgZone | undefined,
     _changeDetectorRef?: ChangeDetectorRef | undefined);
-    _checkDisabled(event: Event): void;
+    _content: ElementRef<HTMLElement> | undefined;
     focus(origin?: FocusOrigin, options?: FocusOptions): void;
     readonly _focused: Subject<MatMenuItem>;
     _getHostElement(): HTMLElement;
@@ -226,7 +226,7 @@ export class MatMenuItem extends _MatMenuItemBase implements FocusableOption, Ca
     // (undocumented)
     static ɵcmp: i0.ɵɵComponentDeclaration<MatMenuItem, "[mat-menu-item]", ["matMenuItem"], { "disabled": "disabled"; "disableRipple": "disableRipple"; "role": "role"; }, {}, never, ["*"]>;
     // (undocumented)
-    static ɵfac: i0.ɵɵFactoryDeclaration<MatMenuItem, [null, null, null, { optional: true; }, null]>;
+    static ɵfac: i0.ɵɵFactoryDeclaration<MatMenuItem, [null, null, null, { optional: true; }, null, null]>;
 }
 
 // @public (undocumented)