Skip to content

Commit

Permalink
fix(menu,list,select): do not stopPropagation on native events when h…
Browse files Browse the repository at this point in the history
…andled only prevent default

Fixes #4817

We need to communicate between components when an event has been handled (e.g. keyboard navigation or clicking). This CL focuses on listening to `defaultPrevented` in order to communicate that something was handled. We also have to patch ripple on submenu to make sure that the ripple isn't triggered.

PiperOrigin-RevId: 561748634
  • Loading branch information
e111077 authored and Copybara-Service committed Aug 31, 2023
1 parent fcfc696 commit b85b57f
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 20 deletions.
6 changes: 2 additions & 4 deletions list/internal/list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,7 @@ export class List extends LitElement {
* The content to be slotted into the list.
*/
private renderContent() {
return html`<span><slot @click=${(event: Event) => {
event.stopPropagation();
}}></slot></span>`;
return html`<span><slot></slot></span>`;
}

/**
Expand All @@ -104,7 +102,7 @@ export class List extends LitElement {
*/
private handleKeydown(event: KeyboardEvent) {
const key = event.key;
if (!isNavigableKey(key)) {
if (event.defaultPrevented || !isNavigableKey(key)) {
return;
}
// do not use this.items directly so we don't re-query the DOM unnecessarily
Expand Down
3 changes: 3 additions & 0 deletions menu/internal/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,9 @@ export abstract class Menu extends LitElement {
if (!isServer) {
window.addEventListener('click', this.onWindowClick, {capture: true});
}

// need to self-identify as an md-menu for submenu ripple identification.
this.toggleAttribute('md-menu', true);
}

override disconnectedCallback() {
Expand Down
2 changes: 1 addition & 1 deletion menu/internal/menuitem/menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export class MenuItemEl extends ListItemEl implements MenuItem {
}

protected override onKeydown(event: KeyboardEvent) {
if (this.keepOpen) return;
if (this.keepOpen || event.defaultPrevented) return;
const keyCode = event.code;

if (!event.defaultPrevented && isClosableKey(keyCode)) {
Expand Down
64 changes: 51 additions & 13 deletions menu/internal/submenuitem/sub-menu-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,15 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {html} from 'lit';
import {property, queryAssignedElements, state} from 'lit/decorators.js';
import {html, PropertyValues} from 'lit';
import {property, query, queryAssignedElements, state} from 'lit/decorators.js';

import {List} from '../../../list/internal/list.js';
import {MdRipple} from '../../../ripple/ripple.js';
import {Corner, Menu} from '../menu.js';
import {MenuItemEl} from '../menuitem/menu-item.js';
import {CLOSE_REASON, CloseMenuEvent, createActivateTypeaheadEvent, createCloseOnFocusoutEvent, createDeactivateItemsEvent, createDeactivateTypeaheadEvent, createStayOpenOnFocusoutEvent, KEYDOWN_CLOSE_KEYS, NAVIGABLE_KEY, SELECTION_KEY} from '../shared.js';

function stopPropagation(event: Event) {
event.stopPropagation();
}

/**
* @fires deactivate-items Requests the parent menu to deselect other items when
* a submenu opens
Expand Down Expand Up @@ -54,6 +51,8 @@ export class SubMenuItem extends MenuItemEl {

@state() protected submenuHover = false;

@query('md-ripple') private readonly rippleEl!: MdRipple;

@queryAssignedElements({slot: 'submenu', flatten: true})
private readonly menus!: Menu[];

Expand Down Expand Up @@ -116,6 +115,8 @@ export class SubMenuItem extends MenuItemEl {
protected override onKeydown(event: KeyboardEvent) {
const shouldOpenSubmenu = this.isSubmenuOpenKey(event.code);

if (event.defaultPrevented) return;

if (event.code === SELECTION_KEY.SPACE) {
// prevent space from scrolling. Only open the submenu.
event.preventDefault();
Expand Down Expand Up @@ -156,13 +157,21 @@ export class SubMenuItem extends MenuItemEl {
name="submenu"
@pointerenter=${this.onSubmenuPointerEnter}
@pointerleave=${this.onSubmenuPointerLeave}
@pointerdown=${stopPropagation}
@click=${stopPropagation}
@keydown=${this.onSubMenuKeydown}
@close-menu=${this.onCloseSubmenu}
></slot></span>`;
}

override firstUpdated(changed: PropertyValues<this>) {
super.firstUpdated(changed);

const {handleEvent} = this.rippleEl;

// TODO(b/298476971): remove once ripple has a better solution
this.rippleEl.handleEvent =
callIfEventNotBubbledThroughMenu(this, handleEvent.bind(this.rippleEl));
}

private onCloseSubmenu(event: CloseMenuEvent) {
const {itemPath, reason} = event.detail;
itemPath.push(this);
Expand All @@ -186,14 +195,16 @@ export class SubMenuItem extends MenuItemEl {
}

private async onSubMenuKeydown(event: KeyboardEvent) {
// Stop propagation so that we don't accidentally close every parent menu.
// Additionally, we want to isolate things like the typeahead keydowns
// from bubbling up to the parent menu and confounding things.
event.stopPropagation();
if (event.defaultPrevented) return;
const shouldClose = this.isSubmenuCloseKey(event.code);

if (!shouldClose) return;

// Communicate that it's handled so that we don't accidentally close every
// parent menu. Additionally, we want to isolate things like the typeahead
// keydowns from bubbling up to the parent menu and confounding things.
event.preventDefault();

this.close(() => {
List.deactivateActiveItem(this.submenuEl!.items);
this.listItemRoot?.focus();
Expand All @@ -208,7 +219,7 @@ export class SubMenuItem extends MenuItemEl {
*/
show(onOpened = () => {}) {
const menu = this.submenuEl;
if (!menu) return;
if (!menu || menu.open) return;

menu.quick = true;
// Submenus are in overflow when not fixed. Can remove once we have native
Expand Down Expand Up @@ -315,3 +326,30 @@ export class SubMenuItem extends MenuItemEl {
this.submenuHover = false;
}
}

/**
* Calls the given callback if the event has not bubbled through an md-menu.
*/
function callIfEventNotBubbledThroughMenu(
menuItem: HTMLElement, callback: (event: Event) => Promise<void>) {
return async (event: Event) => {
const path = event.composedPath();

for (const element of path) {
const isMenu =
!!(element as Element | HTMLElement)?.hasAttribute?.('md-menu');

// The path has a submenu, do not invoke callback;
if (isMenu) return;

// We have reached the target submenu. Any other menus in path are
// ancestors.
if (element === menuItem) {
break;
}
}

// invoke the callback because we have not run into a submenu.
await callback(event);
};
}
2 changes: 1 addition & 1 deletion menu/internal/typeaheadController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ export class TypeaheadController {
* activates Olive
*/
private typeahead(event: KeyboardEvent) {
if (event.defaultPrevented) return;
clearTimeout(this.cancelTypeaheadTimeout);
// Stop typingahead if one of the navigation or selection keys (except for
// Space) are pressed
Expand All @@ -210,7 +211,6 @@ export class TypeaheadController {

// If Space is pressed, prevent it from selecting and closing the menu
if (event.code === 'Space') {
event.stopPropagation();
event.preventDefault();
}

Expand Down
10 changes: 9 additions & 1 deletion menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,22 @@ declare global {
* @example
* ```html
* <div style="position:relative;">
* <!--
* The menu ref in this example can be achieved by any method such as the
* preferred `@query` decorator in Lit or a ref in React.
* -->
* <button
* id="anchor"
* @click=${() => this.menuRef.value.show()}>
* Click to open menu
* </button>
* <!--
* `has-overflow` is required when using a submenu which overflows the
* menu's contents
* menu's contents.
*
* Additionally, `anchor` ingests an idref which do not pass through shadow
* roots. You can also set `.anchorElement` to an element reference if
* necessary.
* -->
* <md-menu anchor="anchor" has-overflow ${ref(menuRef)}>
* <md-menu-item header="This is a header"></md-menu-item>
Expand Down

0 comments on commit b85b57f

Please sign in to comment.