From ebecac2d9ed4b18fcd3e3a0f918f5f42d3cb1c56 Mon Sep 17 00:00:00 2001 From: m2rt Date: Wed, 16 Sep 2026 11:42:49 +0300 Subject: [PATCH 1/3] fix(time-picker): correct item height and ignore engine-driven scrolls #678 Co-Authored-By: Claude Opus 5 (1M context) --- .../time-picker/time-picker.component.spec.ts | 491 ++++++++++++++++++ .../form/time-picker/time-picker.component.ts | 286 ++++++++-- 2 files changed, 742 insertions(+), 35 deletions(-) diff --git a/src/tedi/components/form/time-picker/time-picker.component.spec.ts b/src/tedi/components/form/time-picker/time-picker.component.spec.ts index d573d2575..365aea107 100644 --- a/src/tedi/components/form/time-picker/time-picker.component.spec.ts +++ b/src/tedi/components/form/time-picker/time-picker.component.spec.ts @@ -779,6 +779,8 @@ describe("TimePickerComponent", () => { value: 240, configurable: true, }); + // The wheel gesture that scrolled the column. + hourColumn.dispatchEvent(new Event("wheel", { bubbles: true })); hourColumn.dispatchEvent(new Event("scroll")); jest.runAllTimers(); @@ -794,6 +796,7 @@ describe("TimePickerComponent", () => { value: 100, configurable: true, }); + hourColumn.dispatchEvent(new Event("wheel", { bubbles: true })); hourColumn.dispatchEvent(new Event("scroll")); const clearSpy = jest.spyOn(global, "clearTimeout"); @@ -805,6 +808,494 @@ describe("TimePickerComponent", () => { } }); }); + + describe("host gesture listeners", () => { + it("should detach every gesture listener it attached on destroy", () => { + const localFixture = TestBed.createComponent(TimePickerComponent); + const localHost = localFixture.nativeElement as HTMLElement; + const addSpy = jest.spyOn(localHost, "addEventListener"); + const removeSpy = jest.spyOn(localHost, "removeEventListener"); + + localFixture.detectChanges(); + + const attached = addSpy.mock.calls.filter(([name]) => + ["wheel", "touchstart", "pointerdown"].includes(name), + ); + expect(attached.map(([name]) => name)).toEqual( + expect.arrayContaining(["wheel", "touchstart", "pointerdown"]), + ); + + localFixture.destroy(); + + // The exact same function reference and options object have to come back + // off the host, which is what the arrow-function handler property buys. + for (const [name, listener, options] of attached) { + expect(removeSpy).toHaveBeenCalledWith(name, listener, options); + } + }); + }); + + describe("programmatic scroll echo", () => { + const SELECTED_CLASS = "tedi-time-picker__item--selected"; + + // jsdom has no layout, so scrollTop is a read-only 0 and measureItemHeight() + // falls back to DEFAULT_ITEM_HEIGHT (40). A writable stub lets a test place + // the column at an offset that rounds to a different row than the one the + // component scrolled to. + const stubScrollTop = (element: HTMLElement): void => { + let current = 0; + Object.defineProperty(element, "scrollTop", { + configurable: true, + get: () => current, + set: (next: number) => { + current = next; + }, + }); + }; + + const columnAt = (index: number): HTMLElement => + el.querySelectorAll(".tedi-time-picker__column")[index]; + + const itemsIn = (column: HTMLElement): NodeListOf => + column.querySelectorAll(".tedi-time-picker__item"); + + // A user scroll is a gesture followed by the scroll it produced. An engine + // scroll (Gecko handing a recreated column the offset of the one it + // replaced) arrives with no gesture in front of it. + const userScrollTo = (column: HTMLElement, top: number): void => { + column.dispatchEvent(new Event("wheel", { bubbles: true })); + column.scrollTop = top; + column.dispatchEvent(new Event("scroll")); + }; + + const engineScrollTo = (column: HTMLElement, top: number): void => { + column.scrollTop = top; + column.dispatchEvent(new Event("scroll")); + }; + + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("should keep the programmatic index when an instant scroll echo rounds to another item", () => { + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(9); + + // The column settled one row away from the offset the component asked for + // (384 / 40 rounds to 10). The echo of an instant programmatic scroll must + // not move the highlight off the authoritative row. + hourColumn.scrollTop = 384; + hourColumn.dispatchEvent(new Event("scroll")); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(9); + + const hourItems = itemsIn(hourColumn); + expect(hourItems[9].classList.contains(SELECTED_CLASS)).toBe(true); + expect(hourItems[10].classList.contains(SELECTED_CLASS)).toBe(false); + }); + + it("should not commit a snap-adjustment echo that lands after the scroll lock expires", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + expect(hourColumn.scrollTop).toBe(9 * 40); + + // An echo of the programmatic scroll, delivered after the old fixed 50 ms + // lock would have expired: with no gesture behind it and an offset the + // scroll never asked for, it must not become the value. + jest.advanceTimersByTime(60); + + hourColumn.scrollTop = 560; + hourColumn.dispatchEvent(new Event("scroll")); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("should not commit a snap-adjustment echo that trickles in over several frames", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + // A slow machine can stretch the settling well past any fixed window, so + // each event has to keep the scroll marked programmatic. + for (const offset of [420, 470, 520, 560, 600]) { + jest.advanceTimersByTime(120); + hourColumn.scrollTop = offset; + hourColumn.dispatchEvent(new Event("scroll")); + } + + jest.advanceTimersByTime(1000); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(onChange).not.toHaveBeenCalled(); + }); + + it("should still commit a user scroll that follows a settled programmatic scroll", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + // Everything the programmatic scroll caused has been delivered and the + // column has been still since; the wheel is the user's again. + jest.advanceTimersByTime(1000); + + userScrollTo(hourColumn, 15 * 40); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("15:30"); + expect(onChange).toHaveBeenCalledWith("15:30"); + expect(component.highlightedHourIndex()).toBe(15); + }); + + it("should put the column back when the engine scrolls it with no gesture", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + jest.advanceTimersByTime(1000); + + // Gecko drops the previous picker's offset onto this column. + engineScrollTo(hourColumn, 14 * 40); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(onChange).not.toHaveBeenCalled(); + expect(hourColumn.scrollTop).toBe(9 * 40); + expect(component.highlightedHourIndex()).toBe(9); + }); + + it("should put the column back when the engine scrolls it while the programmatic scroll is settling", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + // Inside the settle window of the alignment that the value triggered. + jest.advanceTimersByTime(20); + engineScrollTo(hourColumn, 14 * 40); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(onChange).not.toHaveBeenCalled(); + expect(hourColumn.scrollTop).toBe(9 * 40); + expect(component.highlightedHourIndex()).toBe(9); + }); + + it("should leave a snap adjustment of the programmatic scroll alone", () => { + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + // Within one row of the offset asked for, which is as far as a snap + // adjustment can move the column. + jest.advanceTimersByTime(60); + engineScrollTo(hourColumn, 9 * 40 + 24); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(hourColumn.scrollTop).toBe(9 * 40 + 24); + expect(component.highlightedHourIndex()).toBe(9); + }); + + it("should track the highlight live while a smooth programmatic scroll runs", () => { + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + itemsIn(hourColumn)[15].click(); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(15); + + // Mid-animation position of the smooth scroll started by the click. + hourColumn.scrollTop = 480; + hourColumn.dispatchEvent(new Event("scroll")); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(12); + expect(itemsIn(hourColumn)[12].classList.contains(SELECTED_CLASS)).toBe( + true, + ); + }); + + it("should re-align the column for a new value while the previous scroll is still settling", () => { + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + expect(hourColumn.scrollTop).toBe(9 * 40); + + // Still inside the settle window of the first scroll. + jest.advanceTimersByTime(20); + + component.writeValue("17:30"); + fixture.detectChanges(); + + expect(hourColumn.scrollTop).toBe(17 * 40); + expect(component.highlightedHourIndex()).toBe(17); + }); + + it("should re-align the columns when the measured item height changes", () => { + let notifyResize: (() => void) | undefined; + class ResizeObserverMock { + constructor(callback: () => void) { + notifyResize = callback; + } + observe(): void {} + unobserve(): void {} + disconnect(): void {} + } + const originalResizeObserver = globalThis.ResizeObserver; + globalThis.ResizeObserver = + ResizeObserverMock as unknown as typeof ResizeObserver; + + try { + const localFixture = TestBed.createComponent(TimePickerComponent); + const localEl = localFixture.nativeElement as HTMLElement; + localFixture.detectChanges(); + + const hourColumn = localEl.querySelectorAll( + ".tedi-time-picker__column", + )[0]; + stubScrollTop(hourColumn); + + localFixture.componentInstance.writeValue("09:30"); + localFixture.detectChanges(); + + // Measured through the DEFAULT_ITEM_HEIGHT fallback jsdom forces. + expect(hourColumn.scrollTop).toBe(9 * 40); + + const item = localEl.querySelector( + ".tedi-time-picker__item", + )!; + Object.defineProperty(item, "offsetHeight", { + value: 48, + configurable: true, + }); + notifyResize?.(); + + expect(hourColumn.scrollTop).toBe(9 * 48); + expect(localFixture.componentInstance.highlightedHourIndex()).toBe(9); + } finally { + globalThis.ResizeObserver = originalResizeObserver; + } + }); + + // Every gesture in the suite used to be a wheel, so dropping "touchstart" + // or "pointerdown" from the gesture list changed nothing. A scroll that + // any of them started has to be read as the user's. + const expectGestureScrollCommits = (gesture: string): void => { + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + jest.advanceTimersByTime(1000); + + hourColumn.dispatchEvent(new Event(gesture, { bubbles: true })); + hourColumn.scrollTop = 15 * 40; + hourColumn.dispatchEvent(new Event("scroll")); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(15); + expect(component.value()).toBe("15:30"); + }; + + it("should commit a scroll that a touchstart started", () => { + expectGestureScrollCommits("touchstart"); + }); + + it("should commit a scroll that a pointerdown started", () => { + expectGestureScrollCommits("pointerdown"); + }); + + it("should commit the user's scroll when it starts while a smooth programmatic scroll is running", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + itemsIn(hourColumn)[15].click(); + fixture.detectChanges(); + + expect(component.value()).toBe("15:00"); + + // The wheel starts spinning before the smooth scroll to 15 has finished. + // The user owns the column from the first event, however many follow. + for (const offset of [640, 680, 720]) { + jest.advanceTimersByTime(16); + hourColumn.dispatchEvent(new Event("wheel", { bubbles: true })); + hourColumn.scrollTop = offset; + hourColumn.dispatchEvent(new Event("scroll")); + } + + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(18); + expect(component.value()).toBe("18:00"); + expect(onChange).toHaveBeenLastCalledWith("18:00"); + }); + + it("should not let a gesture that never scrolled anything turn a later engine scroll into a value", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + jest.advanceTimersByTime(1000); + + // A pointer going down on the faded padding at the end of the column: + // it is inside the column, it hits no item, and nothing scrolls. + hourColumn.dispatchEvent(new Event("pointerdown", { bubbles: true })); + jest.advanceTimersByTime(1000); + + // Long afterwards, Gecko drops the previous picker's offset onto this + // column. There is no gesture behind it. + engineScrollTo(hourColumn, 14 * 40); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.value()).toBe("09:30"); + expect(onChange).not.toHaveBeenCalled(); + expect(hourColumn.scrollTop).toBe(9 * 40); + expect(component.highlightedHourIndex()).toBe(9); + }); + + it("should commit a user scroll that lands within one row of the programmatic target", () => { + const onChange = jest.fn(); + component.registerOnChange(onChange); + + const hourColumn = columnAt(0); + stubScrollTop(hourColumn); + + component.writeValue("09:30"); + fixture.detectChanges(); + + // Still inside the settle window of the alignment, and one row is as + // close as a scroll can start to where that alignment parked. + jest.advanceTimersByTime(20); + userScrollTo(hourColumn, 10 * 40); + jest.advanceTimersByTime(200); + fixture.detectChanges(); + + expect(component.highlightedHourIndex()).toBe(10); + expect(component.value()).toBe("10:30"); + expect(onChange).toHaveBeenCalledWith("10:30"); + }); + + it("should keep the cached item height when a resize fires with no layout box", () => { + let notifyResize: (() => void) | undefined; + class ResizeObserverMock { + constructor(callback: () => void) { + notifyResize = callback; + } + observe(): void {} + unobserve(): void {} + disconnect(): void {} + } + const originalResizeObserver = globalThis.ResizeObserver; + globalThis.ResizeObserver = + ResizeObserverMock as unknown as typeof ResizeObserver; + + try { + const localFixture = TestBed.createComponent(TimePickerComponent); + const localEl = localFixture.nativeElement as HTMLElement; + localFixture.detectChanges(); + + const hourColumn = localEl.querySelectorAll( + ".tedi-time-picker__column", + )[0]; + stubScrollTop(hourColumn); + + localFixture.componentInstance.writeValue("09:30"); + localFixture.detectChanges(); + + const item = localEl.querySelector( + ".tedi-time-picker__item", + )!; + Object.defineProperty(item, "offsetHeight", { + value: 48, + configurable: true, + }); + notifyResize?.(); + expect(hourColumn.scrollTop).toBe(9 * 48); + + jest.advanceTimersByTime(1000); + + // The consumer hides the picker, so the observer fires with every box + // collapsed to 0. A row is not 40px tall because it has no box. + Object.defineProperty(item, "offsetHeight", { + value: 0, + configurable: true, + }); + notifyResize?.(); + + expect(hourColumn.scrollTop).toBe(9 * 48); + + // The cached height still has to be the one the rows actually have, + // so the next user scroll reads the right row out of the offset. + hourColumn.dispatchEvent(new Event("wheel", { bubbles: true })); + hourColumn.scrollTop = 15 * 48; + hourColumn.dispatchEvent(new Event("scroll")); + jest.advanceTimersByTime(200); + + expect(localFixture.componentInstance.value()).toBe("15:30"); + } finally { + globalThis.ResizeObserver = originalResizeObserver; + } + }); + }); }); @Component({ diff --git a/src/tedi/components/form/time-picker/time-picker.component.ts b/src/tedi/components/form/time-picker/time-picker.component.ts index c7842f3ae..87549a239 100644 --- a/src/tedi/components/form/time-picker/time-picker.component.ts +++ b/src/tedi/components/form/time-picker/time-picker.component.ts @@ -26,9 +26,32 @@ import { RadioComponent } from "../radio/radio.component"; export type TimePickerVariant = "scroll" | "slots" | "dropdown"; const DEFAULT_ITEM_HEIGHT = 40; -const SMOOTH_SCROLL_LOCK_MS = 400; -const INSTANT_SCROLL_LOCK_MS = 50; +/** Quiet period before a programmatic scroll counts as finished. */ +const SCROLL_SETTLE_QUIET_MS = 200; +/** Releases a column whose programmatic scroll never fired a scroll event. */ +const SCROLL_SETTLE_MAX_WAIT_MS = 1000; const SCROLL_DEBOUNCE_MS = 150; +/** + * How long a gesture stays armed before it has scrolled anything. A pointer can + * go down on a column's padding and never move it, and nothing else would clear + * the flag. Cancelled as soon as the gesture does scroll. + */ +const GESTURE_WITHOUT_SCROLL_MS = 500; +/** + * The gestures that can scroll a column. Scrollbars are hidden and scrolling + * keys are handled in `onColumnKeydown`, so a scroll with none of these behind + * it was not started by the user. + */ +const USER_GESTURE_EVENTS = [ + "wheel", + "touchstart", + "touchmove", + "pointerdown", +] as const; +const USER_GESTURE_OPTIONS: AddEventListenerOptions = { + capture: true, + passive: true, +}; type WheelType = "hour" | "minute"; @@ -92,16 +115,41 @@ export class TimePickerComponent private onTouched: () => void = () => {}; private initialized = false; + /** + * Held until the programmatic scroll has actually settled, not for a fixed + * time: `scrollTo` reports its own events plus the scroll-snap adjustment on + * top, and the engine decides when those arrive. + */ private readonly isProgrammaticScroll: Record = { hour: false, minute: false, }; + private readonly isSmoothProgrammaticScroll: Record = { + hour: false, + minute: false, + }; + /** Only a scroll with a gesture behind it may change the value. */ + private readonly hasUserGesture: Record = { + hour: false, + minute: false, + }; + /** Guards against a correction and the engine trading scrolls indefinitely. */ + private readonly hasCorrectedScroll: Record = { + hour: false, + minute: false, + }; + private readonly programmaticScrollTarget: Partial< + Record + > = {}; private readonly scrollLockTimer: Partial< Record> > = {}; private readonly scrollDebounceTimer: Partial< Record> > = {}; + private readonly gestureTimer: Partial< + Record> + > = {}; private cachedItemHeight: number | null = null; private resizeObserver: ResizeObserver | null = null; @@ -205,28 +253,64 @@ export class TimePickerComponent this.initialized = true; this.cachedItemHeight = this.measureItemHeight(); if (typeof ResizeObserver !== "undefined") { - this.resizeObserver = new ResizeObserver( - () => (this.cachedItemHeight = this.measureItemHeight()), - ); + this.resizeObserver = new ResizeObserver(() => this.syncItemHeight()); this.resizeObserver.observe(this.el.nativeElement as HTMLElement); } + const host = this.el.nativeElement as HTMLElement; + USER_GESTURE_EVENTS.forEach((name) => { + host.addEventListener(name, this.handleUserGesture, USER_GESTURE_OPTIONS); + }); requestAnimationFrame(() => this.alignScroll("instant")); } - private measureItemHeight(): number { + /** + * The height a row is actually rendered at, or 0 when it has no box at all + * (the picker is hidden, or has not been laid out yet). + */ + private measureItemBox(): number { const root = this.el.nativeElement as HTMLElement; const item = root.querySelector( ".tedi-time-picker__item", ) as HTMLElement | null; - return item?.offsetHeight || DEFAULT_ITEM_HEIGHT; + return item?.offsetHeight ?? 0; + } + + private measureItemHeight(): number { + return this.measureItemBox() || DEFAULT_ITEM_HEIGHT; } private getItemHeight(): number { return this.cachedItemHeight ?? this.measureItemHeight(); } + /** + * Column offsets are derived from the item height, so when the height changes + * (a breakpoint switch, or the first real measurement after + * `DEFAULT_ITEM_HEIGHT` stood in) they no longer land on the highlighted row. + */ + private syncItemHeight(): void { + // A hidden element measures 0 and ResizeObserver fires for it, so keep the + // last height actually rendered rather than caching the fallback. + const height = this.measureItemBox(); + if (!height || height === this.cachedItemHeight) return; + + this.cachedItemHeight = height; + if (this.variant() !== "scroll") return; + + this.scrollColumnToIndex("hour", this.hourScrollIndex(), "instant"); + this.scrollColumnToIndex("minute", this.minuteScrollIndex(), "instant"); + } + ngOnDestroy(): void { this.resizeObserver?.disconnect(); + const host = this.el.nativeElement as HTMLElement; + USER_GESTURE_EVENTS.forEach((name) => { + host.removeEventListener( + name, + this.handleUserGesture, + USER_GESTURE_OPTIONS, + ); + }); (Object.keys(this.scrollLockTimer) as WheelType[]).forEach((k) => { const t = this.scrollLockTimer[k]; if (t) clearTimeout(t); @@ -235,6 +319,10 @@ export class TimePickerComponent const t = this.scrollDebounceTimer[k]; if (t) clearTimeout(t); }); + (Object.keys(this.gestureTimer) as WheelType[]).forEach((k) => { + const t = this.gestureTimer[k]; + if (t) clearTimeout(t); + }); } writeValue(value: string | null): void { @@ -288,6 +376,9 @@ export class TimePickerComponent onHourClick(hour: string): void { if (this.isDisabled()) return; + // The pointer went down to pick a row, not to scroll the column. + this.hasUserGesture.hour = false; + this.clearGestureTimer("hour"); const idx = parseInt(hour, 10); this.scrollColumnToIndex("hour", idx, "smooth"); this.selectHour(hour); @@ -296,6 +387,8 @@ export class TimePickerComponent onMinuteClick(minute: string): void { if (this.isDisabled()) return; + this.hasUserGesture.minute = false; + this.clearGestureTimer("minute"); const list = this.minutes(); const idx = list.indexOf(minute); if (idx >= 0) this.scrollColumnToIndex("minute", idx, "smooth"); @@ -376,37 +469,133 @@ export class TimePickerComponent } } + private readonly handleUserGesture = (event: Event): void => { + const target = event.target as Element | null; + const column = target?.closest?.(".tedi-time-picker__column") ?? null; + if (!column) return; + + let type: WheelType | null = null; + if (column === this.getColumnElement("hour")) type = "hour"; + if (column === this.getColumnElement("minute")) type = "minute"; + if (!type) return; + + this.hasUserGesture[type] = true; + this.hasCorrectedScroll[type] = false; + this.armGestureTimer(type); + }; + + /** + * Bounds a gesture that never scrolls. Without it one pointer-down on a + * column's padding would leave it marked user-driven forever, and the next + * offset the engine handed it would be committed as a value. + */ + private armGestureTimer(type: WheelType): void { + this.clearGestureTimer(type); + this.gestureTimer[type] = setTimeout(() => { + this.hasUserGesture[type] = false; + delete this.gestureTimer[type]; + }, GESTURE_WITHOUT_SCROLL_MS); + } + + private clearGestureTimer(type: WheelType): void { + const existing = this.gestureTimer[type]; + if (existing) clearTimeout(existing); + delete this.gestureTimer[type]; + } + + /** + * Precedence, highest first: a gesture on this column (the offset is the + * user's, and it takes over any animation still running), a programmatic + * scroll (its own echo, never re-read as a selection), or neither, which + * means the engine moved it and the column goes back on the value's row. + */ onColumnScroll(type: WheelType): void { const column = this.getColumnElement(type); if (!column) return; + if (this.hasUserGesture[type]) { + // The debounce owns the flag from here, not the no-scroll timeout. + this.clearGestureTimer(type); + this.clearProgrammaticScroll(type); + this.setScrollIndex(type, this.indexFromScroll(type, column)); + this.scheduleScrollSelection(type); + return; + } + + if (this.isProgrammaticScroll[type]) { + // Keep the lock open for as long as events keep arriving, so a column + // that is still settling (a snap adjustment on top of the scroll, a slow + // frame) stays owned by the scroll that started it. + this.releaseScrollLockAfter(type, SCROLL_SETTLE_QUIET_MS); + + // A non-smooth programmatic scroll has already set the authoritative + // index. Re-deriving it from scrollTop would move the highlight to a + // neighbouring row whenever the measured item height and the rendered one + // disagree. Smooth ones fall through: their highlight tracks the running + // animation. + if (this.isSmoothProgrammaticScroll[type]) { + this.setScrollIndex(type, this.indexFromScroll(type, column)); + return; + } + + // A snap adjustment settles on the row next to the offset it was given, + // so anything further out is not this scroll finishing. + const target = this.programmaticScrollTarget[type] ?? column.scrollTop; + if (Math.abs(column.scrollTop - target) <= this.getItemHeight()) return; + } + + this.restoreColumnPosition(type); + } + + private indexFromScroll(type: WheelType, column: HTMLElement): number { const list = type === "hour" ? this.hours : this.minutes(); const rawIndex = Math.round(column.scrollTop / this.getItemHeight()); - const index = Math.max(0, Math.min(rawIndex, list.length - 1)); + return Math.max(0, Math.min(rawIndex, list.length - 1)); + } + private setScrollIndex(type: WheelType, index: number): void { if (type === "hour") this.hourScrollIndex.set(index); else this.minuteScrollIndex.set(index); + } - if (this.isProgrammaticScroll[type]) return; - + private scheduleScrollSelection(type: WheelType): void { const existing = this.scrollDebounceTimer[type]; if (existing) clearTimeout(existing); this.scrollDebounceTimer[type] = setTimeout(() => { - const col = this.getColumnElement(type); - if (!col) return; - const finalIdx = Math.max( - 0, - Math.min( - Math.round(col.scrollTop / this.getItemHeight()), - list.length - 1, - ), - ); - if (type === "hour") this.selectHour(list[finalIdx]); - else this.selectMinute(list[finalIdx]); + const column = this.getColumnElement(type); + if (!column) return; + + const list = type === "hour" ? this.hours : this.minutes(); + const index = this.indexFromScroll(type, column); + + // The gesture has produced its value. Whatever scrolls the column next + // has to come with a gesture of its own to be read as a selection. + this.hasUserGesture[type] = false; + this.clearGestureTimer(type); + + if (type === "hour") this.selectHour(list[index]); + else this.selectMinute(list[index]); }, SCROLL_DEBOUNCE_MS); } + /** + * Gecko hands a torn-down scroll container's offset to the one replacing it + * in the same position, so a picker can arrive already scrolled to the + * previous picker's row. Nothing marks that as not-the-user, which is how + * `09:30` came to render and report `14:06`. + */ + private restoreColumnPosition(type: WheelType): void { + if (this.hasCorrectedScroll[type]) return; + + const index = + type === "hour" ? this.selectedHourIndex() : this.selectedMinuteIndex(); + // Only a correction that moved the column spends the one-shot budget. + if (this.scrollColumnToIndex(type, index, "instant")) { + this.hasCorrectedScroll[type] = true; + } + } + /** Roving-tabindex helper for dropdown items: only the selected item (or the first * when nothing is selected yet) is in the tab sequence. */ getDropdownTabIndex(index: number): number { @@ -518,26 +707,36 @@ export class TimePickerComponent const targetHour = this.selectedHourIndex(); const targetMinute = this.selectedMinuteIndex(); + // A fresh alignment is a fresh start for the correction guard. + this.hasCorrectedScroll.hour = false; + this.hasCorrectedScroll.minute = false; + if (behavior !== "smooth") { this.hourScrollIndex.set(targetHour); this.minuteScrollIndex.set(targetMinute); } - if (!this.isProgrammaticScroll.hour) { + // Only a smooth scroll is worth protecting from a realignment: it is still + // animating towards the value that started it, and an instant scrollTo to + // the same row would cut that animation short. An instant scroll has + // already arrived, so a new value must be allowed to move the column even + // while the column is settling. + if (!this.isSmoothProgrammaticScroll.hour) { this.scrollColumnToIndex("hour", targetHour, behavior); } - if (!this.isProgrammaticScroll.minute) { + if (!this.isSmoothProgrammaticScroll.minute) { this.scrollColumnToIndex("minute", targetMinute, behavior); } } + /** Returns whether a scroll was actually issued. */ private scrollColumnToIndex( type: WheelType, index: number, behavior: ScrollBehavior = "auto", - ): void { + ): boolean { const column = this.getColumnElement(type); - if (!column) return; + if (!column) return false; const target = index * this.getItemHeight(); @@ -546,12 +745,11 @@ export class TimePickerComponent else this.minuteScrollIndex.set(index); } - if (Math.abs(column.scrollTop - target) < 1) return; + if (Math.abs(column.scrollTop - target) < 1) return false; this.isProgrammaticScroll[type] = true; - - const existing = this.scrollLockTimer[type]; - if (existing) clearTimeout(existing); + this.isSmoothProgrammaticScroll[type] = behavior === "smooth"; + this.programmaticScrollTarget[type] = target; if (typeof column.scrollTo === "function") { column.scrollTo({ top: target, behavior }); @@ -559,11 +757,29 @@ export class TimePickerComponent column.scrollTop = target; } - this.scrollLockTimer[type] = setTimeout( - () => { - this.isProgrammaticScroll[type] = false; - }, - behavior === "smooth" ? SMOOTH_SCROLL_LOCK_MS : INSTANT_SCROLL_LOCK_MS, - ); + this.releaseScrollLockAfter(type, SCROLL_SETTLE_MAX_WAIT_MS); + return true; + } + + private releaseScrollLockAfter(type: WheelType, delay: number): void { + const existing = this.scrollLockTimer[type]; + if (existing) clearTimeout(existing); + + this.scrollLockTimer[type] = setTimeout(() => { + this.isProgrammaticScroll[type] = false; + this.isSmoothProgrammaticScroll[type] = false; + delete this.programmaticScrollTarget[type]; + delete this.scrollLockTimer[type]; + }, delay); + } + + /** Drops every trace of a programmatic scroll, target offset included. */ + private clearProgrammaticScroll(type: WheelType): void { + const existing = this.scrollLockTimer[type]; + if (existing) clearTimeout(existing); + delete this.scrollLockTimer[type]; + this.isProgrammaticScroll[type] = false; + this.isSmoothProgrammaticScroll[type] = false; + delete this.programmaticScrollTarget[type]; } } From 23046a62a74adc3364793195a0131d01c81c6fd9 Mon Sep 17 00:00:00 2001 From: m2rt Date: Fri, 18 Sep 2026 13:19:09 +0300 Subject: [PATCH 2/3] fix(time-picker): fixed lock expiry and scroll gesture state issue #678 --- .../form/time-picker/time-picker.component.ts | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/tedi/components/form/time-picker/time-picker.component.ts b/src/tedi/components/form/time-picker/time-picker.component.ts index 87549a239..02d642fd3 100644 --- a/src/tedi/components/form/time-picker/time-picker.component.ts +++ b/src/tedi/components/form/time-picker/time-picker.component.ts @@ -144,6 +144,8 @@ export class TimePickerComponent private readonly scrollLockTimer: Partial< Record> > = {}; + /** Latest moment a programmatic scroll may still own its column. */ + private readonly scrollLockDeadline: Partial> = {}; private readonly scrollDebounceTimer: Partial< Record> > = {}; @@ -563,17 +565,17 @@ export class TimePickerComponent if (existing) clearTimeout(existing); this.scrollDebounceTimer[type] = setTimeout(() => { + delete this.scrollDebounceTimer[type]; + + this.hasUserGesture[type] = false; + this.clearGestureTimer(type); + const column = this.getColumnElement(type); if (!column) return; const list = type === "hour" ? this.hours : this.minutes(); const index = this.indexFromScroll(type, column); - // The gesture has produced its value. Whatever scrolls the column next - // has to come with a gesture of its own to be read as a selection. - this.hasUserGesture[type] = false; - this.clearGestureTimer(type); - if (type === "hour") this.selectHour(list[index]); else this.selectMinute(list[index]); }, SCROLL_DEBOUNCE_MS); @@ -757,6 +759,7 @@ export class TimePickerComponent column.scrollTop = target; } + this.scrollLockDeadline[type] = Date.now() + SCROLL_SETTLE_MAX_WAIT_MS; this.releaseScrollLockAfter(type, SCROLL_SETTLE_MAX_WAIT_MS); return true; } @@ -765,12 +768,19 @@ export class TimePickerComponent const existing = this.scrollLockTimer[type]; if (existing) clearTimeout(existing); + const deadline = this.scrollLockDeadline[type]; + const wait = + deadline === undefined + ? delay + : Math.max(0, Math.min(delay, deadline - Date.now())); + this.scrollLockTimer[type] = setTimeout(() => { this.isProgrammaticScroll[type] = false; this.isSmoothProgrammaticScroll[type] = false; delete this.programmaticScrollTarget[type]; delete this.scrollLockTimer[type]; - }, delay); + delete this.scrollLockDeadline[type]; + }, wait); } /** Drops every trace of a programmatic scroll, target offset included. */ @@ -778,6 +788,7 @@ export class TimePickerComponent const existing = this.scrollLockTimer[type]; if (existing) clearTimeout(existing); delete this.scrollLockTimer[type]; + delete this.scrollLockDeadline[type]; this.isProgrammaticScroll[type] = false; this.isSmoothProgrammaticScroll[type] = false; delete this.programmaticScrollTarget[type]; From 20640ec81a721a60d1fc944690731d46f9b1bf1a Mon Sep 17 00:00:00 2001 From: m2rt Date: Fri, 9 Oct 2026 15:59:25 +0300 Subject: [PATCH 3/3] test(time-picker): assert minute column realigns on item height change #678 --- .../form/time-picker/time-picker.component.spec.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/tedi/components/form/time-picker/time-picker.component.spec.ts b/src/tedi/components/form/time-picker/time-picker.component.spec.ts index 365aea107..c93be5b88 100644 --- a/src/tedi/components/form/time-picker/time-picker.component.spec.ts +++ b/src/tedi/components/form/time-picker/time-picker.component.spec.ts @@ -1099,16 +1099,20 @@ describe("TimePickerComponent", () => { const localEl = localFixture.nativeElement as HTMLElement; localFixture.detectChanges(); - const hourColumn = localEl.querySelectorAll( + const columns = localEl.querySelectorAll( ".tedi-time-picker__column", - )[0]; + ); + const hourColumn = columns[0]; + const minuteColumn = columns[1]; stubScrollTop(hourColumn); + stubScrollTop(minuteColumn); localFixture.componentInstance.writeValue("09:30"); localFixture.detectChanges(); // Measured through the DEFAULT_ITEM_HEIGHT fallback jsdom forces. expect(hourColumn.scrollTop).toBe(9 * 40); + expect(minuteColumn.scrollTop).toBe(30 * 40); const item = localEl.querySelector( ".tedi-time-picker__item", @@ -1120,6 +1124,7 @@ describe("TimePickerComponent", () => { notifyResize?.(); expect(hourColumn.scrollTop).toBe(9 * 48); + expect(minuteColumn.scrollTop).toBe(30 * 48); expect(localFixture.componentInstance.highlightedHourIndex()).toBe(9); } finally { globalThis.ResizeObserver = originalResizeObserver;