From fe7b7f554918de08a7f3de0ef8c6eacfa9559a5a Mon Sep 17 00:00:00 2001 From: ShaneK Date: Tue, 18 Aug 2026 20:48:50 -0700 Subject: [PATCH 1/2] fix(picker-column): commit value on outside press --- .../test/prefer-wheel/datetime.e2e.ts | 92 +++++ .../picker-column/picker-column.tsx | 231 ++++++++++--- .../test/scroll/picker-column.e2e.ts | 315 ++++++++++++++++++ 3 files changed, 593 insertions(+), 45 deletions(-) create mode 100644 core/src/components/picker-column/test/scroll/picker-column.e2e.ts diff --git a/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts b/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts index fb1b2e31ebf..5f7f3586ee3 100644 --- a/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts +++ b/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts @@ -688,3 +688,95 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { }); }); }); + +/** + * This behavior does not vary across modes/directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('datetime: wheel value'), () => { + test('should give an outside click handler the date the wheel is showing', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/30449', + }); + + await page.setContent( + ` + + + `, + config + ); + + await page.locator('.datetime-ready').waitFor(); + + await page.evaluate(() => { + const datetime = document.querySelector('ion-datetime') as any; + const column = datetime.shadowRoot.querySelector('.year-column'); + const scrollEl = column.shadowRoot.querySelector('.picker-opts'); + const w = window as any; + + w.lastScrollAt = 0; + scrollEl.addEventListener('scroll', () => { + w.lastScrollAt = performance.now(); + }); + + /** + * Stands in for an application's own Save button, which reads the + * datetime's value when it is clicked. + */ + document.querySelector('#save')!.addEventListener('click', () => { + w.onSave = { + datetimeValue: datetime.value, + visibleYear: String(column.querySelector('.option-active')?.value ?? ''), + msSinceScroll: performance.now() - w.lastScrollAt, + }; + }); + + w.startScroll = () => scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + }); + + /** + * Press the column the way a drag would, so the scroll that follows counts + * as the user's. Pressing dead centre lands on the year already selected, + * so the press itself does not change the value. + */ + const column = (await page.locator('.year-column').boundingBox())!; + await page.mouse.move(column.x + column.width / 2, column.y + column.height / 2); + await page.mouse.down(); + await page.mouse.up(); + + await page.evaluate(() => (window as any).startScroll()); + + /** + * Wait until a different year is under the highlight, while the column is + * still scrolling towards its resting place. + */ + await page.waitForFunction( + () => { + const datetime = document.querySelector('ion-datetime') as any; + const highlighted = datetime.shadowRoot.querySelector('.year-column .option-active'); + const isScrolling = performance.now() - (window as any).lastScrollAt < 100; + return highlighted !== null && String(highlighted.value) !== '2022' && isScrolling; + }, + undefined, + { timeout: 5000 } + ); + + await page.locator('#save').click(); + + const onSave = await page.evaluate(() => (window as any).onSave); + + /** + * Guards against a false pass: if the column had already stopped + * scrolling then this test is not exercising the race at all. + */ + expect(onSave.msSinceScroll).toBeLessThan(100); + + expect(onSave.visibleYear).not.toBe('2022'); + + // The value the Save button saw is the year the user could see. + expect(onSave.datetimeValue).toContain(`${onSave.visibleYear}-`); + }); + }); +}); diff --git a/core/src/components/picker-column/picker-column.tsx b/core/src/components/picker-column/picker-column.tsx index 80de2c2daee..f60e9a42c96 100644 --- a/core/src/components/picker-column/picker-column.tsx +++ b/core/src/components/picker-column/picker-column.tsx @@ -33,6 +33,15 @@ export class PickerColumn implements ComponentInterface { private canExitInputMode = true; private assistiveFocusable?: HTMLElement; private updateValueTextOnScroll = false; + private scrollEndTimeout?: ReturnType; + private centeredOption?: HTMLIonPickerColumnOptionElement; + private isTrackingScroll = false; + private isUserScroll = false; + /** + * The haptics for the wheel picker are an iOS-only feature. As a result, + * they should be disabled on Android. + */ + private enableHaptics = false; @State() ariaLabel: string | null = null; @@ -134,6 +143,7 @@ export class PickerColumn implements ComponentInterface { this.initializeScrollListener(); } else { this.isColumnVisible = false; + this.stopTrackingScroll(); if (this.destroyScrollListener) { this.destroyScrollListener(); @@ -219,6 +229,14 @@ export class PickerColumn implements ComponentInterface { this.ariaLabel = this.el.getAttribute('aria-label') ?? 'Select a value'; } + disconnectedCallback() { + /** + * A pending settle would otherwise commit a value on a column that is no + * longer in the DOM. + */ + this.discardScroll(); + } + private centerPickerItemInView = (target: HTMLElement, smooth = true, canExitInputMode = true) => { const { isColumnVisible, scrollEl } = this; @@ -236,6 +254,7 @@ export class PickerColumn implements ComponentInterface { */ this.canExitInputMode = canExitInputMode; this.updateValueTextOnScroll = false; + this.isUserScroll = false; scrollEl.scroll({ top, left: 0, @@ -298,6 +317,129 @@ export class PickerColumn implements ComponentInterface { this.isActive = state; }; + private clearScrollEndTimeout = () => { + if (this.scrollEndTimeout) { + clearTimeout(this.scrollEndTimeout); + this.scrollEndTimeout = undefined; + } + }; + + /** + * Restarts the idle timer that commits the centered option. + */ + private restartScrollEndTimeout = () => { + this.clearScrollEndTimeout(); + this.scrollEndTimeout = setTimeout(this.settle, SCROLL_END_DELAY); + }; + + /** + * Capture so the column still commits if an application's own pointerdown + * handler stops the event before it reaches the document. + */ + private watchForOutsidePress = () => { + doc?.addEventListener('pointerdown', this.onPointerDownOutside, { capture: true }); + }; + + private stopWatchingForOutsidePress = () => { + doc?.removeEventListener('pointerdown', this.onPointerDownOutside, { capture: true }); + }; + + /** + * Stops reacting to scrolls without touching a commit that is already + * pending. Hiding a column has always let that commit land. + */ + private stopTrackingScroll = () => { + this.isTrackingScroll = false; + this.isUserScroll = false; + this.stopWatchingForOutsidePress(); + }; + + /** + * Abandons the scroll and everything pending on it, for a column that is + * going away and cannot commit anything. The isScrolling reset matters too: + * the next scroll reads it to decide whether it is starting a fresh + * interaction. + */ + private discardScroll = () => { + this.stopTrackingScroll(); + this.clearScrollEndTimeout(); + this.scrollEndCallback = undefined; + this.centeredOption = undefined; + this.isScrolling = false; + }; + + /** + * Commits the option that is currently centered under the highlight and ends + * the scroll interaction. + */ + private settle = () => { + const { centeredOption } = this; + + this.clearScrollEndTimeout(); + this.stopWatchingForOutsidePress(); + + this.isUserScroll = false; + this.isScrolling = false; + this.updateValueTextOnScroll = true; + + this.enableHaptics && hapticSelectionEnd(); + + /** + * Certain tasks (such as those that + * cause re-renders) should only be done + * once scrolling has finished, otherwise + * flickering may occur. + */ + const { scrollEndCallback } = this; + if (scrollEndCallback) { + scrollEndCallback(); + this.scrollEndCallback = undefined; + } + + /** + * Reset this flag as the + * next scroll interaction could + * be a scroll from the user. In this + * case, we should exit input mode. + */ + this.canExitInputMode = true; + + if (centeredOption !== undefined) { + this.setValue(centeredOption.value); + } + }; + + /** + * Commits the option the user can see when they press anything outside of the + * column during a scroll they started. This runs on pointerdown rather than + * click so the value is up to date by the time an application's own click + * handler (a Save button, for example) reads it. + */ + private onPointerDownOutside = (ev: Event) => { + const { centeredOption } = this; + + /** + * The column can live in another component's Shadow DOM, where a document + * listener sees the target retargeted to the outer host. The composed path + * is the only reliable way to tell a press on the column apart. + */ + if (!this.isScrolling || ev.composedPath().includes(this.el)) { + return; + } + + /** + * Landing on the centered option halts the in-flight momentum scroll, so the + * committed value and the option the user is left looking at agree. The + * option list can be replaced mid-scroll, and a detached option has no + * offset to center on. + */ + if (centeredOption !== undefined && centeredOption.isConnected) { + this.centerPickerItemInView(centeredOption, false, false); + } + + this.settle(); + }; + /** * When the column scrolls, the component * needs to determine which item is centered @@ -305,29 +447,35 @@ export class PickerColumn implements ComponentInterface { * the item object. */ private initializeScrollListener = () => { - /** - * The haptics for the wheel picker are - * an iOS-only feature. As a result, they should - * be disabled on Android. - */ - const enableHaptics = isPlatform('ios'); + this.enableHaptics = isPlatform('ios'); + this.isTrackingScroll = true; const { el, scrollEl } = this; - let timeout: ReturnType | undefined; let activeEl: HTMLIonPickerColumnOptionElement | undefined = this.activeItem; const scrollCallback = () => { raf(() => { - if (!scrollEl) return; + /** + * This frame cannot be cancelled, so a scroll that arrived just before + * the column stopped tracking would otherwise re-arm it. + */ + if (!this.isTrackingScroll || !scrollEl) return; - if (timeout) { - clearTimeout(timeout); - timeout = undefined; - } + // Armed before the early returns below, so a scroll never ends uncommitted. + this.restartScrollEndTimeout(); if (!this.isScrolling) { - enableHaptics && hapticSelectionStart(); + this.enableHaptics && hapticSelectionStart(); this.isScrolling = true; + + /** + * Only a scroll the user drove represents an uncommitted selection. + * A scroll the column started itself is already heading for the value + * that was set, so an outside press must not cut it short. + */ + if (this.isUserScroll) { + this.watchForOutsidePress(); + } } /** @@ -396,20 +544,25 @@ export class PickerColumn implements ComponentInterface { } } - if (activeEl !== undefined) { - this.setPickerItemActiveState(activeEl, false); - } - + /** + * A scroll can land with no selectable option centered, such as an + * overscroll bounce briefly centering the empty padding rows. Keep the + * current selection until an option is centered again. + */ if (newActiveElement === undefined || newActiveElement.disabled) { return; } + if (activeEl !== undefined) { + this.setPickerItemActiveState(activeEl, false); + } + /** * If we are selecting a new value, * we need to run haptics again. */ if (newActiveElement !== activeEl) { - enableHaptics && hapticSelectionChanged(); + this.enableHaptics && hapticSelectionChanged(); if (this.canExitInputMode) { /** @@ -445,36 +598,14 @@ export class PickerColumn implements ComponentInterface { this.assistiveFocusable?.setAttribute('aria-valuetext', this.getOptionValueText(newActiveElement)); } - timeout = setTimeout(() => { - this.isScrolling = false; - this.updateValueTextOnScroll = true; - enableHaptics && hapticSelectionEnd(); - - /** - * Certain tasks (such as those that - * cause re-renders) should only be done - * once scrolling has finished, otherwise - * flickering may occur. - */ - const { scrollEndCallback } = this; - if (scrollEndCallback) { - scrollEndCallback(); - this.scrollEndCallback = undefined; - } - - /** - * Reset this flag as the - * next scroll interaction could - * be a scroll from the user. In this - * case, we should exit input mode. - */ - this.canExitInputMode = true; - - this.setValue(newActiveElement.value); - }, 250); + this.centeredOption = newActiveElement; }); }; + const userScrollCallback = () => { + this.isUserScroll = true; + }; + /** * Wrap this in an raf so that the scroll callback * does not fire when component is initially shown. @@ -483,9 +614,13 @@ export class PickerColumn implements ComponentInterface { if (!scrollEl) return; scrollEl.addEventListener('scroll', scrollCallback); + scrollEl.addEventListener('pointerdown', userScrollCallback); + scrollEl.addEventListener('wheel', userScrollCallback, { passive: true }); this.destroyScrollListener = () => { scrollEl.removeEventListener('scroll', scrollCallback); + scrollEl.removeEventListener('pointerdown', userScrollCallback); + scrollEl.removeEventListener('wheel', userScrollCallback); }; }); }; @@ -707,3 +842,9 @@ export class PickerColumn implements ComponentInterface { } const PICKER_ITEM_ACTIVE_CLASS = 'option-active'; + +/** + * How long the column must be idle before the centered option is treated as + * the user's selection. + */ +const SCROLL_END_DELAY = 250; diff --git a/core/src/components/picker-column/test/scroll/picker-column.e2e.ts b/core/src/components/picker-column/test/scroll/picker-column.e2e.ts new file mode 100644 index 00000000000..6929bfb727a --- /dev/null +++ b/core/src/components/picker-column/test/scroll/picker-column.e2e.ts @@ -0,0 +1,315 @@ +import { expect } from '@playwright/test'; +import type { E2EPage } from '@utils/test/playwright'; +import { configs, test } from '@utils/test/playwright'; + +/** + * This behavior does not vary across modes/directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('picker-column: scroll'), () => { + test.beforeEach(async ({ page }) => { + await page.setContent( + ` + + + ${Array.from( + { length: 200 }, + (_, i) => `${i}` + ).join('')} + + + + `, + config + ); + + await page.locator('ion-picker-column-option.option-active').waitFor(); + + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + const w = window as any; + + w.lastScrollAt = 0; + scrollEl.addEventListener('scroll', () => { + w.lastScrollAt = performance.now(); + }); + + /** + * Records the value at the moment the click handler ran, alongside the + * option the user could actually see under the highlight. + */ + document.querySelector('#save')!.addEventListener('click', () => { + w.onSave = { + value: col.value, + highlighted: col.querySelector('.option-active')?.value ?? null, + msSinceScroll: performance.now() - w.lastScrollAt, + }; + }); + + w.startScroll = () => scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + }); + }); + + /** + * Presses the column the way a drag would, so the scroll that follows counts + * as the user's. Pressing dead centre lands on the option already selected, + * so the press itself does not change the value. + */ + const pressColumn = async (page: E2EPage) => { + const box = (await page.locator('ion-picker-column').boundingBox())!; + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.up(); + }; + + /** + * Clicks the Save button once the column has visibly moved off its starting + * option but is still scrolling towards its resting place. + */ + const clickSaveMidScroll = async (page: E2EPage) => { + await pressColumn(page); + await page.evaluate(() => (window as any).startScroll()); + + await page.waitForFunction( + () => { + const col = document.querySelector('ion-picker-column') as any; + const highlighted = col.querySelector('.option-active'); + const isScrolling = performance.now() - (window as any).lastScrollAt < 100; + return highlighted !== null && highlighted.value !== '5' && isScrolling; + }, + undefined, + { timeout: 5000 } + ); + + await page.locator('#save').click(); + + return await page.evaluate(() => (window as any).onSave); + }; + + test('should commit the visible option before an outside click handler runs', async ({ page }, testInfo) => { + testInfo.annotations.push({ + type: 'issue', + description: 'https://github.com/ionic-team/ionic-framework/issues/30449', + }); + + const onSave = await clickSaveMidScroll(page); + + /** + * Guards against a false pass: if the column had already stopped + * scrolling then this test is not exercising the race at all. + */ + expect(onSave.msSinceScroll).toBeLessThan(100); + + expect(onSave.highlighted).not.toBe('5'); + + // The value the Save button saw is the option the user could see. + expect(onSave.value).toBe(onSave.highlighted); + }); + + test('should not move on past the option it committed to an outside click', async ({ page }) => { + const onSave = await clickSaveMidScroll(page); + + // Give any residual momentum and the pending commit time to resolve. + await page.waitForTimeout(600); + + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', onSave.value); + }); + + /** + * An overscroll bounce can briefly leave the column's empty padding rows + * under the highlight instead of an option. That frame must not throw away + * the current selection or the pending commit. + */ + test('should keep its selection when a scroll frame has no option centered', async ({ page }) => { + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + + /** + * Snapping would otherwise pull the column straight back onto an + * option, which is what hides this in a normal scroll. + */ + scrollEl.style.scrollSnapType = 'none'; + scrollEl.scrollTop = 0; + scrollEl.dispatchEvent(new Event('scroll')); + }); + + // Long enough that any pending commit has resolved. + await page.waitForTimeout(400); + + await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); + + const { value, highlighted } = await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + return { value: col.value, highlighted: col.querySelector('.option-active')?.value ?? null }; + }); + + expect(value).toBe(highlighted); + }); + + /** + * The same uncentered frame arriving part way through a scroll must keep the + * option that was already centered, and must still commit it. + */ + test('should commit the last centered option when a later frame has none', async ({ page }) => { + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + const option = col.querySelectorAll('ion-picker-column-option')[10]; + + /** + * Snapping would otherwise pull the column back onto an option, which is + * what hides this in a normal scroll. + */ + scrollEl.style.scrollSnapType = 'none'; + + option.scrollIntoView({ block: 'center' }); + scrollEl.dispatchEvent(new Event('scroll')); + }); + + // Let the column register option 10 as centered. + await expect(page.locator('ion-picker-column-option.option-active')).toHaveJSProperty('value', '10'); + + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + + // Now put the empty padding rows under the highlight. + scrollEl.scrollTop = 0; + scrollEl.dispatchEvent(new Event('scroll')); + }); + + // Long enough that any pending commit has resolved. + await page.waitForTimeout(400); + + await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '10'); + }); + + /** + * The frame in which the column reacts to a scroll cannot be cancelled, so + * it can land after the column has already been torn down. + */ + test('should not commit a value after the column is removed mid-scroll', async ({ page }) => { + const changes = await page.evaluate(async () => { + const col = document.querySelector('ion-picker-column') as any; + const picker = document.querySelector('ion-picker')!; + const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + const recorded: unknown[] = []; + + col.addEventListener('ionChange', (ev: any) => recorded.push(ev.detail.value)); + + /** + * The column registered its own scroll listener first, so by the time + * this one runs the column has already queued the frame that reacts to + * this scroll. Removing the column here leaves that frame pending. Wait + * a few scrolls first so the column has centered an option to commit. + */ + let scrolls = 0; + scrollEl.addEventListener('scroll', () => { + if (++scrolls === 5) { + picker.remove(); + } + }); + + scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + + // Long enough that any pending commit has resolved. + await new Promise((resolve) => setTimeout(resolve, 600)); + + return recorded; + }); + + expect(changes).toEqual([]); + }); + + /** + * A scroll the column starts itself is not a selection the user has made, so + * an outside press during one must not freeze it or commit an option it is + * only passing through. + */ + test('should not commit an option that a programmatic scroll is passing through', async ({ page }) => { + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const w = window as any; + + w.changes = []; + col.addEventListener('ionChange', (ev: any) => w.changes.push(ev.detail.value)); + + col.value = '150'; + }); + + // Wait until the column is part way to the option that was just set. + await page.waitForFunction( + () => { + const highlighted = document.querySelector('ion-picker-column .option-active') as any; + return highlighted !== null && highlighted.value !== '5' && highlighted.value !== '150'; + }, + undefined, + { timeout: 5000 } + ); + + await page.locator('#save').click(); + + // Long enough that any pending commit has resolved. + await page.waitForTimeout(600); + + const result = await page.evaluate(() => ({ + value: (document.querySelector('ion-picker-column') as any).value, + changes: (window as any).changes, + })); + + // Setting the value property must not emit ionChange. + expect(result.changes).toEqual([]); + + // The column must still be headed for the option the application asked for. + expect(result.value).toBe('150'); + }); + + /** + * Selecting an option directly scrolls the column to it. That scroll belongs + * to the selection the user already made, so an outside press during it must + * not redirect the value to an option on the way. + */ + test('should not commit an option that a selected scroll is passing through', async ({ page }) => { + await page.evaluate(() => { + const col = document.querySelector('ion-picker-column') as any; + const w = window as any; + + w.changes = []; + col.addEventListener('ionChange', (ev: any) => w.changes.push(ev.detail.value)); + }); + + // A press that selects the option already under the highlight, so nothing scrolls yet. + await pressColumn(page); + + // Stands in for tapping an option far down the column. + await page.evaluate(() => (document.querySelector('ion-picker-column') as any).setValue('150')); + + // Wait until the column is part way to the option that was selected. + await page.waitForFunction( + () => { + const highlighted = document.querySelector('ion-picker-column .option-active') as any; + return highlighted !== null && highlighted.value !== '5' && highlighted.value !== '150'; + }, + undefined, + { timeout: 5000 } + ); + + await page.locator('#save').click(); + + // Long enough that any pending commit has resolved. + await page.waitForTimeout(600); + + const result = await page.evaluate(() => ({ + value: (document.querySelector('ion-picker-column') as any).value, + changes: (window as any).changes, + })); + + // Only the selection itself is committed, not an option on the way to it. + expect(result.changes).toEqual(['150']); + expect(result.value).toBe('150'); + }); + }); +}); From c8b6c1c2d93ff111db5b5f1c26b1d592c9d8c0a6 Mon Sep 17 00:00:00 2001 From: ShaneK Date: Fri, 21 Aug 2026 08:28:15 -0700 Subject: [PATCH 2/2] fix(picker-column): commit every coasting wheel to the option it shows --- core/src/components/datetime/datetime.tsx | 45 +- .../test/prefer-wheel/datetime.e2e.ts | 69 ++- .../picker-column/picker-column.tsx | 129 +++-- .../picker-column/test/scroll/index.html | 74 +++ .../test/scroll/picker-column.e2e.ts | 545 +++++++++++++----- 5 files changed, 629 insertions(+), 233 deletions(-) create mode 100644 core/src/components/picker-column/test/scroll/index.html diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index 4613befccac..3de359a494a 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -1716,8 +1716,6 @@ export class Datetime implements ComponentInterface { private renderCombinedDatePickerColumn() { const { defaultParts, disabled, workingParts, locale, minParts, maxParts, todayParts, isDateEnabled } = this; - const activePart = this.getActivePartsWithFallback(); - /** * By default, generate a range of 3 months: * Previous month, current month, and next month @@ -1801,8 +1799,11 @@ export class Datetime implements ComponentInterface { const { value } = ev.detail; const findPart = parts.find(({ month, day, year }) => value === `${year}-${month}-${day}`); + // Read live so parts a sibling column just committed are included. + const activePart = this.getActivePartsWithFallback(); + this.setWorkingParts({ - ...workingParts, + ...this.workingParts, ...findPart, }); @@ -1908,7 +1909,6 @@ export class Datetime implements ComponentInterface { const { disabled, workingParts } = this; - const activePart = this.getActivePartsWithFallback(); const pickerColumnValue = (workingParts.day !== null ? workingParts.day : this.defaultParts.day) ?? undefined; return ( @@ -1920,8 +1920,11 @@ export class Datetime implements ComponentInterface { disabled={disabled} value={pickerColumnValue} onIonChange={(ev: CustomEvent) => { + // Read live so parts a sibling column just committed are included. + const activePart = this.getActivePartsWithFallback(); + this.setWorkingParts({ - ...workingParts, + ...this.workingParts, day: ev.detail.value, }); @@ -1955,8 +1958,6 @@ export class Datetime implements ComponentInterface { const { disabled, workingParts } = this; - const activePart = this.getActivePartsWithFallback(); - return ( { + // Read live so parts a sibling column just committed are included. + const activePart = this.getActivePartsWithFallback(); + this.setWorkingParts({ - ...workingParts, + ...this.workingParts, month: ev.detail.value, }); @@ -2003,8 +2007,6 @@ export class Datetime implements ComponentInterface { const { disabled, workingParts } = this; - const activePart = this.getActivePartsWithFallback(); - return ( { + // Read live so parts a sibling column just committed are included. + const activePart = this.getActivePartsWithFallback(); + this.setWorkingParts({ - ...workingParts, + ...this.workingParts, year: ev.detail.value, }); @@ -2079,7 +2084,7 @@ export class Datetime implements ComponentInterface { } private renderHourPickerColumn(hoursData: WheelColumnOption[]) { - const { disabled, workingParts } = this; + const { disabled } = this; if (hoursData.length === 0) return []; const activePart = this.getActivePartsWithFallback(); @@ -2093,8 +2098,9 @@ export class Datetime implements ComponentInterface { value={activePart.hour} numericInput onIonChange={(ev: CustomEvent) => { + // Read live so parts a sibling column just committed are included. this.setWorkingParts({ - ...workingParts, + ...this.workingParts, hour: ev.detail.value, }); @@ -2121,7 +2127,7 @@ export class Datetime implements ComponentInterface { ); } private renderMinutePickerColumn(minutesData: WheelColumnOption[]) { - const { disabled, workingParts } = this; + const { disabled } = this; if (minutesData.length === 0) return []; const activePart = this.getActivePartsWithFallback(); @@ -2135,8 +2141,9 @@ export class Datetime implements ComponentInterface { value={activePart.minute} numericInput onIonChange={(ev: CustomEvent) => { + // Read live so parts a sibling column just committed are included. this.setWorkingParts({ - ...workingParts, + ...this.workingParts, minute: ev.detail.value, }); @@ -2163,7 +2170,7 @@ export class Datetime implements ComponentInterface { ); } private renderDayPeriodPickerColumn(dayPeriodData: WheelColumnOption[]) { - const { disabled, workingParts } = this; + const { disabled } = this; if (dayPeriodData.length === 0) { return []; } @@ -2180,10 +2187,12 @@ export class Datetime implements ComponentInterface { disabled={disabled} value={activePart.ampm} onIonChange={(ev: CustomEvent) => { - const hour = calculateHourFromAMPM(workingParts, ev.detail.value); + // Read live so parts a sibling column just committed are included. + const currentParts = this.workingParts; + const hour = calculateHourFromAMPM(currentParts, ev.detail.value); this.setWorkingParts({ - ...workingParts, + ...currentParts, ampm: ev.detail.value, hour, }); diff --git a/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts b/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts index 5f7f3586ee3..edfdd29c1d3 100644 --- a/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts +++ b/core/src/components/datetime/test/prefer-wheel/datetime.e2e.ts @@ -693,6 +693,10 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { * This behavior does not vary across modes/directions. */ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + /** + * The column sits in the datetime's Shadow DOM, so a document-level listener + * sees the press retargeted to the datetime host. + */ test.describe(title('datetime: wheel value'), () => { test('should give an outside click handler the date the wheel is showing', async ({ page }, testInfo) => { testInfo.annotations.push({ @@ -711,9 +715,9 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await page.locator('.datetime-ready').waitFor(); await page.evaluate(() => { - const datetime = document.querySelector('ion-datetime') as any; - const column = datetime.shadowRoot.querySelector('.year-column'); - const scrollEl = column.shadowRoot.querySelector('.picker-opts'); + const datetime = document.querySelector('ion-datetime')!; + const column = datetime.shadowRoot!.querySelector('ion-picker-column.year-column')!; + const scrollEl = column.shadowRoot!.querySelector('.picker-opts')!; const w = window as any; w.lastScrollAt = 0; @@ -728,17 +732,44 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => document.querySelector('#save')!.addEventListener('click', () => { w.onSave = { datetimeValue: datetime.value, - visibleYear: String(column.querySelector('.option-active')?.value ?? ''), - msSinceScroll: performance.now() - w.lastScrollAt, + visibleYear: String(column.querySelector('.option-active')?.value ?? ''), }; }); w.startScroll = () => scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + + /** + * Presses Save in the same frame the wheel is first seen showing a + * different year while still scrolling. Done in the page so no round trip + * can let the scroll finish first, which would leave nothing to race. + */ + w.pressSaveWhenMidScroll = () => + new Promise((resolve, reject) => { + const deadline = performance.now() + 5000; + + const poll = () => { + const highlighted = column.querySelector('.option-active'); + const isScrolling = performance.now() - w.lastScrollAt < 100; + + if (highlighted !== null && String(highlighted.value) !== '2022' && isScrolling) { + const save = document.querySelector('#save')!; + save.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, composed: true })); + save.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + resolve(); + } else if (performance.now() > deadline) { + reject(new Error('the wheel never showed a year other than the one it was reporting')); + } else { + requestAnimationFrame(poll); + } + }; + + requestAnimationFrame(poll); + }); }); /** * Press the column the way a drag would, so the scroll that follows counts - * as the user's. Pressing dead centre lands on the year already selected, + * as the user's. Pressing dead center lands on the year already selected, * so the press itself does not change the value. */ const column = (await page.locator('.year-column').boundingBox())!; @@ -747,36 +778,14 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await page.mouse.up(); await page.evaluate(() => (window as any).startScroll()); - - /** - * Wait until a different year is under the highlight, while the column is - * still scrolling towards its resting place. - */ - await page.waitForFunction( - () => { - const datetime = document.querySelector('ion-datetime') as any; - const highlighted = datetime.shadowRoot.querySelector('.year-column .option-active'); - const isScrolling = performance.now() - (window as any).lastScrollAt < 100; - return highlighted !== null && String(highlighted.value) !== '2022' && isScrolling; - }, - undefined, - { timeout: 5000 } - ); - - await page.locator('#save').click(); + await page.evaluate(() => (window as any).pressSaveWhenMidScroll()); const onSave = await page.evaluate(() => (window as any).onSave); - /** - * Guards against a false pass: if the column had already stopped - * scrolling then this test is not exercising the race at all. - */ - expect(onSave.msSinceScroll).toBeLessThan(100); - expect(onSave.visibleYear).not.toBe('2022'); // The value the Save button saw is the year the user could see. - expect(onSave.datetimeValue).toContain(`${onSave.visibleYear}-`); + expect(onSave.datetimeValue.split('-')[0]).toBe(onSave.visibleYear); }); }); }); diff --git a/core/src/components/picker-column/picker-column.tsx b/core/src/components/picker-column/picker-column.tsx index f60e9a42c96..ea56f0711b2 100644 --- a/core/src/components/picker-column/picker-column.tsx +++ b/core/src/components/picker-column/picker-column.tsx @@ -35,8 +35,12 @@ export class PickerColumn implements ComponentInterface { private updateValueTextOnScroll = false; private scrollEndTimeout?: ReturnType; private centeredOption?: HTMLIonPickerColumnOptionElement; - private isTrackingScroll = false; private isUserScroll = false; + /** + * Where the column halted itself, so the halt's own scroll events are not + * read as movement. + */ + private haltedAtScrollTop?: number; /** * The haptics for the wheel picker are an iOS-only feature. As a result, * they should be disabled on Android. @@ -143,7 +147,15 @@ export class PickerColumn implements ComponentInterface { this.initializeScrollListener(); } else { this.isColumnVisible = false; - this.stopTrackingScroll(); + + /** + * The pending timer still runs so it resets the scroll state, but the + * option it would have committed is dropped. A column the user cannot + * see should not change its value. + */ + this.stopWatchingForOutsidePress(); + this.centeredOption = undefined; + this.haltedAtScrollTop = undefined; if (this.destroyScrollListener) { this.destroyScrollListener(); @@ -254,7 +266,14 @@ export class PickerColumn implements ComponentInterface { */ this.canExitInputMode = canExitInputMode; this.updateValueTextOnScroll = false; + + /** + * This scroll takes over from whatever the user was doing, so an outside + * press must not commit an option the column is only passing through. + */ this.isUserScroll = false; + this.stopWatchingForOutsidePress(); + scrollEl.scroll({ top, left: 0, @@ -325,9 +344,9 @@ export class PickerColumn implements ComponentInterface { }; /** - * Restarts the idle timer that commits the centered option. + * Resets the idle timer that commits the centered option. */ - private restartScrollEndTimeout = () => { + private resetScrollEndTimeout = () => { this.clearScrollEndTimeout(); this.scrollEndTimeout = setTimeout(this.settle, SCROLL_END_DELAY); }; @@ -345,27 +364,18 @@ export class PickerColumn implements ComponentInterface { }; /** - * Stops reacting to scrolls without touching a commit that is already - * pending. Hiding a column has always let that commit land. - */ - private stopTrackingScroll = () => { - this.isTrackingScroll = false; - this.isUserScroll = false; - this.stopWatchingForOutsidePress(); - }; - - /** - * Abandons the scroll and everything pending on it, for a column that is - * going away and cannot commit anything. The isScrolling reset matters too: - * the next scroll reads it to decide whether it is starting a fresh - * interaction. + * Resets everything the current scroll interaction is holding, so nothing + * pending on it can commit later. */ private discardScroll = () => { - this.stopTrackingScroll(); + this.stopWatchingForOutsidePress(); this.clearScrollEndTimeout(); this.scrollEndCallback = undefined; this.centeredOption = undefined; + this.haltedAtScrollTop = undefined; this.isScrolling = false; + this.isUserScroll = false; + this.canExitInputMode = true; }; /** @@ -405,7 +415,12 @@ export class PickerColumn implements ComponentInterface { this.canExitInputMode = true; if (centeredOption !== undefined) { - this.setValue(centeredOption.value); + // Cleared here so a later scroll that centers nothing cannot fall back on it. + this.centeredOption = undefined; + + if (centeredOption.isConnected) { + this.setValue(centeredOption.value); + } } }; @@ -416,14 +431,24 @@ export class PickerColumn implements ComponentInterface { * handler (a Save button, for example) reads it. */ private onPointerDownOutside = (ev: Event) => { - const { centeredOption } = this; + const { centeredOption, parentEl, scrollEl } = this; + + if (!this.isScrolling) { + return; + } /** * The column can live in another component's Shadow DOM, where a document * listener sees the target retargeted to the outer host. The composed path * is the only reliable way to tell a press on the column apart. */ - if (!this.isScrolling || ev.composedPath().includes(this.el)) { + const path = ev.composedPath(); + + /** + * A press anywhere in the parent picker counts as inside, so reaching for a + * sibling wheel leaves this column coasting. + */ + if (path.includes(this.el) || (parentEl != null && path.includes(parentEl))) { return; } @@ -435,6 +460,7 @@ export class PickerColumn implements ComponentInterface { */ if (centeredOption !== undefined && centeredOption.isConnected) { this.centerPickerItemInView(centeredOption, false, false); + this.haltedAtScrollTop = scrollEl?.scrollTop; } this.settle(); @@ -448,7 +474,6 @@ export class PickerColumn implements ComponentInterface { */ private initializeScrollListener = () => { this.enableHaptics = isPlatform('ios'); - this.isTrackingScroll = true; const { el, scrollEl } = this; let activeEl: HTMLIonPickerColumnOptionElement | undefined = this.activeItem; @@ -456,26 +481,39 @@ export class PickerColumn implements ComponentInterface { const scrollCallback = () => { raf(() => { /** - * This frame cannot be cancelled, so a scroll that arrived just before - * the column stopped tracking would otherwise re-arm it. + * This frame cannot be cancelled, and moving the column disconnects it + * without the observer reporting a change, so this checks live state. */ - if (!this.isTrackingScroll || !scrollEl) return; + if (!this.el.isConnected || !this.isColumnVisible || !scrollEl) return; + + const { haltedAtScrollTop } = this; + + if (haltedAtScrollTop !== undefined) { + /** + * Still at the halted position, so this frame is the halt landing + * rather than the wheel moving, and re-arming would start a second + * commit cycle. + */ + if (Math.abs(scrollEl.scrollTop - haltedAtScrollTop) <= HALT_TOLERANCE) return; + + this.haltedAtScrollTop = undefined; + } // Armed before the early returns below, so a scroll never ends uncommitted. - this.restartScrollEndTimeout(); + this.resetScrollEndTimeout(); if (!this.isScrolling) { this.enableHaptics && hapticSelectionStart(); this.isScrolling = true; + } - /** - * Only a scroll the user drove represents an uncommitted selection. - * A scroll the column started itself is already heading for the value - * that was set, so an outside press must not cut it short. - */ - if (this.isUserScroll) { - this.watchForOutsidePress(); - } + /** + * Only a user-driven scroll holds an uncommitted selection. Checked every + * frame because the user can take hold of the wheel mid-scroll, and + * re-adding the same listener is a no-op. + */ + if (this.isUserScroll) { + this.watchForOutsidePress(); } /** @@ -604,6 +642,9 @@ export class PickerColumn implements ComponentInterface { const userScrollCallback = () => { this.isUserScroll = true; + + // Taking hold of the wheel ends the halt even if it has not moved yet. + this.haltedAtScrollTop = undefined; }; /** @@ -616,11 +657,18 @@ export class PickerColumn implements ComponentInterface { scrollEl.addEventListener('scroll', scrollCallback); scrollEl.addEventListener('pointerdown', userScrollCallback); scrollEl.addEventListener('wheel', userScrollCallback, { passive: true }); + /** + * A `pointerdown` fires once per touch, so a drag that pauses long enough + * to commit would look like a scroll the column started. A `touchmove` + * keeps arriving while the finger moves. + */ + scrollEl.addEventListener('touchmove', userScrollCallback, { passive: true }); this.destroyScrollListener = () => { scrollEl.removeEventListener('scroll', scrollCallback); scrollEl.removeEventListener('pointerdown', userScrollCallback); scrollEl.removeEventListener('wheel', userScrollCallback); + scrollEl.removeEventListener('touchmove', userScrollCallback); }; }); }; @@ -845,6 +893,15 @@ const PICKER_ITEM_ACTIVE_CLASS = 'option-active'; /** * How long the column must be idle before the centered option is treated as - * the user's selection. + * the user's selection. Replaceable by the `scrollend` event once that is + * supported everywhere (https://caniuse.com/?search=scrollend). */ const SCROLL_END_DELAY = 250; + +/** + * How far, in pixels, the column may report from where it was halted and still + * count as having stayed there. Centering targets a position between two snap + * points, so mandatory snapping corrects it by half the column height less + * three option heights: 2px at the default sizes. + */ +const HALT_TOLERANCE = 2; diff --git a/core/src/components/picker-column/test/scroll/index.html b/core/src/components/picker-column/test/scroll/index.html new file mode 100644 index 00000000000..d5727968f73 --- /dev/null +++ b/core/src/components/picker-column/test/scroll/index.html @@ -0,0 +1,74 @@ + + + + + Picker Column - Scroll + + + + + + + + + + + + + Picker Column - Scroll + + + + + + + + + + Save + +
Flick the wheel, then tap Save mid-scroll.
+ + + + + + diff --git a/core/src/components/picker-column/test/scroll/picker-column.e2e.ts b/core/src/components/picker-column/test/scroll/picker-column.e2e.ts index 6929bfb727a..000b99a5fe6 100644 --- a/core/src/components/picker-column/test/scroll/picker-column.e2e.ts +++ b/core/src/components/picker-column/test/scroll/picker-column.e2e.ts @@ -1,7 +1,18 @@ import { expect } from '@playwright/test'; -import type { E2EPage } from '@utils/test/playwright'; +import type { E2EPage, E2ELocator } from '@utils/test/playwright'; import { configs, test } from '@utils/test/playwright'; +/** The idle period the column waits out before it commits the centered option. */ +const SCROLL_END_DELAY = 250; + +/** Long enough that a pending commit has either landed or is never coming. */ +const COMMIT_WINDOW = SCROLL_END_DELAY + 350; + +interface SaveRecord { + value: string; + highlighted: string; +} + /** * This behavior does not vary across modes/directions. */ @@ -26,8 +37,8 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => await page.locator('ion-picker-column-option.option-active').waitFor(); await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + const column = document.querySelector('ion-picker-column')!; + const scrollEl = column.shadowRoot!.querySelector('.picker-opts')!; const w = window as any; w.lastScrollAt = 0; @@ -37,23 +48,26 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => /** * Records the value at the moment the click handler ran, alongside the - * option the user could actually see under the highlight. + * option the user could see under the highlight. */ document.querySelector('#save')!.addEventListener('click', () => { w.onSave = { - value: col.value, - highlighted: col.querySelector('.option-active')?.value ?? null, - msSinceScroll: performance.now() - w.lastScrollAt, + value: String(column.value), + highlighted: String(column.querySelector('.option-active')?.value ?? ''), }; }); - - w.startScroll = () => scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); }); }); + const startScroll = (page: E2EPage) => + page.evaluate(() => { + const scrollEl = document.querySelector('ion-picker-column')!.shadowRoot!.querySelector('.picker-opts')!; + scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + }); + /** * Presses the column the way a drag would, so the scroll that follows counts - * as the user's. Pressing dead centre lands on the option already selected, + * as the user's. Pressing dead center lands on the option already selected, * so the press itself does not change the value. */ const pressColumn = async (page: E2EPage) => { @@ -64,42 +78,132 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => }; /** - * Clicks the Save button once the column has visibly moved off its starting - * option but is still scrolling towards its resting place. + * The race needs the highlight off the committed value, so the column is + * showing one option while reporting another. */ - const clickSaveMidScroll = async (page: E2EPage) => { - await pressColumn(page); - await page.evaluate(() => (window as any).startScroll()); - - await page.waitForFunction( + const waitForMidScroll = (page: E2EPage) => + page.waitForFunction( () => { - const col = document.querySelector('ion-picker-column') as any; - const highlighted = col.querySelector('.option-active'); + const highlighted = document.querySelector( + 'ion-picker-column .option-active' + ); + const column = document.querySelector('ion-picker-column')!; const isScrolling = performance.now() - (window as any).lastScrollAt < 100; - return highlighted !== null && highlighted.value !== '5' && isScrolling; + return highlighted !== null && String(highlighted.value) !== String(column.value) && isScrolling; }, undefined, { timeout: 5000 } ); - await page.locator('#save').click(); + /** Presses an element with `pointerdown` before `click`, the order a tap uses. */ + const press = (page: E2EPage, selector: string) => + page.evaluate((selector) => { + const target = document.querySelector(selector)!; + target.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, composed: true })); + target.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + }, selector); + + /** + * Presses the given element in the same frame the column is first seen + * mid-scroll. The polling and the press share one evaluate so no round trip + * can let the scroll finish first, which would leave nothing to race. + * + * Passing `past` waits for the highlight to get beyond a given option, for + * when the column already reports the option it is traveling towards. + */ + const pressWhenMidScroll = (page: E2EPage, selector: string, past?: number) => + page.evaluate( + ({ selector, past }) => + new Promise((resolve, reject) => { + const column = document.querySelector('ion-picker-column')!; + const deadline = performance.now() + 5000; + + const isMidScroll = () => { + const highlighted = column.querySelector('.option-active'); + + if (highlighted === null || performance.now() - (window as any).lastScrollAt >= 100) { + return false; + } + + return past === undefined + ? String(highlighted.value) !== String(column.value) + : Number(highlighted.value) > past; + }; + + const poll = () => { + if (isMidScroll()) { + const target = document.querySelector(selector)!; + target.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, composed: true })); + target.dispatchEvent(new MouseEvent('click', { bubbles: true, composed: true })); + resolve(); + } else if (performance.now() > deadline) { + reject(new Error('the column never reached the expected mid-scroll state')); + } else { + requestAnimationFrame(poll); + } + }; + + requestAnimationFrame(poll); + }), + { selector, past } + ); + + const highlightedValue = (page: E2EPage) => + page.evaluate(() => { + const highlighted = document.querySelector( + 'ion-picker-column .option-active' + ); + return highlighted === null ? null : Number(highlighted.value); + }); + + /** + * Presses Save once the column has visibly moved off the option it is + * reporting but is still scrolling towards its resting place. + */ + const pressSaveMidScroll = async (page: E2EPage): Promise => { + await pressColumn(page); + await startScroll(page); + await pressWhenMidScroll(page, '#save'); return await page.evaluate(() => (window as any).onSave); }; + /** + * Stands in for an overscroll bounce leaving the empty padding rows under + * the highlight. Scrolling onto the padding cannot produce this reliably, + * because snapping pulls the column straight back. + */ + const hideOptionsFromHitTesting = (page: E2EPage) => + page.evaluate(() => { + document.querySelectorAll('ion-picker-column-option').forEach((option) => { + option.style.pointerEvents = 'none'; + }); + }); + + const dispatchScroll = (page: E2EPage) => + page.evaluate(() => { + const scrollEl = document.querySelector('ion-picker-column')!.shadowRoot!.querySelector('.picker-opts')!; + scrollEl.dispatchEvent(new Event('scroll')); + }); + + const waitForColumnIdle = (page: E2EPage) => + page.waitForFunction((delay) => performance.now() - (window as any).lastScrollAt > delay, COMMIT_WINDOW, { + timeout: 10000, + }); + + /** Drains the rAF the column schedules to react to a scroll. */ + const flushAnimationFrames = (page: E2EPage) => + page.evaluate( + () => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(() => resolve()))) + ); + test('should commit the visible option before an outside click handler runs', async ({ page }, testInfo) => { testInfo.annotations.push({ type: 'issue', description: 'https://github.com/ionic-team/ionic-framework/issues/30449', }); - const onSave = await clickSaveMidScroll(page); - - /** - * Guards against a false pass: if the column had already stopped - * scrolling then this test is not exercising the race at all. - */ - expect(onSave.msSinceScroll).toBeLessThan(100); + const onSave = await pressSaveMidScroll(page); expect(onSave.highlighted).not.toBe('5'); @@ -107,45 +211,101 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => expect(onSave.value).toBe(onSave.highlighted); }); + /** + * Tapping an option part way through a flick replaces the selection, so an + * outside press during the scroll to it must not commit an option on the way. + */ + test('should keep a selection made mid-flick when Save is pressed', async ({ page }) => { + await pressColumn(page); + await startScroll(page); + await waitForMidScroll(page); + + // Stands in for tapping an option while the wheel is still coasting. + await page.locator('ion-picker-column').evaluate((column: HTMLIonPickerColumnElement) => column.setValue('150')); + await pressWhenMidScroll(page, '#save'); + + const onSave: SaveRecord = await page.evaluate(() => (window as any).onSave); + + expect(onSave.value).toBe('150'); + + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '150'); + }); + + /** + * Taking hold of the wheel mid-scroll does not start a new scroll, so the + * column has to notice the user part way through one it is already running. + */ + test('should commit the visible option when the user takes over a scroll in progress', async ({ page }) => { + await page.locator('ion-picker-column').evaluate((column: HTMLIonPickerColumnElement) => { + column.value = '150'; + }); + + // Wait until the column is genuinely in transit, past 5 but not yet at 150. + await page.waitForFunction( + () => { + const highlighted = document.querySelector( + 'ion-picker-column .option-active' + ); + const value = highlighted === null ? null : Number(highlighted.value); + return value !== null && value > 5 && value < 150; + }, + undefined, + { timeout: 5000 } + ); + + // Stands in for grabbing the wheel while it is still traveling. + await page.evaluate(() => { + const scrollEl = document.querySelector('ion-picker-column')!.shadowRoot!.querySelector('.picker-opts')!; + scrollEl.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, composed: true })); + scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); + }); + + // Press once the user's own scroll has carried it past where it was headed. + await pressWhenMidScroll(page, '#save', 150); + + const onSave: SaveRecord = await page.evaluate(() => (window as any).onSave); + + expect(onSave.value).toBe(onSave.highlighted); + }); + test('should not move on past the option it committed to an outside click', async ({ page }) => { - const onSave = await clickSaveMidScroll(page); + const onSave = await pressSaveMidScroll(page); - // Give any residual momentum and the pending commit time to resolve. - await page.waitForTimeout(600); + /** + * Waits for the column to stop rather than a fixed window. If the press had + * not halted the momentum, the scroll would run on for about a second and + * commit a later option. + */ + await waitForColumnIdle(page); await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', onSave.value); }); /** - * An overscroll bounce can briefly leave the column's empty padding rows - * under the highlight instead of an option. That frame must not throw away - * the current selection or the pending commit. + * The halt is the column scrolling itself, so the scroll events it produces + * must not be read as the wheel still moving and start a second commit. */ - test('should keep its selection when a scroll frame has no option centered', async ({ page }) => { - await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + test('should emit one change for a scroll ended by an outside click', async ({ page }) => { + const ionChange = await page.spyOnEvent('ionChange'); - /** - * Snapping would otherwise pull the column straight back onto an - * option, which is what hides this in a normal scroll. - */ - scrollEl.style.scrollSnapType = 'none'; - scrollEl.scrollTop = 0; - scrollEl.dispatchEvent(new Event('scroll')); - }); + const onSave = await pressSaveMidScroll(page); - // Long enough that any pending commit has resolved. - await page.waitForTimeout(400); + await waitForColumnIdle(page); - await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); + expect(ionChange).toHaveReceivedEventTimes(1); + expect(ionChange).toHaveReceivedEventDetail({ value: onSave.value }); + }); - const { value, highlighted } = await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - return { value: col.value, highlighted: col.querySelector('.option-active')?.value ?? null }; - }); + /** + * A frame with no option centered must leave the highlight where it is. + */ + test('should keep the highlight when a scroll frame has no option centered', async ({ page }) => { + await hideOptionsFromHitTesting(page); + await dispatchScroll(page); + await page.waitForTimeout(COMMIT_WINDOW); - expect(value).toBe(highlighted); + await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); + await expect(page.locator('ion-picker-column-option.option-active')).toHaveJSProperty('value', '5'); }); /** @@ -153,38 +313,88 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => * option that was already centered, and must still commit it. */ test('should commit the last centered option when a later frame has none', async ({ page }) => { - await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const scrollEl = col.shadowRoot.querySelector('.picker-opts'); - const option = col.querySelectorAll('ion-picker-column-option')[10]; + await pressColumn(page); + await startScroll(page); + await waitForMidScroll(page); - /** - * Snapping would otherwise pull the column back onto an option, which is - * what hides this in a normal scroll. - */ - scrollEl.style.scrollSnapType = 'none'; + /** + * From here no frame can center an option, so whatever was centered last is + * what the column falls back on. Read it after the scroll rAF has drained + * so an already queued frame cannot move it afterwards. + */ + await hideOptionsFromHitTesting(page); + await flushAnimationFrames(page); - option.scrollIntoView({ block: 'center' }); - scrollEl.dispatchEvent(new Event('scroll')); - }); + const centered = await page + .locator('ion-picker-column-option.option-active') + .evaluate((option: HTMLIonPickerColumnOptionElement) => String(option.value)); + expect(centered).not.toBe('5'); + + await dispatchScroll(page); + + /** + * The scroll is still coasting, and every frame it produces now finds no + * option, so the commit lands once it stops. + */ + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', centered); + + await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); + await expect(page.locator('ion-picker-column-option.option-active')).toHaveJSProperty('value', centered); + }); + + /** + * Once a commit has landed, a later scroll that never centers an option must + * not fall back on it. + */ + test('should not commit an option left over from a finished scroll', async ({ page }) => { + await pressColumn(page); + await startScroll(page); + await waitForColumnIdle(page); - // Let the column register option 10 as centered. - await expect(page.locator('ion-picker-column-option.option-active')).toHaveJSProperty('value', '10'); + // The scroll ran to the end of the column, so this is where it settled. + const committed = await highlightedValue(page); + expect(committed).toBe(199); + // From here the column can never find an option under the highlight again. + await hideOptionsFromHitTesting(page); + + await pressColumn(page); await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const scrollEl = col.shadowRoot.querySelector('.picker-opts'); + const scrollEl = document.querySelector('ion-picker-column')!.shadowRoot!.querySelector('.picker-opts')!; + scrollEl.scrollTo({ top: 0, behavior: 'smooth' }); + }); + await press(page, '#save'); - // Now put the empty padding rows under the highlight. - scrollEl.scrollTop = 0; - scrollEl.dispatchEvent(new Event('scroll')); + /** + * Falling back on the leftover option would have halted this scroll and + * dragged the column back to the far end, so it would never arrive. + */ + await page.waitForFunction( + () => document.querySelector('ion-picker-column')!.shadowRoot!.querySelector('.picker-opts')!.scrollTop < 50, + undefined, + { timeout: 5000 } + ); + + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', String(committed)); + }); + + /** + * A keyed list reorder moves a column rather than replacing it, which + * disconnects and reconnects it without the visibility observer reporting a + * change. The column has to keep reacting to scrolls afterwards. + */ + test('should still track scrolling after the column is moved', async ({ page }) => { + await page.evaluate(() => { + const host = document.createElement('div'); + document.body.appendChild(host); + host.appendChild(document.querySelector('ion-picker')!); }); - // Long enough that any pending commit has resolved. - await page.waitForTimeout(400); + await startScroll(page); - await expect(page.locator('ion-picker-column-option.option-active')).toHaveCount(1); - await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '10'); + // The last option, since the scroll runs to the end of the column. + await expect(page.locator('ion-picker-column-option.option-active')).toHaveJSProperty('value', '199'); + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '199'); }); /** @@ -192,124 +402,161 @@ configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => * it can land after the column has already been torn down. */ test('should not commit a value after the column is removed mid-scroll', async ({ page }) => { - const changes = await page.evaluate(async () => { - const col = document.querySelector('ion-picker-column') as any; - const picker = document.querySelector('ion-picker')!; - const scrollEl = col.shadowRoot.querySelector('.picker-opts'); - const recorded: unknown[] = []; + /** + * Spied on the column rather than the page because the picker is removed + * before the commit would fire, and an event on a detached element never + * reaches the page. + */ + const ionChange = await (page.locator('ion-picker-column') as E2ELocator).spyOnEvent('ionChange'); - col.addEventListener('ionChange', (ev: any) => recorded.push(ev.detail.value)); + await page.evaluate(() => { + const column = document.querySelector('ion-picker-column')!; + const scrollEl = column.shadowRoot!.querySelector('.picker-opts')!; /** * The column registered its own scroll listener first, so by the time * this one runs the column has already queued the frame that reacts to - * this scroll. Removing the column here leaves that frame pending. Wait + * this scroll. Removing the picker here leaves that frame pending. Wait * a few scrolls first so the column has centered an option to commit. */ let scrolls = 0; scrollEl.addEventListener('scroll', () => { if (++scrolls === 5) { - picker.remove(); + document.querySelector('ion-picker')!.remove(); } }); scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); - - // Long enough that any pending commit has resolved. - await new Promise((resolve) => setTimeout(resolve, 600)); - - return recorded; }); - expect(changes).toEqual([]); + await page.waitForTimeout(COMMIT_WINDOW); + + expect(ionChange).not.toHaveReceivedEvent(); }); /** - * A scroll the column starts itself is not a selection the user has made, so - * an outside press during one must not freeze it or commit an option it is - * only passing through. + * A scroll the column starts itself is not a selection the user made, so an + * outside press during one must not freeze it. */ test('should not commit an option that a programmatic scroll is passing through', async ({ page }) => { - await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const w = window as any; - - w.changes = []; - col.addEventListener('ionChange', (ev: any) => w.changes.push(ev.detail.value)); + const ionChange = await page.spyOnEvent('ionChange'); - col.value = '150'; + await page.locator('ion-picker-column').evaluate((column: HTMLIonPickerColumnElement) => { + column.value = '150'; }); - // Wait until the column is part way to the option that was just set. - await page.waitForFunction( - () => { - const highlighted = document.querySelector('ion-picker-column .option-active') as any; - return highlighted !== null && highlighted.value !== '5' && highlighted.value !== '150'; - }, - undefined, - { timeout: 5000 } - ); - - await page.locator('#save').click(); - - // Long enough that any pending commit has resolved. - await page.waitForTimeout(600); + await pressWhenMidScroll(page, '#save'); - const result = await page.evaluate(() => ({ - value: (document.querySelector('ion-picker-column') as any).value, - changes: (window as any).changes, - })); + await page.waitForTimeout(COMMIT_WINDOW); - // Setting the value property must not emit ionChange. - expect(result.changes).toEqual([]); + // Setting the `value` property must not emit `ionChange`. + expect(ionChange).not.toHaveReceivedEvent(); // The column must still be headed for the option the application asked for. - expect(result.value).toBe('150'); + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '150'); }); /** * Selecting an option directly scrolls the column to it. That scroll belongs * to the selection the user already made, so an outside press during it must - * not redirect the value to an option on the way. + * not redirect the value. */ test('should not commit an option that a selected scroll is passing through', async ({ page }) => { - await page.evaluate(() => { - const col = document.querySelector('ion-picker-column') as any; - const w = window as any; - - w.changes = []; - col.addEventListener('ionChange', (ev: any) => w.changes.push(ev.detail.value)); - }); + const ionChange = await page.spyOnEvent('ionChange'); // A press that selects the option already under the highlight, so nothing scrolls yet. await pressColumn(page); // Stands in for tapping an option far down the column. - await page.evaluate(() => (document.querySelector('ion-picker-column') as any).setValue('150')); + await page.locator('ion-picker-column').evaluate((column: HTMLIonPickerColumnElement) => column.setValue('150')); - // Wait until the column is part way to the option that was selected. - await page.waitForFunction( - () => { - const highlighted = document.querySelector('ion-picker-column .option-active') as any; - return highlighted !== null && highlighted.value !== '5' && highlighted.value !== '150'; - }, - undefined, - { timeout: 5000 } + await pressWhenMidScroll(page, '#save'); + + await page.waitForTimeout(COMMIT_WINDOW); + + // Only the selection itself is committed, not an option on the way to it. + expect(ionChange).toHaveReceivedEventTimes(1); + expect(ionChange).toHaveReceivedEventDetail({ value: '150' }); + await expect(page.locator('ion-picker-column')).toHaveJSProperty('value', '150'); + }); + }); +}); + +/** + * This behavior does not vary across modes/directions. + */ +configs({ modes: ['ios'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('picker-column: sibling columns'), () => { + /** + * A press inside the picker does not read a coasting column's value, so it + * must not stop it on an option the user was only scrolling past. + */ + test('should keep coasting when another column in the picker is pressed', async ({ page }) => { + await page.setContent( + ` + + + ${Array.from( + { length: 200 }, + (_, i) => `${i}` + ).join('')} + + + a + b + + + `, + config ); - await page.locator('#save').click(); + await page.locator('.first ion-picker-column-option.option-active').waitFor(); + + await page.evaluate(() => { + const scrollEl = document.querySelector('.first')!.shadowRoot!.querySelector('.picker-opts')!; + (window as any).lastScrollAt = 0; + scrollEl.addEventListener('scroll', () => { + (window as any).lastScrollAt = performance.now(); + }); + }); + + const first = page.locator('.first'); + const box = (await first.boundingBox())!; + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.up(); - // Long enough that any pending commit has resolved. - await page.waitForTimeout(600); + // Press the sibling in the same frame the first column is seen mid-flick. + await page.evaluate(() => { + const scrollEl = document.querySelector('.first')!.shadowRoot!.querySelector('.picker-opts')!; + scrollEl.scrollTo({ top: scrollEl.scrollHeight, behavior: 'smooth' }); - const result = await page.evaluate(() => ({ - value: (document.querySelector('ion-picker-column') as any).value, - changes: (window as any).changes, - })); + return new Promise((resolve, reject) => { + const column = document.querySelector('.first')!; + const deadline = performance.now() + 5000; + + const poll = () => { + const highlighted = column.querySelector('.option-active'); + const isScrolling = performance.now() - (window as any).lastScrollAt < 100; + + if (highlighted !== null && String(highlighted.value) !== String(column.value) && isScrolling) { + const sibling = document.querySelector('.second')!; + sibling.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true, composed: true })); + resolve(); + } else if (performance.now() > deadline) { + reject(new Error('the first column never reached the expected mid-scroll state')); + } else { + requestAnimationFrame(poll); + } + }; - // Only the selection itself is committed, not an option on the way to it. - expect(result.changes).toEqual(['150']); - expect(result.value).toBe('150'); + requestAnimationFrame(poll); + }); + }); + + // The scroll was headed for the end of the column, so that is where it belongs. + await expect(first).toHaveJSProperty('value', '199'); + await expect(page.locator('.first ion-picker-column-option.option-active')).toHaveJSProperty('value', '199'); }); }); });