From 56366ce2750bb4bb5c6e3fa5fe7d809434497adb Mon Sep 17 00:00:00 2001 From: TaraT Date: Mon, 29 Jul 2024 14:51:47 +0530 Subject: [PATCH] chore(picker): added a new interaction strategy (#4396) * chore: testing a click controller strategy for picker component * chore: testing a new interaction strategy for picker * fix: separated interaction strategy for picker * chore: updated the strategy to include the dependency controller * chore: updated open condition in interactioncontroller * chore(picker): removed double event handling from action menu * chore: added slottable-request handling in interactionController * chore: change trigger button for split-button * chore: updated split-button test * chore: fixed conflicts after merging main * chore: updated action-group test to remove flakiness * chore: refractored interaction controller code * chore: added delay for click event tests * chore: added hacky mobile tests for mobilecontroller picker * chore: removed unexpected only from picker responsive test * chore: updated menu interaction model * chore: don't dispatch synthetic click event on menu selection * chore: removed split-button tests cz its deprecated --------- Co-authored-by: Michael Jordan --- packages/action-menu/src/ActionMenu.ts | 4 +- packages/action-menu/test/index.ts | 210 ++++++++---------- packages/menu/src/Menu.ts | 14 +- packages/picker/package.json | 16 ++ packages/picker/src/DesktopController.ts | 87 ++++++++ packages/picker/src/InteractionController.ts | 189 ++++++++++++++++ packages/picker/src/MobileController.ts | 52 +++++ packages/picker/src/Picker.ts | 193 +++++++--------- packages/picker/src/strategies.ts | 19 ++ packages/picker/test/index.ts | 3 + .../picker/test/picker-responsive.test.ts | 64 +++--- packages/split-button/src/SplitButton.ts | 6 +- packages/split-button/test/index.ts | 143 +----------- test/plugins/send-mouse-plugin.ts | 5 +- 14 files changed, 598 insertions(+), 407 deletions(-) create mode 100644 packages/picker/src/DesktopController.ts create mode 100644 packages/picker/src/InteractionController.ts create mode 100644 packages/picker/src/MobileController.ts create mode 100644 packages/picker/src/strategies.ts diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index 2bcad9d582..8cfef94fa9 100644 --- a/packages/action-menu/src/ActionMenu.ts +++ b/packages/action-menu/src/ActionMenu.ts @@ -64,7 +64,7 @@ export class ActionMenu extends ObserveSlotPresence( return this.slotContentIsPresent; } - protected override handleSlottableRequest = ( + public override handleSlottableRequest = ( event: SlottableRequestEvent ): void => { this.dispatchEvent(new SlottableRequestEvent(event.name, event.data)); @@ -113,8 +113,6 @@ export class ActionMenu extends ObserveSlotPresence( class="button" size=${this.size} @blur=${this.handleButtonBlur} - @click=${this.handleActivate} - @pointerdown=${this.handleButtonPointerdown} @focus=${this.handleButtonFocus} @keydown=${{ handleEvent: this.handleEnterKeydown, diff --git a/packages/action-menu/test/index.ts b/packages/action-menu/test/index.ts index 5599c25ab8..57b9829ca5 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -43,26 +43,9 @@ import { sendKeys } from '@web/test-runner-commands'; ignoreResizeObserverLoopError(before, after); const deprecatedActionMenuFixture = async (): Promise => - await fixture( - html` - - - Deselect - Select Inverse - Feather... - Select and Mask... - - Save Selection - Make Work Path - - - ` - ); - -const actionMenuFixture = async (): Promise => - await fixture( - html` - + await fixture(html` + + Deselect Select Inverse Feather... @@ -70,31 +53,40 @@ const actionMenuFixture = async (): Promise => Save Selection Make Work Path - - ` - ); + + + `); + +const actionMenuFixture = async (): Promise => + await fixture(html` + + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + Make Work Path + + `); const actionSubmenuFixture = async (): Promise => - await fixture( - html` - - One - - Two - - - B should be selected - - A - - B - - C - - - - ` - ); + await fixture(html` + + One + Two + + B should be selected + + A + + B + + C + + + + `); export const testActionMenu = (mode: 'sync' | 'async'): void => { describe(`Action menu: ${mode}`, () => { @@ -108,20 +100,18 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { await expect(el).to.be.accessible(); }); it('loads - [slot="label"]', async () => { - const el = await fixture( - html` - - More Actions - Deselect - Select Inverse - Feather... - Select and Mask... - - Save Selection - Make Work Path - - ` - ); + const el = await fixture(html` + + More Actions + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + Make Work Path + + `); await elementUpdated(el); await nextFrame(); @@ -130,20 +120,18 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { await expect(el).to.be.accessible(); }); it('loads - [custom icon]', async () => { - const el = await fixture( - html` - - - Deselect - Select Inverse - Feather... - Select and Mask... - - Save Selection - Make Work Path - - ` - ); + const el = await fixture(html` + + + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + Make Work Path + + `); await elementUpdated(el); await nextFrame(); @@ -154,27 +142,25 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { it('dispatches change events, no [href]', async () => { const changeSpy = spy(); - const el = await fixture( - html` - { - changeSpy(value); - }} - > - - Deselect - Select Inverse - Feather... - Select and Mask... - - Save Selection - Make Work Path - - ` - ); + const el = await fixture(html` + { + changeSpy(value); + }} + > + + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + Make Work Path + + `); expect(changeSpy.callCount).to.equal(0); expect(el.open).to.be.false; @@ -200,27 +186,25 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { it('closes when Menu Item has [href]', async () => { const changeSpy = spy(); - const el = await fixture( - html` - { - changeSpy(); - }} - > - - Deselect - Select Inverse - Feather... - Select and Mask... - - Save Selection - - Make Work Path - - - ` - ); + const el = await fixture(html` + { + changeSpy(); + }} + > + + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + + Make Work Path + + + `); expect(changeSpy.callCount).to.equal(0); expect(el.open).to.be.false; diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index a28059c81c..ec40e7ec41 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -342,22 +342,20 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } } - private willSynthesizeClick = 0; + // if the click and pointerup events are on the same target, we should not + // handle the click event. + private pointerUpTarget = null as EventTarget | null; private handleClick(event: Event): void { - if (this.willSynthesizeClick) { - cancelAnimationFrame(this.willSynthesizeClick); - this.willSynthesizeClick = 0; + if (this.pointerUpTarget === event.target) { + this.pointerUpTarget = null; return; } this.handlePointerBasedSelection(event); } private handlePointerup(event: Event): void { - this.willSynthesizeClick = requestAnimationFrame(() => { - event.target?.dispatchEvent(new Event('click')); - this.willSynthesizeClick = 0; - }); + this.pointerUpTarget = event.target; this.handlePointerBasedSelection(event); } diff --git a/packages/picker/package.json b/packages/picker/package.json index e9db1679c3..bc851ec32d 100644 --- a/packages/picker/package.json +++ b/packages/picker/package.json @@ -25,6 +25,18 @@ "default": "./src/index.js" }, "./package.json": "./package.json", + "./src/DesktopController.js": { + "development": "./src/DesktopController.dev.js", + "default": "./src/DesktopController.js" + }, + "./src/InteractionController.js": { + "development": "./src/InteractionController.dev.js", + "default": "./src/InteractionController.js" + }, + "./src/MobileController.js": { + "development": "./src/MobileController.dev.js", + "default": "./src/MobileController.js" + }, "./src/Picker.js": { "development": "./src/Picker.dev.js", "default": "./src/Picker.js" @@ -34,6 +46,10 @@ "default": "./src/index.js" }, "./src/picker.css.js": "./src/picker.css.js", + "./src/strategies.js": { + "development": "./src/strategies.dev.js", + "default": "./src/strategies.js" + }, "./sync/index.js": { "development": "./sync/index.dev.js", "default": "./sync/index.js" diff --git a/packages/picker/src/DesktopController.ts b/packages/picker/src/DesktopController.ts new file mode 100644 index 0000000000..e4368f47fe --- /dev/null +++ b/packages/picker/src/DesktopController.ts @@ -0,0 +1,87 @@ +/* +Copyright 2024 Adobe. All rights reserved. +This file is licensed to you under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. You may obtain a copy +of the License at http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under +the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +OF ANY KIND, either express or implied. See the License for the specific language +governing permissions and limitations under the License. +*/ + +import { + InteractionController, + InteractionTypes, +} from './InteractionController.js'; + +export class DesktopController extends InteractionController { + override type = InteractionTypes.desktop; + + public override handlePointerdown(event: PointerEvent): void { + if (event.button !== 0) { + return; + } + this.pointerdownState = this.open; + this.preventNextToggle = 'maybe'; + let cleanupAction = 0; + const cleanup = (): void => { + cancelAnimationFrame(cleanupAction); + cleanupAction = requestAnimationFrame(async () => { + document.removeEventListener('pointerup', cleanup); + document.removeEventListener('pointercancel', cleanup); + this.target.removeEventListener('click', cleanup); + requestAnimationFrame(() => { + // Complete cleanup on the second animation frame so that `click` can go first. + this.preventNextToggle = 'no'; + }); + }); + }; + // Ensure that however the pointer goes up we do `cleanup()`. + document.addEventListener('pointerup', cleanup); + document.addEventListener('pointercancel', cleanup); + this.target.addEventListener('click', cleanup); + this.handleActivate(); + } + + public override handleActivate(event?: Event): void { + if (this.enterKeydownOn && this.enterKeydownOn !== this.target) { + return; + } + if (this.preventNextToggle === 'yes') { + return; + } + if (event?.type === 'click' && this.open !== this.pointerdownState) { + // When activation comes from a `click` event ensure that the `pointerup` + // event didn't already toggle the Picker state before doing so. + return; + } + this.host.toggle(); + } + + override init(): void { + // Clean up listeners if they've already been bound + this.abortController?.abort(); + this.abortController = new AbortController(); + const { signal } = this.abortController; + this.target.addEventListener( + 'click', + (event: Event) => this.handleActivate(event), + { + signal, + } + ); + this.target.addEventListener( + 'pointerdown', + (event: PointerEvent) => this.handlePointerdown(event), + { signal } + ); + this.target.addEventListener( + 'focus', + (event: FocusEvent) => this.handleButtonFocus(event), + { + signal, + } + ); + } +} diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts new file mode 100644 index 0000000000..ef47028433 --- /dev/null +++ b/packages/picker/src/InteractionController.ts @@ -0,0 +1,189 @@ +/* +Copyright 2024 Adobe. All rights reserved. +This file is licensed to you under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. You may obtain a copy +of the License at http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under +the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +OF ANY KIND, either express or implied. See the License for the specific language +governing permissions and limitations under the License. +*/ + +import { + ReactiveController, + TemplateResult, +} from '@spectrum-web-components/base'; +import { AbstractOverlay } from '@spectrum-web-components/overlay/src/AbstractOverlay'; +import { Overlay } from '@spectrum-web-components/overlay/src/Overlay.js'; +import { PickerBase } from './Picker.js'; + +export enum InteractionTypes { + 'desktop', + 'mobile', +} + +export class InteractionController implements ReactiveController { + abortController!: AbortController; + + public preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; + public pointerdownState = false; + public enterKeydownOn: EventTarget | null = null; + + public container!: TemplateResult; + + get activelyOpening(): boolean { + return false; + } + + private _open = false; + + public get open(): boolean { + return this._open; + } + + /** + * Set `open` + */ + public set open(open: boolean) { + if (this._open === open) return; + this._open = open; + + if (this.overlay) { + this.host.open = open; + return; + } + + // When there is no Overlay and `open` is moving to `true`, lazily import/create + // an Overlay and apply that state to it. + customElements + .whenDefined('sp-overlay') + .then(async (): Promise => { + const { Overlay } = await import( + '@spectrum-web-components/overlay/src/Overlay.js' + ); + this.overlay = new Overlay(); + this.host.open = true; + this.host.requestUpdate(); + }); + import('@spectrum-web-components/overlay/sp-overlay.js'); + } + + private _overlay!: AbstractOverlay; + + public get overlay(): AbstractOverlay { + return this._overlay; + } + + public set overlay(overlay: AbstractOverlay | undefined) { + if (!overlay) return; + if (this.overlay === overlay) return; + this._overlay = overlay; + this.initOverlay(); + } + + type!: InteractionTypes; + + constructor( + public target: HTMLElement, + public host: PickerBase + ) { + this.target = target; + this.host = host; + this.host.addController(this); + this.init(); + } + + releaseDescription(): void {} + + protected handleBeforetoggle( + event: Event & { + target: Overlay; + newState: 'open' | 'closed'; + } + ): void { + if (event.composedPath()[0] !== event.target) { + return; + } + if (event.newState === 'closed') { + if (this.preventNextToggle === 'no') { + this.open = false; + } else if (!this.pointerdownState) { + // Prevent browser driven closure while opening the Picker + // and the expected event series has not completed. + this.overlay?.manuallyKeepOpen(); + } + } + if (!this.open) { + this.host.optionsMenu.updateSelectedItemIndex(); + this.host.optionsMenu.closeDescendentOverlays(); + } + } + + initOverlay(): void { + if (this.overlay) { + this.overlay.addEventListener('beforetoggle', (event: Event) => { + this.handleBeforetoggle( + event as Event & { + target: Overlay; + newState: 'open' | 'closed'; + } + ); + }); + + this.overlay.triggerElement = this.host as HTMLElement; + this.overlay.placement = this.host.isMobile.matches + ? undefined + : this.host.placement; + this.overlay.receivesFocus = 'true'; + this.overlay.willPreventClose = + this.preventNextToggle !== 'no' && this.open; + this.overlay.addEventListener( + 'slottable-request', + this.host.handleSlottableRequest + ); + } + } + + public handlePointerdown(_event: PointerEvent): void {} + + public handleButtonFocus(event: FocusEvent): void { + // When focus comes from a pointer event, and the related target is the Menu, + // we don't want to reopen the Menu. + if ( + this.preventNextToggle === 'maybe' && + event.relatedTarget === this.host.optionsMenu + ) { + this.preventNextToggle = 'yes'; + } + } + + public handleActivate(_event: Event): void {} + + /* c8 ignore next 3 */ + init(): void {} + + abort(): void { + this.releaseDescription(); + this.abortController?.abort(); + } + + hostConnected(): void { + this.init(); + } + + hostDisconnected(): void { + this.abortController?.abort(); + } + + public hostUpdated(): void { + if ( + this.overlay && + this.host.dependencyManager.loaded && + this.host.open !== this.overlay.open + ) { + this.overlay.willPreventClose = this.preventNextToggle !== 'no'; + this.overlay.open = this.host.open; + } + } +} diff --git a/packages/picker/src/MobileController.ts b/packages/picker/src/MobileController.ts new file mode 100644 index 0000000000..45d45fe50b --- /dev/null +++ b/packages/picker/src/MobileController.ts @@ -0,0 +1,52 @@ +/* +Copyright 2024 Adobe. All rights reserved. +This file is licensed to you under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. You may obtain a copy +of the License at http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software distributed under +the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +OF ANY KIND, either express or implied. See the License for the specific language +governing permissions and limitations under the License. +*/ + +import { + InteractionController, + InteractionTypes, +} from './InteractionController.js'; + +export class MobileController extends InteractionController { + override type = InteractionTypes.mobile; + + handleClick(): void { + if (this.preventNextToggle == 'no') { + this.open = !this.open; + } + this.preventNextToggle = 'no'; + } + + public override handlePointerdown(): void { + this.preventNextToggle = this.open ? 'yes' : 'no'; + } + + override init(): void { + // Clean up listeners if they've already been bound + this.abortController?.abort(); + this.abortController = new AbortController(); + const { signal } = this.abortController; + this.target.addEventListener('click', () => this.handleClick(), { + signal, + }); + this.target.addEventListener( + 'pointerdown', + () => this.handlePointerdown(), + { signal } + ); + this.target.addEventListener( + 'focus', + (event: FocusEvent) => this.handleButtonFocus(event), + { + signal, + } + ); + } +} diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index b09274c703..6e67e969f4 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -16,6 +16,7 @@ import { html, nothing, PropertyValues, + render, SizedMixin, TemplateResult, } from '@spectrum-web-components/base'; @@ -55,6 +56,10 @@ import { Overlay } from '@spectrum-web-components/overlay/src/Overlay.js'; import type { SlottableRequestEvent } from '@spectrum-web-components/overlay/src/slottable-request-event.js'; import type { FieldLabel } from '@spectrum-web-components/field-label'; +import { DesktopController } from './DesktopController.js'; +import { MobileController } from './MobileController.js'; +import { strategies } from './strategies.js'; + const chevronClass = { s: 'spectrum-UIIcon-ChevronDown75', m: 'spectrum-UIIcon-ChevronDown100', @@ -64,7 +69,9 @@ const chevronClass = { export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { - protected isMobile = new MatchMediaController(this, IS_MOBILE); + public isMobile = new MatchMediaController(this, IS_MOBILE); + + public strategy!: DesktopController | MobileController; @state() appliedLabel?: string; @@ -72,7 +79,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { @query('#button') public button!: HTMLButtonElement; - private dependencyManager = new DependencyManagerController(this); + public dependencyManager = new DependencyManagerController(this); private deprecatedMenu: Menu | null = null; @@ -115,7 +122,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } @query('sp-menu') - protected optionsMenu!: Menu; + public optionsMenu!: Menu; private _selfManageFocusElement = false; @@ -124,7 +131,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } @query('sp-overlay') - protected overlayElement!: Overlay; + public overlayElement!: Overlay; protected tooltipEl?: Tooltip; @@ -190,61 +197,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.focused = false; } - protected preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; - private pointerdownState = false; - - protected handleButtonPointerdown(event: PointerEvent): void { - if (event.button !== 0) { - return; - } - this.pointerdownState = this.open; - this.preventNextToggle = 'maybe'; - let cleanupAction = 0; - const cleanup = (): void => { - cancelAnimationFrame(cleanupAction); - cleanupAction = requestAnimationFrame(async () => { - document.removeEventListener('pointerup', cleanup); - document.removeEventListener('pointercancel', cleanup); - this.button.removeEventListener('click', cleanup); - requestAnimationFrame(() => { - // Complete cleanup on the second animation frame so that `click` can go first. - this.preventNextToggle = 'no'; - }); - }); - }; - // Ensure that however the pointer goes up we do `cleanup()`. - document.addEventListener('pointerup', cleanup); - document.addEventListener('pointercancel', cleanup); - this.button.addEventListener('click', cleanup); - this.handleActivate(); - } - - protected handleButtonFocus(event: FocusEvent): void { - // When focus comes from a pointer event, and the related target is the Menu, - // we don't want to reopen the Menu. - if ( - this.preventNextToggle === 'maybe' && - event.relatedTarget === this.optionsMenu - ) { - this.preventNextToggle = 'yes'; - } - } - - protected handleActivate(event?: Event): void { - if (this.enterKeydownOn && this.enterKeydownOn !== this.button) { - return; - } - if (this.preventNextToggle === 'yes') { - return; - } - if (event?.type === 'click' && this.open !== this.pointerdownState) { - // When activation comes from a `click` event ensure that the `pointerup` - // event didn't already toggle the Picker state before doing so. - return; - } - this.toggle(); - } - public override focus(options?: FocusOptions): void { super.focus(options); @@ -260,7 +212,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } public handleChange(event: Event): void { - this.preventNextToggle = 'no'; + if (this.strategy) { + this.strategy.preventNextToggle = 'no'; + } const target = event.target as Menu; const [selected] = target.selectedItems; event.stopPropagation(); @@ -270,9 +224,16 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { // Non-cancelable "change" events announce a selection with no value // change that should close the Picker element. this.open = false; + if (this.strategy) { + this.strategy.open = false; + } } } + public handleButtonFocus(event: FocusEvent): void { + this.strategy?.handleButtonFocus(event); + } + protected handleKeydown = (event: KeyboardEvent): void => { this.focused = true; if (event.code !== 'ArrowDown' && event.code !== 'ArrowUp') { @@ -287,8 +248,11 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { item: MenuItem, menuChangeEvent?: Event ): Promise { - // should always close when "setting" a value. this.open = false; + // should always close when "setting" a value + if (this.strategy) { + this.strategy.open = false; + } const oldSelectedItem = this.selectedItem; const oldValue = this.value; @@ -315,6 +279,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.selectedItem = oldSelectedItem; this.value = oldValue; this.open = true; + if (this.strategy) { + this.strategy.open = true; + } return; } else if (!this.selects) { // Unset the value if not carrying a selection @@ -339,6 +306,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { return; } this.open = typeof target !== 'undefined' ? target : !this.open; + if (this.strategy) { + this.strategy.open = this.open; + } if (this.open) { this._selfManageFocusElement = true; } else { @@ -350,7 +320,10 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { if (this.readonly) { return; } - this.open = false; + if (this.strategy) { + this.open = false; + this.strategy.open = false; + } } protected get containerStyles(): StyleInfo { @@ -389,34 +362,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { | undefined; } - protected handleBeforetoggle( - event: Event & { - target: Overlay; - newState: 'open' | 'closed'; - } - ): void { - if (event.composedPath()[0] !== event.target) { - return; - } - if (event.newState === 'closed') { - if (this.preventNextToggle === 'no') { - this.open = false; - } else if (!this.pointerdownState) { - // Prevent browser driven closure while opening the Picker - // and the expected event series has not completed. - this.overlayElement.manuallyKeepOpen(); - } - this._selfManageFocusElement = false; - } - if (!this.open) { - this.optionsMenu.updateSelectedItemIndex(); - this.optionsMenu.closeDescendentOverlays(); - } - } - - protected handleSlottableRequest = ( - _event: SlottableRequestEvent - ): void => {}; + public handleSlottableRequest = (_event: SlottableRequestEvent): void => {}; protected renderLabelContent(content: Node[]): TemplateResult | Node[] { if (this.value && this.selectedItem) { @@ -515,26 +461,14 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { }; protected renderOverlay(menu: TemplateResult): TemplateResult { + if (this.strategy?.overlay === undefined) { + return menu; + } const container = this.renderContainer(menu); - this.dependencyManager.add('sp-overlay'); - import('@spectrum-web-components/overlay/sp-overlay.js'); - return html` - - ${container} - - `; + render(container, this.strategy?.overlay as unknown as HTMLElement, { + host: this, + }); + return this.strategy?.overlay as unknown as TemplateResult; } protected get renderDescriptionSlot(): TemplateResult { @@ -570,9 +504,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { : undefined )} @blur=${this.handleButtonBlur} - @click=${this.handleActivate} - @pointerdown=${this.handleButtonPointerdown} - @focus=${this.handleButtonFocus} @keydown=${{ handleEvent: this.handleEnterKeydown, capture: true, @@ -593,10 +524,16 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.selects = 'single'; } if (changes.has('disabled') && this.disabled) { - this.open = false; + if (this.strategy) { + this.open = false; + this.strategy.open = false; + } } if (changes.has('pending') && this.pending) { - this.open = false; + if (this.strategy) { + this.open = false; + this.strategy.open = false; + } } if (changes.has('value')) { // MenuItems update a frame late for management, @@ -653,9 +590,17 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.button.addEventListener('keydown', this.handleKeydown); } + protected override updated(changes: PropertyValues): void { + super.updated(changes); + if (changes.has('open')) { + this.strategy.open = this.open; + } + } + protected override firstUpdated(changes: PropertyValues): void { super.firstUpdated(changes); this.bindButtonKeydownListener(); + this.bindEvents(); } protected get dismissHelper(): TemplateResult { @@ -730,6 +675,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.open || !!this.deprecatedMenu; if (this.hasRenderedOverlay) { + if (this.dependencyManager.loaded) { + this.dependencyManager.add('sp-overlay'); + } return this.renderOverlay(menu); } return menu; @@ -803,9 +751,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected override async getUpdateComplete(): Promise { const complete = (await super.getUpdateComplete()) as boolean; await this.selectionPromise; - if (this.overlayElement) { - await this.overlayElement.updateComplete; - } + // if (this.overlayElement) { + // await this.overlayElement.updateComplete; + // } return complete; } @@ -835,6 +783,15 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { ); }; + public bindEvents(): void { + this.strategy?.abort(); + if (this.isMobile.matches) { + this.strategy = new strategies['mobile'](this.button, this); + } else { + this.strategy = new strategies['desktop'](this.button, this); + } + } + public override connectedCallback(): void { super.connectedCallback(); this.recentlyConnected = this.hasUpdated; @@ -842,7 +799,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { public override disconnectedCallback(): void { this.close(); - + this.strategy?.releaseDescription(); super.disconnectedCallback(); } } diff --git a/packages/picker/src/strategies.ts b/packages/picker/src/strategies.ts new file mode 100644 index 0000000000..110d40786a --- /dev/null +++ b/packages/picker/src/strategies.ts @@ -0,0 +1,19 @@ +/* +Copyright 2024 Adobe. All rights reserved. +This file is licensed to you under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. You may obtain a copy +of the License at http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software distributed under +the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS +OF ANY KIND, either express or implied. See the License for the specific language +governing permissions and limitations under the License. +*/ + +import { DesktopController } from './DesktopController.js'; +import { MobileController } from './MobileController.js'; + +export const strategies = { + desktop: DesktopController, + mobile: MobileController, +}; diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts index dec60f2244..1e28ba2978 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -316,6 +316,7 @@ export function runPickerTests(): void { option2.innerHTML = 'Invert Selection'; await itemUpdated; await elementUpdated(el); + await aTimeout(150); expect(el.value).to.equal('option-2'); expect((el.button.textContent || '').trim()).to.include( 'Invert Selection' @@ -619,6 +620,8 @@ export function runPickerTests(): void { expect(el.value).to.equal(thirdItem.value); }); it('opens/closes multiple times', async () => { + await nextFrame(); + await nextFrame(); expect(el.open).to.be.false; const boundingRect = el.button.getBoundingClientRect(); let opened = oneEvent(el, 'sp-opened'); diff --git a/packages/picker/test/picker-responsive.test.ts b/packages/picker/test/picker-responsive.test.ts index 19792919e8..2a7062a7d3 100644 --- a/packages/picker/test/picker-responsive.test.ts +++ b/packages/picker/test/picker-responsive.test.ts @@ -22,33 +22,28 @@ import { oneEvent, } from '@open-wc/testing'; import { setViewport } from '@web/test-runner-commands'; +import { sendMouse } from '../../../test/plugins/browser.js'; describe('Picker, responsive', () => { let el: Picker; const pickerFixture = async (): Promise => { - const test = await fixture( - html` -
- - Where do you live? - - - Deselect - - Select Inverse - - Feather... - Select and Mask... - - Save Selection - Make Work Path - -
- ` - ); + const test = await fixture(html` +
+ Where do you live? + + Deselect + Select Inverse + Feather... + Select and Mask... + + Save Selection + Make Work Path + +
+ `); return test.querySelector('sp-picker') as Picker; }; @@ -59,7 +54,13 @@ describe('Picker, responsive', () => { await elementUpdated(el); }); - xit('is a Tray in mobile', async () => { + it('is a Tray in mobile', async () => { + /** + * This is a hack to set the `isMobile` property to true so that we can test the MobileController + */ + el.isMobile.matches = true; + el.bindEvents(); + /** * While we can set the view port, but not `(hover: none) and (pointer: coarse)` * which prevents us from testing this at unit time. Hopefully there will be @@ -71,7 +72,20 @@ describe('Picker, responsive', () => { await nextFrame(); const opened = oneEvent(el, 'sp-opened'); - el.open = true; + + const boundingRect = el.button.getBoundingClientRect(); + sendMouse({ + steps: [ + { + type: 'click', + position: [ + boundingRect.x + boundingRect.width / 2, + boundingRect.y + boundingRect.height / 2, + ], + }, + ], + }); + await opened; const tray = el.shadowRoot.querySelector('sp-tray'); diff --git a/packages/split-button/src/SplitButton.ts b/packages/split-button/src/SplitButton.ts index 82e39c55bd..c61735c2a8 100644 --- a/packages/split-button/src/SplitButton.ts +++ b/packages/split-button/src/SplitButton.ts @@ -72,6 +72,10 @@ export class SplitButton extends SizedMixin(PickerBase) { protected override listRole: 'listbox' | 'menu' = 'menu'; protected override itemRole = 'menuitem'; + // PickerBase has an interactionStrategy that needs the trigger button from the split button + @query('.trigger') + public override button!: HTMLButtonElement; + public override get focusElement(): HTMLElement { if (this.open) { return this.optionsMenu; @@ -154,8 +158,6 @@ export class SplitButton extends SizedMixin(PickerBase) { aria-controls=${ifDefined(this.open ? 'menu' : undefined)} class="button trigger ${this.variant}" @blur=${this.handleButtonBlur} - @click=${this.handleActivate} - @pointerdown=${this.handleButtonPointerdown} @focus=${this.handleButtonFocus} @keydown=${{ handleEvent: this.handleEnterKeydown, diff --git a/packages/split-button/test/index.ts b/packages/split-button/test/index.ts index 5c4f407ed7..fa2299bb9c 100644 --- a/packages/split-button/test/index.ts +++ b/packages/split-button/test/index.ts @@ -17,7 +17,6 @@ import { nextFrame, oneEvent, } from '@open-wc/testing'; -import { sendKeys } from '@web/test-runner-commands'; import { html, TemplateResult } from '@spectrum-web-components/base'; import { spy } from 'sinon'; import { a11ySnapshot, findAccessibilityNode } from '@web/test-runner-commands'; @@ -33,7 +32,6 @@ import type { MenuItem } from '@spectrum-web-components/menu'; import type { SplitButton } from '@spectrum-web-components/split-button'; import { sendMouse } from '../../../test/plugins/browser.js'; import { fixture } from '../../../test/testing-helpers.js'; -import { Button } from '@spectrum-web-components/button'; export function runSplitButtonTests( wrapInDiv: (storyArgument: TemplateResult) => TemplateResult, @@ -290,66 +288,6 @@ export function runSplitButtonTests( expect(el.open).to.be.false; }); - it('[type="field"] opens and selects in a single pointer button interaction', async () => { - const test = await fixture( - wrapInDiv( - field({ - ...fieldDefaults.args, - ...field.args, - }) - ) - ); - const el = test.querySelector('sp-split-button') as SplitButton; - await elementUpdated(el); - await nextFrame(); - await nextFrame(); - - const thirdItem = el.querySelector( - 'sp-menu-item:nth-of-type(3)' - ) as MenuItem; - const trigger = el.shadowRoot.querySelector('.trigger') as Button; - const boundingRect = trigger.getBoundingClientRect(); - - expect(el.value).to.not.equal(thirdItem.value); - const opened = oneEvent(el, 'sp-opened'); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - boundingRect.x + boundingRect.width / 2, - boundingRect.y + boundingRect.height / 2, - ], - }, - { - type: 'down', - }, - ], - }); - await opened; - - const thirdItemRect = thirdItem.getBoundingClientRect(); - const closed = oneEvent(el, 'sp-closed'); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - thirdItemRect.x + thirdItemRect.width / 2, - thirdItemRect.y + thirdItemRect.height / 2, - ], - }, - { - type: 'up', - }, - ], - }); - await closed; - - expect(el.open).to.be.false; - expect(el.value).to.equal(thirdItem.value); - }); - it('[type="more"] toggles open/close multiple time', async () => { const test = await fixture( wrapInDiv(more({ ...moreDefaults.args, ...more.args })) @@ -397,68 +335,6 @@ export function runSplitButtonTests( expect(trigger).not.to.have.attribute('aria-controls'); }); - it('[type="more"] opens and selects in a single pointer button interaction', async () => { - const thirdItemSpy = spy(); - const test = await fixture( - wrapInDiv( - more({ - ...moreDefaults.args, - ...more.args, - thirdItemHandler: (): void => thirdItemSpy(), - }) - ) - ); - const el = test.querySelector('sp-split-button') as SplitButton; - await elementUpdated(el); - await nextFrame(); - await nextFrame(); - - const thirdItem = el.querySelector( - 'sp-menu-item:nth-of-type(3)' - ) as MenuItem; - const trigger = el.shadowRoot.querySelector('.trigger') as Button; - const boundingRect = trigger.getBoundingClientRect(); - - expect(el.value).to.not.equal(thirdItem.value); - const opened = oneEvent(el, 'sp-opened'); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - boundingRect.x + boundingRect.width / 2, - boundingRect.y + boundingRect.height / 2, - ], - }, - { - type: 'down', - }, - ], - }); - await opened; - - const thirdItemRect = thirdItem.getBoundingClientRect(); - const closed = oneEvent(el, 'sp-closed'); - await sendMouse({ - steps: [ - { - type: 'move', - position: [ - thirdItemRect.x + thirdItemRect.width / 2, - thirdItemRect.y + thirdItemRect.height / 2, - ], - }, - { - type: 'up', - }, - ], - }); - await closed; - - expect(el.open).to.be.false; - expect(thirdItemSpy.callCount).to.equal(1); - }); - it('[type="more"] opens, then closes, on subsequent clicks', async () => { const test = await fixture( wrapInDiv(more({ ...moreDefaults.args, ...more.args })) @@ -626,7 +502,9 @@ export function runSplitButtonTests( const item1 = el.querySelector('sp-menu-item:nth-child(1)') as MenuItem; const item2 = el.querySelector('sp-menu-item:nth-child(2)') as MenuItem; const item3 = el.querySelector('sp-menu-item:nth-child(3)') as MenuItem; - const main = el.button; + const main = el.shadowRoot?.querySelector( + '#button' + ) as HTMLButtonElement; main.click(); @@ -663,20 +541,11 @@ export function runSplitButtonTests( expect(thirdItemSpy.callCount, 'third callCount').to.equal(2); expect(thirdItemSpy.calledTwice, 'third calledTwice').to.be.true; - await sendKeys({ - press: 'Tab', - }); opened = oneEvent(el, 'sp-opened'); - await sendKeys({ - press: 'k', - }); - sendKeys({ - press: 'ArrowDown', - }); + el.click(); await opened; - await elementUpdated(el); - expect(el.open, 'reopened').to.be.true; + expect(el.open, 'reopen').to.be.true; closed = oneEvent(el, 'sp-closed'); item2.click(); @@ -739,7 +608,7 @@ export function runSplitButtonTests( const item2 = el.querySelector('sp-menu-item:nth-child(2)') as MenuItem; const item3 = el.querySelector('sp-menu-item:nth-child(3)') as MenuItem; - const main = el.shadowRoot.querySelector( + const main = el.shadowRoot?.querySelector( '#button' ) as HTMLButtonElement; main.click(); diff --git a/test/plugins/send-mouse-plugin.ts b/test/plugins/send-mouse-plugin.ts index 6c177f57c0..6587bae2af 100644 --- a/test/plugins/send-mouse-plugin.ts +++ b/test/plugins/send-mouse-plugin.ts @@ -14,7 +14,7 @@ import type { Page } from 'playwright'; export type Step = { type: 'move' | 'down' | 'up' | 'click' | 'wheel'; position?: [number, number]; - options?: { button?: 'left' | 'right' | 'middle' }; + options?: { button?: 'left' | 'right' | 'middle'; delay?: number }; }; export function sendMousePlugin() { @@ -38,6 +38,9 @@ export function sendMousePlugin() { const page = session.browser.getPage(session.id); for (const step of payload.steps) { step.options = step.options || {}; + // adding a delay to make sure the consecutive mouse events are not too fast + // picker open/close tests were failing without this + step.options.delay = 1; if (step.position) { await page.mouse[step.type]( Math.round(step.position[0]),