Repository navigation
fix(DateInput, DateTimeInput): the month scroller's steps follow design:reduced-motion #8496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+488
−5
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
f2c04aa
fix(DateInput, DateTimeInput): the month scroller's steps follow desi…
vjeux 06fd623
fix(DateInput, DateTimeInput): every month step replaces a running Re…
vjeux 19a9cc7
fix(DateInput, DateTimeInput): a step that lands under Reduce Motion …
vjeux 6728ff1
test(DateInput): MonthScroller's proofs read the dip as it is created…
vjeux File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] With Reduce Motion on (`design:reduced-motion`), the month arrows in the DateInput and DateTimeInput bottom-sheet calendars, Reset, and a keyboard move into another month no longer scroll the months: on a computer with a mouse or trackpad the month changes at once, and on a touch screen it changes through a short opacity dip. A second step during the dip replaces the first, and unmounting the calendar during the dip scrolls nothing. | ||
|
|
||
| @vjeux |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | ||
|
|
||
| import {act, render} from '@testing-library/react'; | ||
| import {createRef} from 'react'; | ||
| import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; | ||
| import {MonthScroller, type MonthScrollerHandle} from './MonthScroller'; | ||
|
|
||
| /** | ||
| * A touch screen with Reduced Motion on: the policy's combined query | ||
| * (motion/reducedMotion.ts) and the coarse pointer both match. | ||
| */ | ||
| function stubReducedMotionTouch(): void { | ||
| vi.stubGlobal('matchMedia', (query: string) => ({ | ||
| matches: | ||
| query.includes('prefers-reduced-motion: reduce') || | ||
| /pointer:\s*coarse/.test(query), | ||
| media: query, | ||
| onchange: null, | ||
| addListener: () => {}, | ||
| removeListener: () => {}, | ||
| addEventListener: () => {}, | ||
| removeEventListener: () => {}, | ||
| dispatchEvent: () => false, | ||
| })); | ||
| } | ||
|
|
||
| class FakeResizeObserver { | ||
| constructor(private callback: ResizeObserverCallback) {} | ||
| observe() { | ||
| this.callback([], this); | ||
| } | ||
| unobserve() {} | ||
| disconnect() {} | ||
| } | ||
|
|
||
| describe('MonthScroller under Reduced Motion on a touch screen', () => { | ||
| const scrollTo = vi.fn(); | ||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| stubReducedMotionTouch(); | ||
| vi.stubGlobal('ResizeObserver', FakeResizeObserver); | ||
| vi.spyOn(HTMLElement.prototype, 'clientWidth', 'get').mockReturnValue(300); | ||
| vi.spyOn(HTMLElement.prototype, 'clientHeight', 'get').mockReturnValue(300); | ||
| Element.prototype.scrollTo = scrollTo; | ||
| Element.prototype.animate = vi.fn(() => ({ | ||
| cancel: vi.fn(), | ||
| })) as unknown as typeof Element.prototype.animate; | ||
| scrollTo.mockClear(); | ||
| }); | ||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| vi.unstubAllGlobals(); | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| function renderScroller({ | ||
| onVisibleMonthChange = () => {}, | ||
| }: {onVisibleMonthChange?: (monthIndex: number) => void} = {}) { | ||
| const handleRef = createRef<MonthScrollerHandle>(); | ||
| const view = render( | ||
| <MonthScroller | ||
| handleRef={handleRef} | ||
| minMonthIndex={0} | ||
| maxMonthIndex={23} | ||
| initialMonthIndex={9} | ||
| onVisibleMonthChange={onVisibleMonthChange} | ||
| selectedDate={null} | ||
| today={{year: 2026, month: 10, day: 15}} | ||
| isDateDisabled={() => false} | ||
| weekStartsOn={0} | ||
| onSelect={() => {}} | ||
| />, | ||
| ); | ||
| scrollTo.mockClear(); | ||
| return {handleRef, ...view}; | ||
| } | ||
|
|
||
| it('lets a second step within the dip replace the first: only its scroll lands', () => { | ||
| const {handleRef} = renderScroller(); | ||
| act(() => handleRef.current?.scrollToMonth(10)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(40); | ||
| }); | ||
| act(() => handleRef.current?.scrollToMonth(11)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(400); | ||
| }); | ||
| expect(scrollTo).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('keeps a jump to a distant month made within the dip: the dip does not land after it', () => { | ||
| const {handleRef} = renderScroller(); | ||
| act(() => handleRef.current?.scrollToMonth(10)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(40); | ||
| }); | ||
| // Eleven months on: beyond the mounted window, so it jumps at once. | ||
| act(() => handleRef.current?.scrollToMonth(20)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(400); | ||
| }); | ||
| expect( | ||
| scrollTo.mock.calls.map(([options]) => (options as ScrollToOptions).left), | ||
| ).toEqual([20 * 300]); | ||
| }); | ||
|
|
||
| it('keeps a step at once made within the dip: the dip does not land after it', () => { | ||
| const {handleRef} = renderScroller(); | ||
| act(() => handleRef.current?.scrollToMonth(10)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(40); | ||
| }); | ||
| act(() => handleRef.current?.scrollToMonth(8, 'auto')); | ||
| act(() => { | ||
| vi.advanceTimersByTime(400); | ||
| }); | ||
| expect( | ||
| scrollTo.mock.calls.map(([options]) => (options as ScrollToOptions).left), | ||
| ).toEqual([8 * 300]); | ||
| }); | ||
|
|
||
| it('reports a trackpad scroll after a step lands, even with an earlier steer that never arrived', () => { | ||
| const onVisibleMonthChange = vi.fn(); | ||
| const {handleRef, container} = renderScroller({onVisibleMonthChange}); | ||
| const scroller = container.querySelector<HTMLElement>( | ||
| '[data-scroller="months"]', | ||
| ); | ||
| if (scroller == null) { | ||
| throw new Error('no scroller'); | ||
| } | ||
| // The re-assert when the wheels close: a steer to the month already in | ||
| // view, whose scroll never arrives. | ||
| act(() => handleRef.current?.scrollToMonth(9, 'auto')); | ||
| // A step lands at its dip's middle. | ||
| act(() => handleRef.current?.scrollToMonth(10)); | ||
| act(() => { | ||
| vi.advanceTimersByTime(400); | ||
| }); | ||
| // A trackpad scroll on to the next month: no touch ends the steer. | ||
| act(() => { | ||
| scroller.scrollLeft = 11 * 300; | ||
| scroller.dispatchEvent(new Event('scroll')); | ||
| vi.advanceTimersByTime(50); | ||
| }); | ||
| expect(onVisibleMonthChange).toHaveBeenLastCalledWith(11); | ||
| }); | ||
|
|
||
| it('scrolls nothing once unmounted within the dip', () => { | ||
| const {handleRef, unmount} = renderScroller(); | ||
| act(() => handleRef.current?.scrollToMonth(10)); | ||
| unmount(); | ||
| act(() => { | ||
| vi.advanceTimersByTime(400); | ||
| }); | ||
| expect(scrollTo).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ import { | |
| plainDateToISO, | ||
| DATE_FORMAT_WITH_WEEKDAY, | ||
| } from '../utils'; | ||
| import {reducedMotionFade} from '../motion/reducedMotion'; | ||
| import {dateInputTouchSizes, dateInputTouchGeometry} from './tokens.stylex'; | ||
| import {useOwnScrollGesture} from './useOwnScrollGesture'; | ||
| import {useScrollSettle} from './useScrollSettle'; | ||
|
|
@@ -431,8 +432,26 @@ export function MonthScroller({ | |
| ); | ||
| }, [paneSize, initialMonthIndex, minMonthIndex, isRTL]); | ||
|
|
||
| // The Reduced Motion dip of a step on a touch screen, owned here so a | ||
| // second step or the unmount clears it. | ||
| const dipRef = useRef<{animation: Animation; timer: number} | null>(null); | ||
| const stopDip = useCallback(() => { | ||
| const dip = dipRef.current; | ||
| if (dip != null) { | ||
| window.clearTimeout(dip.timer); | ||
| dip.animation.cancel(); | ||
| dipRef.current = null; | ||
| } | ||
| }, []); | ||
| // Synchronizes with the dip's timer and animation: neither outlives the | ||
| // scroller. | ||
| useEffect(() => stopDip, [stopDip]); | ||
|
|
||
| const scrollToMonth = useCallback( | ||
| (monthIndex: number, behavior: ScrollBehavior = 'smooth') => { | ||
| // Every step replaces a running dip, whichever way it moves. Otherwise | ||
| // the dip's landing would undo a jump or an immediate step made after it. | ||
| stopDip(); | ||
| const scroller = scrollerRef.current; | ||
| if (scroller == null || paneSize === 0) { | ||
| return; | ||
|
|
@@ -458,13 +477,52 @@ export function MonthScroller({ | |
| recenter(scroller, row); | ||
| return; | ||
| } | ||
| const left = scrollOffsetForRow(row, paneSize, isRTL); | ||
| // Under reduced motion the month changes without travel: at once on a fine | ||
| // primary pointer, under an opacity dip on a coarse one (reducedMotionFade). | ||
| const fade = behavior === 'smooth' ? reducedMotionFade(scroller) : null; | ||
| if (fade != null) { | ||
| // The window mounts around the target first, and the scroll lands in | ||
| // the layout effect below, as a far jump does. So nothing moves the | ||
| // window after the jump. If the window followed the jump, WebKit would | ||
| // re-snap it one pane further on every frame. A scroll that lands on | ||
| // the centre row reports nothing. | ||
| const land = () => { | ||
| // The landing is the newest steer. An earlier steer whose scroll | ||
| // never arrived would otherwise hide every scroll after it, a | ||
| // trackpad's included. Closing the wheels makes such a steer: it | ||
| // re-asserts the month already in view. | ||
| steeredRowRef.current = null; | ||
| if (row === centerRowRef.current) { | ||
| scroller.scrollTo({left, behavior: 'auto'}); | ||
| return; | ||
| } | ||
| pendingScrollRef.current = row; | ||
| centerRowRef.current = row; | ||
| setCenterRow(row); | ||
| }; | ||
| if (fade.duration === 0) { | ||
| land(); | ||
| return; | ||
| } | ||
| // A second step within the dip has replaced the first (above): only | ||
| // its scroll lands, at its own dip's middle. | ||
|
Comment on lines
+508
to
+509
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: this repeats the comment above |
||
| dipRef.current = { | ||
| animation: scroller.animate( | ||
| [{opacity: 1}, {opacity: 0}, {opacity: 1}], | ||
| {duration: fade.duration, easing: fade.easing}, | ||
| ), | ||
| timer: window.setTimeout(() => { | ||
| dipRef.current = null; | ||
| land(); | ||
| }, fade.duration / 2), | ||
| }; | ||
| return; | ||
| } | ||
| steeredRowRef.current = row; | ||
| scroller.scrollTo({ | ||
| left: scrollOffsetForRow(row, paneSize, isRTL), | ||
| behavior, | ||
| }); | ||
| scroller.scrollTo({left, behavior}); | ||
| }, | ||
| [paneSize, minMonthIndex, rowCount, isRTL, recenter], | ||
| [paneSize, minMonthIndex, rowCount, isRTL, recenter, stopDip], | ||
| ); | ||
|
|
||
| // Deliberately un-keyed: it runs after every render, costs a null check, and | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing the month wheels re-asserts the month with
scrollToMonth(month, 'auto')(TouchDateField.tsx:951-955), and when nothing scrollssteeredRowRefkeeps that row. The smooth path used to overwrite it and clear it on arrival; this landing movescenterRowRefand never touchessteeredRowRef, so under Reduce Motion every later trackpad or mouse-wheel scroll to another month returns at the steer check (:655-660) without callingonVisibleMonthChange, and the header and the arrows stay on the stepped month. Clearing the steer here fixes it;centerRowRefalready keeps this landing's own scroll from being reported:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At 9b8d6c0: Fixed. A landing is now the newest steer, so it clears an earlier steer that never arrived.
MonthScroller.test.tsx"reports a trackpad scroll after a step lands, even with an earlier steer that never arrived" covers it, and fails on main.