From b9ab3213edc37bae2051051e2778de063f2d33e1 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Fri, 3 May 2024 15:33:29 +0530 Subject: [PATCH 01/18] chore: testing a click controller strategy for picker component --- packages/picker/package.json | 8 +++ packages/picker/src/ClickController.ts | 54 ++++++++++++++++ packages/picker/src/InteractionController.ts | 66 ++++++++++++++++++++ packages/picker/src/Picker.ts | 11 +++- 4 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 packages/picker/src/ClickController.ts create mode 100644 packages/picker/src/InteractionController.ts diff --git a/packages/picker/package.json b/packages/picker/package.json index a03cfac3c7..a9fac423d7 100644 --- a/packages/picker/package.json +++ b/packages/picker/package.json @@ -25,6 +25,14 @@ "default": "./src/index.js" }, "./package.json": "./package.json", + "./src/ClickController.js": { + "development": "./src/ClickController.dev.js", + "default": "./src/ClickController.js" + }, + "./src/InteractionController.js": { + "development": "./src/InteractionController.dev.js", + "default": "./src/InteractionController.js" + }, "./src/Picker.js": { "development": "./src/Picker.dev.js", "default": "./src/Picker.js" diff --git a/packages/picker/src/ClickController.ts b/packages/picker/src/ClickController.ts new file mode 100644 index 0000000000..fbcf3c91ab --- /dev/null +++ b/packages/picker/src/ClickController.ts @@ -0,0 +1,54 @@ +/* +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 ClickController extends InteractionController { + override type = InteractionTypes.click; + + /** + * An overlay with a `click` interaction should not close on click `triggerElement`. + * When a click is initiated (`pointerdown`), apply `preventNextToggle` when the + * overlay is `open` to prevent from toggling the overlay when the click event + * propagates later in the interaction. + */ + private preventNextToggle = false; + + handleClick(): void { + if (!this.preventNextToggle) { + this.host.open = !this.host.open; + } + this.preventNextToggle = false; + } + + handlePointerdown(): void { + this.preventNextToggle = this.host.open; + } + + 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 } + ); + } +} diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts new file mode 100644 index 0000000000..fe9bcfb158 --- /dev/null +++ b/packages/picker/src/InteractionController.ts @@ -0,0 +1,66 @@ +/* +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 type { ReactiveController } from 'lit'; +import type { PickerBase } from './Picker.js'; + +export enum InteractionTypes { + 'click', +} + +export class InteractionController implements ReactiveController { + abortController!: AbortController; + + get activelyOpening(): boolean { + return false; + } + + type!: InteractionTypes; + + constructor( + public host: PickerBase, + public target: HTMLElement, + private isPersistent = false + ) { + this.host.addController(this); + this.prepareDescription(this.target); + if (this.isPersistent) { + this.init(); + } + } + + prepareDescription(_: HTMLElement): void {} + + releaseDescription(): void {} + + shouldCompleteOpen(): void {} + + /* c8 ignore next 3 */ + init(): void { + // Abstract init() method. + } + + abort(): void { + this.releaseDescription(); + this.abortController?.abort(); + } + + hostConnected(): void { + this.init(); + } + + hostDisconnected(): void { + if (!this.isPersistent) { + this.abort(); + } + } +} diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index e294fc8d92..91779d782c 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -55,6 +55,8 @@ 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 { ClickController } from './ClickController.js'; + const chevronClass = { s: 'spectrum-UIIcon-ChevronDown75', m: 'spectrum-UIIcon-ChevronDown100', @@ -66,6 +68,8 @@ export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected isMobile = new MatchMediaController(this, IS_MOBILE); + public strategy?: ClickController; + @state() appliedLabel?: string; @@ -558,8 +562,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, @@ -644,6 +646,11 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected override firstUpdated(changes: PropertyValues): void { super.firstUpdated(changes); this.bindButtonKeydownListener(); + + this.strategy?.abort(); + this.strategy = undefined; + + this.strategy = new ClickController(this, this.button); } protected get dismissHelper(): TemplateResult { From f350d1ad59b716caed2fcc2ce5576a27c55d47c3 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 14 May 2024 13:21:21 +0530 Subject: [PATCH 02/18] chore: testing a new interaction strategy for picker --- packages/picker/package.json | 14 ++- packages/picker/src/DesktopController.ts | 114 ++++++++++++++++++ packages/picker/src/InteractionController.ts | 42 ++++--- ...ClickController.ts => MobileController.ts} | 3 +- packages/picker/src/Picker.ts | 83 +++---------- packages/picker/src/strategies.ts | 19 +++ 6 files changed, 192 insertions(+), 83 deletions(-) create mode 100644 packages/picker/src/DesktopController.ts rename packages/picker/src/{ClickController.ts => MobileController.ts} (96%) create mode 100644 packages/picker/src/strategies.ts diff --git a/packages/picker/package.json b/packages/picker/package.json index a9fac423d7..580557d1be 100644 --- a/packages/picker/package.json +++ b/packages/picker/package.json @@ -25,14 +25,18 @@ "default": "./src/index.js" }, "./package.json": "./package.json", - "./src/ClickController.js": { - "development": "./src/ClickController.dev.js", - "default": "./src/ClickController.js" + "./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" @@ -42,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..0e35a575af --- /dev/null +++ b/packages/picker/src/DesktopController.ts @@ -0,0 +1,114 @@ +/* +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.click; + + /** + * An overlay with a `click` interaction should not close on click `triggerElement`. + * When a click is initiated (`pointerdown`), apply `preventNextToggle` when the + * overlay is `open` to prevent from toggling the overlay when the click event + * propagates later in the interaction. + */ + // private preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; + // private pointerdownState = false; + + protected handlePointerdown(event: PointerEvent): void { + if (event.button !== 0) { + return; + } + this.host.pointerdownState = this.open; + // this.pointerdownState = this.open; + // this.preventNextToggle = 'maybe'; + this.host.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'; + this.host.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(); + } + + 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.host.preventNextToggle === 'maybe' && + event.relatedTarget === this.host.optionsMenu + ) { + // this.preventNextToggle = 'yes'; + this.host.preventNextToggle = 'yes'; + } + } + + protected handleActivate(event?: Event): void { + if (this.enterKeydownOn && this.enterKeydownOn !== this.button) { + return; + } + if (this.host.preventNextToggle === 'yes') { + return; + } + if ( + event?.type === 'click' && + this.open !== this.host.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: PointerEvent) => 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 index fe9bcfb158..c6658a3c10 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -10,11 +10,13 @@ OF ANY KIND, either express or implied. See the License for the specific languag governing permissions and limitations under the License. */ -import type { ReactiveController } from 'lit'; -import type { PickerBase } from './Picker.js'; +import type { ReactiveController } from '@spectrum-web-components/base'; +import { AbstractOverlay } from '@spectrum-web-components/overlay/src/AbstractOverlay'; +import { PickerBase } from './PickerBase.js'; export enum InteractionTypes { - 'click', + 'desktop', + 'mobile', } export class InteractionController implements ReactiveController { @@ -24,26 +26,38 @@ export class InteractionController implements ReactiveController { return false; } + private handleOverlayReady?: (overlay: AbstractOverlay) => void; + + public get open(): boolean { + return this.host.open; + } + + /** + * Set `open` + */ + public set open(open: boolean) { + this.host.open = open; + } + + toggle(target?: boolean): void { + this.host.toggle(target); + } + type!: InteractionTypes; constructor( - public host: PickerBase, public target: HTMLElement, - private isPersistent = false + public overlay: AbstractOverlay | undefined, + public host: PickerBase ) { - this.host.addController(this); - this.prepareDescription(this.target); - if (this.isPersistent) { - this.init(); - } + this.target = target; + this.overlay = overlay; + this.host = host; + this.init(); } - prepareDescription(_: HTMLElement): void {} - releaseDescription(): void {} - shouldCompleteOpen(): void {} - /* c8 ignore next 3 */ init(): void { // Abstract init() method. diff --git a/packages/picker/src/ClickController.ts b/packages/picker/src/MobileController.ts similarity index 96% rename from packages/picker/src/ClickController.ts rename to packages/picker/src/MobileController.ts index fbcf3c91ab..d5a93ec385 100644 --- a/packages/picker/src/ClickController.ts +++ b/packages/picker/src/MobileController.ts @@ -3,7 +3,6 @@ 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 @@ -15,7 +14,7 @@ import { InteractionTypes, } from './InteractionController.js'; -export class ClickController extends InteractionController { +export class MobileController extends InteractionController { override type = InteractionTypes.click; /** diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 91779d782c..50fadb1819 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -55,7 +55,9 @@ 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 { ClickController } from './ClickController.js'; +import { DesktopController } from './DesktopController.js'; +import { MobileController } from './MobileController.js'; +import { strategies } from './strategies.js'; const chevronClass = { s: 'spectrum-UIIcon-ChevronDown75', @@ -68,7 +70,7 @@ export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected isMobile = new MatchMediaController(this, IS_MOBILE); - public strategy?: ClickController; + public strategy?: DesktopController | MobileController; @state() appliedLabel?: string; @@ -122,7 +124,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected optionsMenu!: Menu; @query('sp-overlay') - protected overlayElement!: Overlay; + public overlayElement!: Overlay; protected tooltipEl?: Tooltip; @@ -188,60 +190,8 @@ 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 preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; + public pointerdownState = false; public override focus(options?: FocusOptions): void { super.focus(options); @@ -562,7 +512,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { : undefined )} @blur=${this.handleButtonBlur} - @focus=${this.handleButtonFocus} @keydown=${{ handleEvent: this.handleEnterKeydown, capture: true, @@ -646,11 +595,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected override firstUpdated(changes: PropertyValues): void { super.firstUpdated(changes); this.bindButtonKeydownListener(); - - this.strategy?.abort(); - this.strategy = undefined; - - this.strategy = new ClickController(this, this.button); + this.bindEvents(); } protected get dismissHelper(): TemplateResult { @@ -830,6 +775,16 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { ); }; + protected bindEvents(): void { + this.strategy?.abort(); + this.strategy = undefined; + this.strategy = new strategies['desktop']( + this.button, + this.overlayElement, + this + ); + } + public override connectedCallback(): void { super.connectedCallback(); this.recentlyConnected = this.hasUpdated; @@ -837,7 +792,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, +}; From 1b836111ad79ec258c9c62905e2dd312ef78c1a1 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Wed, 22 May 2024 19:32:26 +0530 Subject: [PATCH 03/18] fix: separated interaction strategy for picker --- packages/picker/src/DesktopController.ts | 48 ++----- packages/picker/src/InteractionController.ts | 128 ++++++++++++++++--- packages/picker/src/MobileController.ts | 27 ++-- packages/picker/src/Picker.ts | 118 ++++++++--------- 4 files changed, 190 insertions(+), 131 deletions(-) diff --git a/packages/picker/src/DesktopController.ts b/packages/picker/src/DesktopController.ts index 0e35a575af..48b8b4bc23 100644 --- a/packages/picker/src/DesktopController.ts +++ b/packages/picker/src/DesktopController.ts @@ -16,25 +16,14 @@ import { } from './InteractionController.js'; export class DesktopController extends InteractionController { - override type = InteractionTypes.click; + override type = InteractionTypes.desktop; - /** - * An overlay with a `click` interaction should not close on click `triggerElement`. - * When a click is initiated (`pointerdown`), apply `preventNextToggle` when the - * overlay is `open` to prevent from toggling the overlay when the click event - * propagates later in the interaction. - */ - // private preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; - // private pointerdownState = false; - - protected handlePointerdown(event: PointerEvent): void { + public override handlePointerdown(event: PointerEvent): void { if (event.button !== 0) { return; } - this.host.pointerdownState = this.open; - // this.pointerdownState = this.open; - // this.preventNextToggle = 'maybe'; - this.host.preventNextToggle = 'maybe'; + this.pointerdownState = this.open; + this.preventNextToggle = 'maybe'; let cleanupAction = 0; const cleanup = (): void => { cancelAnimationFrame(cleanupAction); @@ -45,7 +34,7 @@ export class DesktopController extends InteractionController { requestAnimationFrame(() => { // Complete cleanup on the second animation frame so that `click` can go first. // this.preventNextToggle = 'no'; - this.host.preventNextToggle = 'no'; + this.preventNextToggle = 'no'; }); }); }; @@ -56,34 +45,19 @@ export class DesktopController extends InteractionController { 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.host.preventNextToggle === 'maybe' && - event.relatedTarget === this.host.optionsMenu - ) { - // this.preventNextToggle = 'yes'; - this.host.preventNextToggle = 'yes'; - } - } - - protected handleActivate(event?: Event): void { - if (this.enterKeydownOn && this.enterKeydownOn !== this.button) { + public override handleActivate(event?: Event): void { + if (this.enterKeydownOn && this.enterKeydownOn !== this.target) { return; } - if (this.host.preventNextToggle === 'yes') { + if (this.preventNextToggle === 'yes') { return; } - if ( - event?.type === 'click' && - this.open !== this.host.pointerdownState - ) { + 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(); + this.open = !this.open; } override init(): void { @@ -93,7 +67,7 @@ export class DesktopController extends InteractionController { const { signal } = this.abortController; this.target.addEventListener( 'click', - (event: PointerEvent) => this.handleActivate(event), + (event: Event) => this.handleActivate(event), { signal, } diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index c6658a3c10..347471a9bf 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -10,9 +10,13 @@ OF ANY KIND, either express or implied. See the License for the specific languag governing permissions and limitations under the License. */ -import type { ReactiveController } from '@spectrum-web-components/base'; +import { + ReactiveController, + TemplateResult, +} from '@spectrum-web-components/base'; import { AbstractOverlay } from '@spectrum-web-components/overlay/src/AbstractOverlay'; -import { PickerBase } from './PickerBase.js'; +import { Overlay } from '@spectrum-web-components/overlay/src/Overlay.js'; +import { PickerBase } from './Picker.js'; export enum InteractionTypes { 'desktop', @@ -22,47 +26,143 @@ export enum InteractionTypes { 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 handleOverlayReady?: (overlay: AbstractOverlay) => void; + private _open = false; public get open(): boolean { - return this.host.open; + return this._open; } /** * Set `open` */ public set open(open: boolean) { - this.host.open = open; + this._open = open; + if (this.overlay) { + // If there already is an Overlay, apply the value of `open` directly. + this.overlay.open = open; + this.host.open = open; + return; + } + if (!open) { + this.host.open = open; + // When `open` moves to `false` and there is not yet an Overlay, + // assume that no Overlay and a closed Overlay are the same and return early. + 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.overlay.open = true; + this.host.open = true; + }); + import('@spectrum-web-components/overlay/sp-overlay.js'); + } + + private _overlay!: AbstractOverlay; + + public get overlay(): AbstractOverlay { + return this._overlay; } - toggle(target?: boolean): void { - this.host.toggle(target); + 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 overlay: AbstractOverlay | undefined, public host: PickerBase ) { this.target = target; - this.overlay = overlay; this.host = host; this.init(); } releaseDescription(): void {} - /* c8 ignore next 3 */ - init(): void { - // Abstract init() method. + 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; + } + } + + 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(); @@ -73,8 +173,6 @@ export class InteractionController implements ReactiveController { } hostDisconnected(): void { - if (!this.isPersistent) { - this.abort(); - } + this.abortController?.abort(); } } diff --git a/packages/picker/src/MobileController.ts b/packages/picker/src/MobileController.ts index d5a93ec385..45d45fe50b 100644 --- a/packages/picker/src/MobileController.ts +++ b/packages/picker/src/MobileController.ts @@ -15,25 +15,17 @@ import { } from './InteractionController.js'; export class MobileController extends InteractionController { - override type = InteractionTypes.click; - - /** - * An overlay with a `click` interaction should not close on click `triggerElement`. - * When a click is initiated (`pointerdown`), apply `preventNextToggle` when the - * overlay is `open` to prevent from toggling the overlay when the click event - * propagates later in the interaction. - */ - private preventNextToggle = false; + override type = InteractionTypes.mobile; handleClick(): void { - if (!this.preventNextToggle) { - this.host.open = !this.host.open; + if (this.preventNextToggle == 'no') { + this.open = !this.open; } - this.preventNextToggle = false; + this.preventNextToggle = 'no'; } - handlePointerdown(): void { - this.preventNextToggle = this.host.open; + public override handlePointerdown(): void { + this.preventNextToggle = this.open ? 'yes' : 'no'; } override init(): void { @@ -49,5 +41,12 @@ export class MobileController extends InteractionController { () => 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 50fadb1819..d9427bdd09 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'; @@ -68,7 +69,7 @@ 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; @@ -121,7 +122,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } @query('sp-menu') - protected optionsMenu!: Menu; + public optionsMenu!: Menu; @query('sp-overlay') public overlayElement!: Overlay; @@ -190,9 +191,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.focused = false; } - public preventNextToggle: 'no' | 'maybe' | 'yes' = 'no'; - public pointerdownState = false; - public override focus(options?: FocusOptions): void { super.focus(options); @@ -208,7 +206,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(); @@ -217,10 +217,24 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } else { // 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 handleActivate(event: Event): void { + this.strategy?.handleActivate(event); + } + + public handleButtonPointerdown(event: PointerEvent): void { + this.strategy?.handlePointerdown(event); + } + + public handleButtonFocus(event: FocusEvent): void { + this.strategy?.handleButtonFocus(event); + } + protected handleKeydown = (event: KeyboardEvent): void => { this.focused = true; if (event.code !== 'ArrowDown' && event.code !== 'ArrowUp') { @@ -235,8 +249,10 @@ 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; @@ -262,7 +278,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 @@ -286,14 +304,19 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { if (this.readonly || this.pending) { return; } - this.open = typeof target !== 'undefined' ? target : !this.open; + if (this.strategy) { + this.strategy.open = + typeof target !== 'undefined' ? target : !this.open; + } } public close(): void { if (this.readonly) { return; } - this.open = false; + if (this.strategy) { + this.strategy.open = false; + } } protected get containerStyles(): StyleInfo { @@ -332,30 +355,6 @@ 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(); - } - } - if (!this.open) { - this.optionsMenu.updateSelectedItemIndex(); - this.optionsMenu.closeDescendentOverlays(); - } - } - protected handleSlottableRequest = ( _event: SlottableRequestEvent ): void => {}; @@ -458,25 +457,10 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected renderOverlay(menu: TemplateResult): TemplateResult { 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 { @@ -532,10 +516,14 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { this.selects = 'single'; } if (changes.has('disabled') && this.disabled) { - this.open = false; + if (this.strategy) { + this.strategy.open = false; + } } if (changes.has('pending') && this.pending) { - this.open = false; + if (this.strategy) { + this.strategy.open = false; + } } if (changes.has('value')) { // MenuItems update a frame late for management, @@ -743,9 +731,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; } @@ -778,11 +766,11 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected bindEvents(): void { this.strategy?.abort(); this.strategy = undefined; - this.strategy = new strategies['desktop']( - this.button, - this.overlayElement, - this - ); + 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 { From 1290ba77f7bb7e8eb0101dd30cb65a1bf6bb0319 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 28 May 2024 11:07:59 +0530 Subject: [PATCH 04/18] chore: updated the strategy to include the dependency controller --- packages/picker/src/InteractionController.ts | 21 +++++++++++++-- packages/picker/src/Picker.ts | 28 ++++++++++++++++---- packages/picker/test/index.ts | 3 +++ 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index 347471a9bf..b9e21a5a2e 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -46,10 +46,13 @@ export class InteractionController implements ReactiveController { * Set `open` */ public set open(open: boolean) { + if (this._open === open) return; this._open = open; if (this.overlay) { // If there already is an Overlay, apply the value of `open` directly. - this.overlay.open = open; + if (this.host.dependencyManager.loaded) { + this.overlay.open = open; + } this.host.open = open; return; } @@ -68,7 +71,10 @@ export class InteractionController implements ReactiveController { '@spectrum-web-components/overlay/src/Overlay.js' ); this.overlay = new Overlay(); - this.overlay.open = true; + this.host.requestUpdate(); + if (this.host.dependencyManager.loaded) { + this.overlay.open = open; + } this.host.open = true; }); import('@spectrum-web-components/overlay/sp-overlay.js'); @@ -95,6 +101,7 @@ export class InteractionController implements ReactiveController { ) { this.target = target; this.host = host; + this.host.addController(this); this.init(); } @@ -175,4 +182,14 @@ export class InteractionController implements ReactiveController { hostDisconnected(): void { this.abortController?.abort(); } + + public hostUpdated(): void { + if ( + this.overlay && + this.host.dependencyManager.loaded && + this.host.open + ) { + this.overlay.open = true; + } + } } diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index d9427bdd09..f26005c07b 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -71,7 +71,7 @@ export const DESCRIPTION_ID = 'option-picker'; export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { public isMobile = new MatchMediaController(this, IS_MOBILE); - public strategy?: DesktopController | MobileController; + public strategy!: DesktopController | MobileController; @state() appliedLabel?: string; @@ -79,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; @@ -217,6 +217,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } else { // 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; } @@ -249,6 +250,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { item: MenuItem, menuChangeEvent?: Event ): Promise { + this.open = false; // should always close when "setting" a value if (this.strategy) { this.strategy.open = false; @@ -278,6 +280,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } this.selectedItem = oldSelectedItem; this.value = oldValue; + this.open = true; if (this.strategy) { this.strategy.open = true; } @@ -304,9 +307,9 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { if (this.readonly || this.pending) { return; } + this.open = typeof target !== 'undefined' ? target : !this.open; if (this.strategy) { - this.strategy.open = - typeof target !== 'undefined' ? target : !this.open; + this.strategy.open = this.open; } } @@ -315,6 +318,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { return; } if (this.strategy) { + this.open = false; this.strategy.open = false; } } @@ -456,6 +460,9 @@ 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); render(container, this.strategy?.overlay as unknown as HTMLElement, { host: this, @@ -517,11 +524,13 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } if (changes.has('disabled') && this.disabled) { if (this.strategy) { + this.open = false; this.strategy.open = false; } } if (changes.has('pending') && this.pending) { if (this.strategy) { + this.open = false; this.strategy.open = false; } } @@ -580,6 +589,13 @@ 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(); @@ -658,6 +674,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; @@ -765,7 +784,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { protected bindEvents(): void { this.strategy?.abort(); - this.strategy = undefined; if (this.isMobile.matches) { this.strategy = new strategies['mobile'](this.button, this); } else { 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'); From f3e64e949e3e5c08bfd69f41680bc4d2bb5a1a22 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 25 Jun 2024 10:33:58 +0530 Subject: [PATCH 05/18] chore: updated open condition in interactioncontroller --- packages/picker/src/InteractionController.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index b9e21a5a2e..c12ff2dc88 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -51,6 +51,7 @@ export class InteractionController implements ReactiveController { if (this.overlay) { // If there already is an Overlay, apply the value of `open` directly. if (this.host.dependencyManager.loaded) { + this.overlay.willPreventClose = this.preventNextToggle !== 'no'; this.overlay.open = open; } this.host.open = open; From f61dc962761eeb36e914089a3eb0017fc74ea496 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 25 Jun 2024 11:51:41 +0530 Subject: [PATCH 06/18] chore(picker): removed double event handling from action menu --- packages/action-menu/src/ActionMenu.ts | 2 - packages/action-menu/test/index.ts | 210 +++++++++++------------ packages/picker/src/Picker.ts | 8 - packages/split-button/src/SplitButton.ts | 2 - 4 files changed, 97 insertions(+), 125 deletions(-) diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index d8ce399ebf..790a4518ba 100644 --- a/packages/action-menu/src/ActionMenu.ts +++ b/packages/action-menu/src/ActionMenu.ts @@ -112,8 +112,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/picker/src/Picker.ts b/packages/picker/src/Picker.ts index f26005c07b..8d48360509 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -224,14 +224,6 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { } } - public handleActivate(event: Event): void { - this.strategy?.handleActivate(event); - } - - public handleButtonPointerdown(event: PointerEvent): void { - this.strategy?.handlePointerdown(event); - } - public handleButtonFocus(event: FocusEvent): void { this.strategy?.handleButtonFocus(event); } diff --git a/packages/split-button/src/SplitButton.ts b/packages/split-button/src/SplitButton.ts index 81f77bdc59..5fc1869675 100644 --- a/packages/split-button/src/SplitButton.ts +++ b/packages/split-button/src/SplitButton.ts @@ -154,8 +154,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, From b163d0cc0e20a51c3deb911238bb4a6313ff6394 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Mon, 8 Jul 2024 14:04:56 +0530 Subject: [PATCH 07/18] chore: added slottable-request handling in interactionController --- packages/action-menu/src/ActionMenu.ts | 2 +- packages/picker/src/InteractionController.ts | 4 ++++ packages/picker/src/Picker.ts | 4 +--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/action-menu/src/ActionMenu.ts b/packages/action-menu/src/ActionMenu.ts index 790a4518ba..63fba89f3a 100644 --- a/packages/action-menu/src/ActionMenu.ts +++ b/packages/action-menu/src/ActionMenu.ts @@ -63,7 +63,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)); diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index c12ff2dc88..2d8cfdd61c 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -150,6 +150,10 @@ export class InteractionController implements ReactiveController { this.overlay.receivesFocus = 'true'; this.overlay.willPreventClose = this.preventNextToggle !== 'no' && this.open; + this.overlay.addEventListener( + 'slottable-request', + this.host.handleSlottableRequest + ); } } diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 8d48360509..3d16496743 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -351,9 +351,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { | undefined; } - protected handleSlottableRequest = ( - _event: SlottableRequestEvent - ): void => {}; + public handleSlottableRequest = (_event: SlottableRequestEvent): void => {}; protected renderLabelContent(content: Node[]): TemplateResult | Node[] { if (this.value && this.selectedItem) { From 4fee1059261e65cd9ec328e2149f5e32f11a8fff Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Mon, 8 Jul 2024 16:59:40 +0530 Subject: [PATCH 08/18] chore: change trigger button for split-button --- packages/split-button/src/SplitButton.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/split-button/src/SplitButton.ts b/packages/split-button/src/SplitButton.ts index 5fc1869675..65449fe350 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; From 88a52d80c1a9c4895edf9ba42f82f3a5c6d4b18a Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 9 Jul 2024 11:46:48 +0530 Subject: [PATCH 09/18] chore: updated split-button test --- packages/split-button/test/index.ts | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/packages/split-button/test/index.ts b/packages/split-button/test/index.ts index 5c4f407ed7..82b5f20f49 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'; @@ -626,7 +625,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 +664,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 +731,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(); From e320657eb8ca61bab7dde25a22275ef2316dfd26 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 16 Jul 2024 11:17:03 +0530 Subject: [PATCH 10/18] chore: fixed conflicts after merging main --- packages/picker/src/Picker.ts | 29 +---------------------------- 1 file changed, 1 insertion(+), 28 deletions(-) diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 8b090c2aee..48d9007693 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -362,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) { From c9b84f9258d64264cadeb7114d392f93a18bdbab Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 16 Jul 2024 11:55:17 +0530 Subject: [PATCH 11/18] chore: updated action-group test to remove flakiness --- packages/action-group/test/action-group.test.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/action-group/test/action-group.test.ts b/packages/action-group/test/action-group.test.ts index fc1073fc7e..9cb13ad845 100644 --- a/packages/action-group/test/action-group.test.ts +++ b/packages/action-group/test/action-group.test.ts @@ -279,18 +279,7 @@ describe('ActionGroup', () => { // get the bounding box of the action-menu const actionMenu = el.querySelector('#action-menu') as ActionMenu; - const actionMenuRect = actionMenu.getBoundingClientRect(); - sendMouse({ - steps: [ - { - position: [ - actionMenuRect.left + actionMenuRect.width / 2, - actionMenuRect.top + actionMenuRect.height / 2, - ], - type: 'click', - }, - ], - }); + actionMenu.click(); await elementUpdated(el); From 91ddb4e0ebe2a2b80c5701e4a02a28ac5a9e6893 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Wed, 17 Jul 2024 14:55:14 +0530 Subject: [PATCH 12/18] chore: refractored interaction controller code --- packages/picker/src/DesktopController.ts | 3 +-- packages/picker/src/InteractionController.ts | 23 +++++--------------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/packages/picker/src/DesktopController.ts b/packages/picker/src/DesktopController.ts index 48b8b4bc23..e4368f47fe 100644 --- a/packages/picker/src/DesktopController.ts +++ b/packages/picker/src/DesktopController.ts @@ -33,7 +33,6 @@ export class DesktopController extends InteractionController { this.target.removeEventListener('click', cleanup); requestAnimationFrame(() => { // Complete cleanup on the second animation frame so that `click` can go first. - // this.preventNextToggle = 'no'; this.preventNextToggle = 'no'; }); }); @@ -57,7 +56,7 @@ export class DesktopController extends InteractionController { // event didn't already toggle the Picker state before doing so. return; } - this.open = !this.open; + this.host.toggle(); } override init(): void { diff --git a/packages/picker/src/InteractionController.ts b/packages/picker/src/InteractionController.ts index 2d8cfdd61c..ef47028433 100644 --- a/packages/picker/src/InteractionController.ts +++ b/packages/picker/src/InteractionController.ts @@ -48,21 +48,12 @@ export class InteractionController implements ReactiveController { public set open(open: boolean) { if (this._open === open) return; this._open = open; + if (this.overlay) { - // If there already is an Overlay, apply the value of `open` directly. - if (this.host.dependencyManager.loaded) { - this.overlay.willPreventClose = this.preventNextToggle !== 'no'; - this.overlay.open = open; - } - this.host.open = open; - return; - } - if (!open) { this.host.open = open; - // When `open` moves to `false` and there is not yet an Overlay, - // assume that no Overlay and a closed Overlay are the same and return early. return; } + // When there is no Overlay and `open` is moving to `true`, lazily import/create // an Overlay and apply that state to it. customElements @@ -72,11 +63,8 @@ export class InteractionController implements ReactiveController { '@spectrum-web-components/overlay/src/Overlay.js' ); this.overlay = new Overlay(); - this.host.requestUpdate(); - if (this.host.dependencyManager.loaded) { - this.overlay.open = open; - } this.host.open = true; + this.host.requestUpdate(); }); import('@spectrum-web-components/overlay/sp-overlay.js'); } @@ -192,9 +180,10 @@ export class InteractionController implements ReactiveController { if ( this.overlay && this.host.dependencyManager.loaded && - this.host.open + this.host.open !== this.overlay.open ) { - this.overlay.open = true; + this.overlay.willPreventClose = this.preventNextToggle !== 'no'; + this.overlay.open = this.host.open; } } } From 088f79361d2d2a05dfade72bf40c98d8cc8b379e Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Thu, 18 Jul 2024 20:02:05 +0530 Subject: [PATCH 13/18] chore: added delay for click event tests --- packages/action-group/test/action-group.test.ts | 14 +++++++++++++- packages/action-menu/test/index.ts | 2 ++ packages/picker/test/index.ts | 4 ++++ packages/split-button/test/index.ts | 12 ++++++++++++ test/plugins/send-mouse-plugin.ts | 2 +- 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/packages/action-group/test/action-group.test.ts b/packages/action-group/test/action-group.test.ts index 9cb13ad845..378fa6e842 100644 --- a/packages/action-group/test/action-group.test.ts +++ b/packages/action-group/test/action-group.test.ts @@ -279,7 +279,19 @@ describe('ActionGroup', () => { // get the bounding box of the action-menu const actionMenu = el.querySelector('#action-menu') as ActionMenu; - actionMenu.click(); + const actionMenuRect = actionMenu.getBoundingClientRect(); + sendMouse({ + steps: [ + { + position: [ + actionMenuRect.left + actionMenuRect.width / 2, + actionMenuRect.top + actionMenuRect.height / 2, + ], + type: 'click', + options: { delay: 100 }, + }, + ], + }); await elementUpdated(el); diff --git a/packages/action-menu/test/index.ts b/packages/action-menu/test/index.ts index 57b9829ca5..a272657c22 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -660,6 +660,7 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); @@ -678,6 +679,7 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts index 1e28ba2978..8540da6620 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -633,6 +633,7 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, ], }); @@ -648,6 +649,7 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, ], }); @@ -663,6 +665,7 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, ], }); @@ -678,6 +681,7 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, ], }); diff --git a/packages/split-button/test/index.ts b/packages/split-button/test/index.ts index 82b5f20f49..565b44670b 100644 --- a/packages/split-button/test/index.ts +++ b/packages/split-button/test/index.ts @@ -261,6 +261,7 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); @@ -279,6 +280,7 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); @@ -319,9 +321,11 @@ export function runSplitButtonTests( boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, { type: 'down', + options: { delay: 100 }, }, ], }); @@ -337,9 +341,11 @@ export function runSplitButtonTests( thirdItemRect.x + thirdItemRect.width / 2, thirdItemRect.y + thirdItemRect.height / 2, ], + options: { delay: 100 }, }, { type: 'up', + options: { delay: 100 }, }, ], }); @@ -428,9 +434,11 @@ export function runSplitButtonTests( boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], + options: { delay: 100 }, }, { type: 'down', + options: { delay: 100 }, }, ], }); @@ -446,9 +454,11 @@ export function runSplitButtonTests( thirdItemRect.x + thirdItemRect.width / 2, thirdItemRect.y + thirdItemRect.height / 2, ], + options: { delay: 100 }, }, { type: 'up', + options: { delay: 100 }, }, ], }); @@ -479,6 +489,7 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); @@ -497,6 +508,7 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', + options: { delay: 100 }, }, ], }); diff --git a/test/plugins/send-mouse-plugin.ts b/test/plugins/send-mouse-plugin.ts index 6c177f57c0..35ddca6270 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() { From 1c26e77e5bdb9357accd7c0024de8cad89c4a33c Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Tue, 23 Jul 2024 17:31:43 +0530 Subject: [PATCH 14/18] chore: added hacky mobile tests for mobilecontroller picker --- .../action-group/test/action-group.test.ts | 1 - packages/action-menu/test/index.ts | 2 - packages/picker/src/Picker.ts | 2 +- packages/picker/test/index.ts | 4 -- .../picker/test/picker-responsive.test.ts | 65 ++++++++++++------- packages/split-button/test/index.ts | 12 ---- test/plugins/send-mouse-plugin.ts | 1 + 7 files changed, 42 insertions(+), 45 deletions(-) diff --git a/packages/action-group/test/action-group.test.ts b/packages/action-group/test/action-group.test.ts index 378fa6e842..fc1073fc7e 100644 --- a/packages/action-group/test/action-group.test.ts +++ b/packages/action-group/test/action-group.test.ts @@ -288,7 +288,6 @@ describe('ActionGroup', () => { actionMenuRect.top + actionMenuRect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); diff --git a/packages/action-menu/test/index.ts b/packages/action-menu/test/index.ts index a272657c22..57b9829ca5 100644 --- a/packages/action-menu/test/index.ts +++ b/packages/action-menu/test/index.ts @@ -660,7 +660,6 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); @@ -679,7 +678,6 @@ export const testActionMenu = (mode: 'sync' | 'async'): void => { rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); diff --git a/packages/picker/src/Picker.ts b/packages/picker/src/Picker.ts index 48d9007693..6e67e969f4 100644 --- a/packages/picker/src/Picker.ts +++ b/packages/picker/src/Picker.ts @@ -783,7 +783,7 @@ export class PickerBase extends SizedMixin(Focusable, { noDefaultSize: true }) { ); }; - protected bindEvents(): void { + public bindEvents(): void { this.strategy?.abort(); if (this.isMobile.matches) { this.strategy = new strategies['mobile'](this.button, this); diff --git a/packages/picker/test/index.ts b/packages/picker/test/index.ts index 8540da6620..1e28ba2978 100644 --- a/packages/picker/test/index.ts +++ b/packages/picker/test/index.ts @@ -633,7 +633,6 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, ], }); @@ -649,7 +648,6 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, ], }); @@ -665,7 +663,6 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, ], }); @@ -681,7 +678,6 @@ export function runPickerTests(): void { boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, ], }); diff --git a/packages/picker/test/picker-responsive.test.ts b/packages/picker/test/picker-responsive.test.ts index 19792919e8..dd7f054213 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,14 @@ describe('Picker, responsive', () => { await elementUpdated(el); }); - xit('is a Tray in mobile', async () => { + it.only('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.requestUpdate(); + 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 +73,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/test/index.ts b/packages/split-button/test/index.ts index 565b44670b..82b5f20f49 100644 --- a/packages/split-button/test/index.ts +++ b/packages/split-button/test/index.ts @@ -261,7 +261,6 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); @@ -280,7 +279,6 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); @@ -321,11 +319,9 @@ export function runSplitButtonTests( boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, { type: 'down', - options: { delay: 100 }, }, ], }); @@ -341,11 +337,9 @@ export function runSplitButtonTests( thirdItemRect.x + thirdItemRect.width / 2, thirdItemRect.y + thirdItemRect.height / 2, ], - options: { delay: 100 }, }, { type: 'up', - options: { delay: 100 }, }, ], }); @@ -434,11 +428,9 @@ export function runSplitButtonTests( boundingRect.x + boundingRect.width / 2, boundingRect.y + boundingRect.height / 2, ], - options: { delay: 100 }, }, { type: 'down', - options: { delay: 100 }, }, ], }); @@ -454,11 +446,9 @@ export function runSplitButtonTests( thirdItemRect.x + thirdItemRect.width / 2, thirdItemRect.y + thirdItemRect.height / 2, ], - options: { delay: 100 }, }, { type: 'up', - options: { delay: 100 }, }, ], }); @@ -489,7 +479,6 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); @@ -508,7 +497,6 @@ export function runSplitButtonTests( rect.top + rect.height / 2, ], type: 'click', - options: { delay: 100 }, }, ], }); diff --git a/test/plugins/send-mouse-plugin.ts b/test/plugins/send-mouse-plugin.ts index 35ddca6270..55d965c9d2 100644 --- a/test/plugins/send-mouse-plugin.ts +++ b/test/plugins/send-mouse-plugin.ts @@ -38,6 +38,7 @@ export function sendMousePlugin() { const page = session.browser.getPage(session.id); for (const step of payload.steps) { step.options = step.options || {}; + step.options.delay = 1; if (step.position) { await page.mouse[step.type]( Math.round(step.position[0]), From 344344ee6d6a01938c03182873a219cc40509e06 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Wed, 24 Jul 2024 14:15:59 +0530 Subject: [PATCH 15/18] chore: removed unexpected only from picker responsive test --- packages/picker/test/picker-responsive.test.ts | 3 +-- test/plugins/send-mouse-plugin.ts | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/picker/test/picker-responsive.test.ts b/packages/picker/test/picker-responsive.test.ts index dd7f054213..2a7062a7d3 100644 --- a/packages/picker/test/picker-responsive.test.ts +++ b/packages/picker/test/picker-responsive.test.ts @@ -54,12 +54,11 @@ describe('Picker, responsive', () => { await elementUpdated(el); }); - it.only('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.requestUpdate(); el.bindEvents(); /** diff --git a/test/plugins/send-mouse-plugin.ts b/test/plugins/send-mouse-plugin.ts index 55d965c9d2..6587bae2af 100644 --- a/test/plugins/send-mouse-plugin.ts +++ b/test/plugins/send-mouse-plugin.ts @@ -38,6 +38,8 @@ 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]( From 7f051a3d901269979fbe66f63b23f812a7f433b3 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Thu, 25 Jul 2024 11:19:55 +0530 Subject: [PATCH 16/18] chore: updated menu interaction model --- packages/menu/src/Menu.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index a28059c81c..e8e5bfe152 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -342,22 +342,21 @@ 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; - }); + event.target?.dispatchEvent(new Event('click')); + this.pointerUpTarget = event.target; this.handlePointerBasedSelection(event); } From 6b214b8f62d97e42b0c6d49f29f161ec2e00f617 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Thu, 25 Jul 2024 18:35:13 +0530 Subject: [PATCH 17/18] chore: don't dispatch synthetic click event on menu selection --- packages/menu/src/Menu.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/menu/src/Menu.ts b/packages/menu/src/Menu.ts index e8e5bfe152..ec40e7ec41 100644 --- a/packages/menu/src/Menu.ts +++ b/packages/menu/src/Menu.ts @@ -355,7 +355,6 @@ export class Menu extends SizedMixin(SpectrumElement, { noDefaultSize: true }) { } private handlePointerup(event: Event): void { - event.target?.dispatchEvent(new Event('click')); this.pointerUpTarget = event.target; this.handlePointerBasedSelection(event); } From fee37bd6cef13f41a554ce89cc967a0b7df86968 Mon Sep 17 00:00:00 2001 From: TarunAdobe Date: Thu, 25 Jul 2024 18:44:39 +0530 Subject: [PATCH 18/18] chore: removed split-button tests cz its deprecated --- packages/split-button/test/index.ts | 123 ---------------------------- 1 file changed, 123 deletions(-) diff --git a/packages/split-button/test/index.ts b/packages/split-button/test/index.ts index 82b5f20f49..fa2299bb9c 100644 --- a/packages/split-button/test/index.ts +++ b/packages/split-button/test/index.ts @@ -32,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, @@ -289,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 })) @@ -396,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 }))