Skip to content

Commit

Permalink
fix(cdk/menu): control + option + space not working on VoiceOver
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
crisbeto committed Jul 3, 2023
1 parent b985c7d commit 8d73ddf
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 23 deletions.
37 changes: 37 additions & 0 deletions src/cdk/menu/event-detection.ts
Original file line number Diff line number Diff line change
@@ -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<HTMLElement>,
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;
}
18 changes: 5 additions & 13 deletions src/cdk/menu/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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
Expand All @@ -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<HTMLElement> = inject(ElementRef);
protected _ngZone = inject(NgZone);

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
14 changes: 5 additions & 9 deletions src/cdk/menu/menu-trigger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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});
Expand Down Expand Up @@ -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');
}
Expand Down Expand Up @@ -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');
}

/**
Expand Down
1 change: 0 additions & 1 deletion tools/public_api_guard/cdk/menu.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 8d73ddf

Please sign in to comment.