From 8e4a2ecf022a81edd6c4273039d74dc6b9c39a24 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 3 Jul 2023 14:13:36 +0200 Subject: [PATCH] fix(cdk/menu): control + option + space not working on VoiceOver (#27401) In #26296 I added a condition in the click handler for menu triggers and menu items to skip clicks dispatched by the keyboard so that they don't trigger the menu twice. The problem is that this also ends up ignoring the default keyboard shortcut that VoiceOver mentions should be used to trigger the menu (Control + Option + Space), because it ends up being dispatched as a plain `click` event and it doesn't trigger the `keydown` event at all. These changes address the issue by removing the previous condition and inferring whether the event will trigger a click by looking at it and the element it's coming from. Fixes #27376. --- src/cdk/menu/event-detection.ts | 37 ++++++++++++++++++++++++++++++ src/cdk/menu/menu-item.ts | 18 ++++----------- src/cdk/menu/menu-trigger.ts | 14 ++++------- tools/public_api_guard/cdk/menu.md | 1 - 4 files changed, 47 insertions(+), 23 deletions(-) create mode 100644 src/cdk/menu/event-detection.ts diff --git a/src/cdk/menu/event-detection.ts b/src/cdk/menu/event-detection.ts new file mode 100644 index 000000000000..bcace6e3fc70 --- /dev/null +++ b/src/cdk/menu/event-detection.ts @@ -0,0 +1,37 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {ElementRef} from '@angular/core'; +import {ENTER, SPACE} from '@angular/cdk/keycodes'; + +/** Checks whether a keyboard event will trigger a native `click` event on an element. */ +export function eventDispatchesNativeClick( + elementRef: ElementRef, + event: KeyboardEvent, +): boolean { + // Synthetic events won't trigger clicks. + if (!event.isTrusted) { + return false; + } + + const el = elementRef.nativeElement; + const keyCode = event.keyCode; + + // Buttons trigger clicks both on space and enter events. + if (el.nodeName === 'BUTTON' && !(el as HTMLButtonElement).disabled) { + return keyCode === ENTER || keyCode === SPACE; + } + + // Links only trigger clicks on enter. + if (el.nodeName === 'A') { + return keyCode === ENTER; + } + + // Any other elements won't dispatch clicks from keyboard events. + return false; +} diff --git a/src/cdk/menu/menu-item.ts b/src/cdk/menu/menu-item.ts index a23fb46cb893..9a71549cad2e 100644 --- a/src/cdk/menu/menu-item.ts +++ b/src/cdk/menu/menu-item.ts @@ -17,7 +17,7 @@ import { Output, } from '@angular/core'; import {BooleanInput, coerceBooleanProperty} from '@angular/cdk/coercion'; -import {FocusableOption, InputModalityDetector} from '@angular/cdk/a11y'; +import {FocusableOption} from '@angular/cdk/a11y'; import {ENTER, hasModifierKey, LEFT_ARROW, RIGHT_ARROW, SPACE} from '@angular/cdk/keycodes'; import {Directionality} from '@angular/cdk/bidi'; import {fromEvent, Subject} from 'rxjs'; @@ -27,6 +27,7 @@ import {CDK_MENU, Menu} from './menu-interface'; import {FocusNext, MENU_STACK} from './menu-stack'; import {FocusableElement} from './pointer-focus-tracker'; import {MENU_AIM, Toggler} from './menu-aim'; +import {eventDispatchesNativeClick} from './event-detection'; /** * Directive which provides the ability for an element to be focused and navigated to using the @@ -44,13 +45,12 @@ import {MENU_AIM, Toggler} from './menu-aim'; '[attr.aria-disabled]': 'disabled || null', '(blur)': '_resetTabIndex()', '(focus)': '_setTabIndex()', - '(click)': '_handleClick()', + '(click)': 'trigger()', '(keydown)': '_onKeydown($event)', }, }) export class CdkMenuItem implements FocusableOption, FocusableElement, Toggler, OnDestroy { protected readonly _dir = inject(Directionality, {optional: true}); - private readonly _inputModalityDetector = inject(InputModalityDetector); readonly _elementRef: ElementRef = inject(ElementRef); protected _ngZone = inject(NgZone); @@ -195,7 +195,8 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, Toggler, switch (event.keyCode) { case SPACE: case ENTER: - if (!hasModifierKey(event)) { + // Skip events that will trigger clicks so the handler doesn't get triggered twice. + if (!hasModifierKey(event) && !eventDispatchesNativeClick(this._elementRef, event)) { this.trigger({keepOpen: event.keyCode === SPACE && !this.closeOnSpacebarTrigger}); } break; @@ -226,15 +227,6 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, Toggler, } } - /** Handles clicks on the menu item. */ - _handleClick() { - // Don't handle clicks originating from the keyboard since we - // already do the same on `keydown` events for enter and space. - if (this._inputModalityDetector.mostRecentModality !== 'keyboard') { - this.trigger(); - } - } - /** Whether this menu item is standalone or within a menu or menu bar. */ private _isStandaloneItem() { return !this._parentMenu; diff --git a/src/cdk/menu/menu-trigger.ts b/src/cdk/menu/menu-trigger.ts index 481e8d7b0fea..1ec2f721c264 100644 --- a/src/cdk/menu/menu-trigger.ts +++ b/src/cdk/menu/menu-trigger.ts @@ -26,13 +26,13 @@ import { UP_ARROW, } from '@angular/cdk/keycodes'; import {_getEventTarget} from '@angular/cdk/platform'; -import {InputModalityDetector} from '@angular/cdk/a11y'; import {fromEvent} from 'rxjs'; import {filter, takeUntil} from 'rxjs/operators'; import {CDK_MENU, Menu} from './menu-interface'; import {PARENT_OR_NEW_MENU_STACK_PROVIDER} from './menu-stack'; import {MENU_AIM} from './menu-aim'; import {CdkMenuTriggerBase, MENU_TRIGGER} from './menu-trigger-base'; +import {eventDispatchesNativeClick} from './event-detection'; /** * A directive that turns its host element into a trigger for a popup menu. @@ -70,7 +70,6 @@ export class CdkMenuTrigger extends CdkMenuTriggerBase implements OnDestroy { private readonly _overlay = inject(Overlay); private readonly _ngZone = inject(NgZone); private readonly _directionality = inject(Directionality, {optional: true}); - private readonly _inputModalityDetector = inject(InputModalityDetector); /** The parent menu this trigger belongs to. */ private readonly _parentMenu = inject(CDK_MENU, {optional: true}); @@ -130,7 +129,8 @@ export class CdkMenuTrigger extends CdkMenuTriggerBase implements OnDestroy { switch (event.keyCode) { case SPACE: case ENTER: - if (!hasModifierKey(event)) { + // Skip events that will trigger clicks so the handler doesn't get triggered twice. + if (!hasModifierKey(event) && !eventDispatchesNativeClick(this._elementRef, event)) { this.toggle(); this.childMenu?.focusFirstItem('keyboard'); } @@ -173,12 +173,8 @@ export class CdkMenuTrigger extends CdkMenuTriggerBase implements OnDestroy { /** Handles clicks on the menu trigger. */ _handleClick() { - // Don't handle clicks originating from the keyboard since we - // already do the same on `keydown` events for enter and space. - if (this._inputModalityDetector.mostRecentModality !== 'keyboard') { - this.toggle(); - this.childMenu?.focusFirstItem('mouse'); - } + this.toggle(); + this.childMenu?.focusFirstItem('mouse'); } /** diff --git a/tools/public_api_guard/cdk/menu.md b/tools/public_api_guard/cdk/menu.md index 3da20723be20..c9e364de0c9a 100644 --- a/tools/public_api_guard/cdk/menu.md +++ b/tools/public_api_guard/cdk/menu.md @@ -129,7 +129,6 @@ export class CdkMenuItem implements FocusableOption, FocusableElement, Toggler, getLabel(): string; getMenu(): Menu | undefined; getMenuTrigger(): CdkMenuTrigger | null; - _handleClick(): void; get hasMenu(): boolean; isMenuOpen(): boolean; // (undocumented)