From b85b57fa792663460dbc1730c9e574e2b5ab998d Mon Sep 17 00:00:00 2001 From: Elliott Marquez Date: Thu, 31 Aug 2023 13:42:43 -0700 Subject: [PATCH] fix(menu,list,select): do not stopPropagation on native events when handled 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 --- list/internal/list.ts | 6 +- menu/internal/menu.ts | 3 + menu/internal/menuitem/menu-item.ts | 2 +- menu/internal/submenuitem/sub-menu-item.ts | 64 +++++++++++++++++----- menu/internal/typeaheadController.ts | 2 +- menu/menu.ts | 10 +++- 6 files changed, 67 insertions(+), 20 deletions(-) diff --git a/list/internal/list.ts b/list/internal/list.ts index 1709fbf07f..3bb3a2f158 100644 --- a/list/internal/list.ts +++ b/list/internal/list.ts @@ -92,9 +92,7 @@ export class List extends LitElement { * The content to be slotted into the list. */ private renderContent() { - return html` { - event.stopPropagation(); - }}>`; + return html``; } /** @@ -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 diff --git a/menu/internal/menu.ts b/menu/internal/menu.ts index 4ead6d65c6..24a642ec12 100644 --- a/menu/internal/menu.ts +++ b/menu/internal/menu.ts @@ -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() { diff --git a/menu/internal/menuitem/menu-item.ts b/menu/internal/menuitem/menu-item.ts index b77a9a2d75..4fab4ed71d 100644 --- a/menu/internal/menuitem/menu-item.ts +++ b/menu/internal/menuitem/menu-item.ts @@ -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)) { diff --git a/menu/internal/submenuitem/sub-menu-item.ts b/menu/internal/submenuitem/sub-menu-item.ts index e4abeeb954..9fa867c7fb 100644 --- a/menu/internal/submenuitem/sub-menu-item.ts +++ b/menu/internal/submenuitem/sub-menu-item.ts @@ -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 @@ -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[]; @@ -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(); @@ -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} >`; } + override firstUpdated(changed: PropertyValues) { + 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); @@ -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(); @@ -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 @@ -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) { + 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); + }; +} \ No newline at end of file diff --git a/menu/internal/typeaheadController.ts b/menu/internal/typeaheadController.ts index 834b19cfa9..1f5c5edfbc 100644 --- a/menu/internal/typeaheadController.ts +++ b/menu/internal/typeaheadController.ts @@ -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 @@ -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(); } diff --git a/menu/menu.ts b/menu/menu.ts index ece6df12ed..1a5ffcbe7e 100644 --- a/menu/menu.ts +++ b/menu/menu.ts @@ -37,6 +37,10 @@ declare global { * @example * ```html *
+ * * * * *