diff --git a/e2e/tests/issues/issue-1154-page-change-focus.e2e.ts b/e2e/tests/issues/issue-1154-page-change-focus.e2e.ts index 33ccae8b..f549f90f 100644 --- a/e2e/tests/issues/issue-1154-page-change-focus.e2e.ts +++ b/e2e/tests/issues/issue-1154-page-change-focus.e2e.ts @@ -32,6 +32,17 @@ async function expectStatus( ); } +/** + * Checks that the last command slid the pager rather than jumping. A slide + * reports around twenty scroll events and a jump one, so the floor is low + * enough for a loaded machine that drops frames. + */ +async function expectSlid(screen: Screen): Promise { + await expect(screen.getByTestId('issue-1154-status')).toHaveText( + /Scrolls: ([5-9]|\d\d)/ + ); +} + /** Waits for the pager to report that it has stopped on `page`. */ async function expectIdleAt(screen: Screen, page: number): Promise { await expect(screen.getByTestId('issue-1154-events')).toHaveText( @@ -305,5 +316,27 @@ test( await expectIdleAt(screen, 1); await wait(SETTLE_MS); await expectStatus(screen, platform, 1, 'none'); + await expectSlid(screen); + } +); + +test( + 'a setPage with the keyboard open still slides', + { ...options, platforms: ['ios'] }, + async ({ app, screen, platform }) => { + // Twice over: blurring in the pass that changes the selection made the + // second move apply the page without animating it, which the landing + // checks cannot see. A slide reports around twenty scroll events, a jump + // one or two. + await focusInputOnPage(app, screen, platform, 0); + await screen.getByTestId('issue-1154-go-1').tap(); + await wait(SETTLE_MS); + await expectSlid(screen); + await screen.getByTestId('issue-1154-input-1').tap(); + await expectStatus(screen, platform, 1, 'input 1'); + await screen.getByTestId('issue-1154-go-2').tap(); + await wait(SETTLE_MS); + await expectSlid(screen); + await expectStatus(screen, platform, 2, 'none'); } ); diff --git a/example/src/gh-issues/Issue1154PageChangeFocusRepro.tsx b/example/src/gh-issues/Issue1154PageChangeFocusRepro.tsx index 6aecd1e4..ef062858 100644 --- a/example/src/gh-issues/Issue1154PageChangeFocusRepro.tsx +++ b/example/src/gh-issues/Issue1154PageChangeFocusRepro.tsx @@ -33,6 +33,8 @@ export function Issue1154PageChangeFocusRepro() { const [avoidsKeyboard, setAvoidsKeyboard] = useState(false); const inputRefs = useRef>([]); const lastRest = useRef(0); + const scrollsSinceCommand = useRef(0); + const [scrollsAtRest, setScrollsAtRest] = useState(0); const lateCommand = useRef | null>(null); const log = (event: string) => setEvents((current) => [...current.slice(-11), event]); @@ -42,6 +44,8 @@ export function Issue1154PageChangeFocusRepro() { // from an earlier command. const startCommand = () => { setEvents([]); + scrollsSinceCommand.current = 0; + setScrollsAtRest(0); followUpCommand.current = null; if (lateCommand.current) { clearTimeout(lateCommand.current); @@ -84,7 +88,7 @@ export function Issue1154PageChangeFocusRepro() { Selected: {selectedPage} | Focused:{' '} {focusedInput === null ? 'none' : `input ${focusedInput}`} | Keyboard:{' '} - {keyboardVisible ? 'visible' : 'hidden'} + {keyboardVisible ? 'visible' : 'hidden'} | Scrolls: {scrollsAtRest} Events: {events.join(', ')} @@ -166,6 +170,10 @@ export function Issue1154PageChangeFocusRepro() { ref={pagerRef} style={styles.pager} onPageScroll={(e) => { + // Counted in a ref, not in state: a re-render on every scroll event + // changes how far the pager travels. The count is published once the + // pager stops, and it separates a slide from a jump. + scrollsSinceCommand.current += 1; // The pager sends the snapped page with no offset just before it // reports stopping, which is where it actually came to rest. if (e.nativeEvent.offset === 0) { @@ -176,6 +184,7 @@ export function Issue1154PageChangeFocusRepro() { // Only idle is logged: a re-render while the finger is still down // changes how far the pager travels. if (e.nativeEvent.pageScrollState === 'idle') { + setScrollsAtRest(scrollsSinceCommand.current); log('state idle at ' + String(lastRest.current)); } }} diff --git a/ios/PagerView.swift b/ios/PagerView.swift index 27c5ad62..375b3498 100644 --- a/ios/PagerView.swift +++ b/ios/PagerView.swift @@ -89,10 +89,11 @@ struct PagerView: View { let page = props.layoutDirection == .rtl ? props.children.count - 1 - restedPage : restedPage - // Read the selection on the next pass, once TabView has applied it. + // Read the selection once TabView has applied it. DispatchQueue.main.async { - // A page the pager is only passing through is not a rest. - guard page == props.currentPage else { + // A page the pager is passing through, or is about to leave for + // a selection still waiting, is not a rest. + guard page == (props.pendingPage ?? props.currentPage) else { return } props.resignFirstResponder(outsidePage: page) @@ -134,6 +135,13 @@ struct PagerView: View { if props.currentPage >= newValue.count && !newValue.isEmpty { props.currentPage = newValue.count - 1 } + // A selection still waiting is clamped too, so a page removed while it + // waits cannot select an index that no longer exists. + if let pending = props.pendingPage, + pending >= newValue.count, + !newValue.isEmpty { + props.pendingPage = newValue.count - 1 + } } .onChange(of: props.currentPage) { newValue in gestureObserver.currentPage = newValue diff --git a/ios/PagerViewProps.swift b/ios/PagerViewProps.swift index a61e6050..20401128 100644 --- a/ios/PagerViewProps.swift +++ b/ios/PagerViewProps.swift @@ -33,6 +33,10 @@ class PagerViewProps: ObservableObject { @Published var layoutDirection: PagerLayoutDirection = .ltr @Published var orientation: UICollectionView.ScrollDirection = .horizontal + /// Page an animated `setPage` selects once its wait ends. Not published, + /// since nothing renders from it. + var pendingPage: Int? + /// Blurs a text input focused on any page other than `page`. The paged /// `TabView` keeps neighbouring pages in the window, so nothing else does. /// An index that is not a page leaves every input alone: there is no page to diff --git a/ios/PagerViewProvider.swift b/ios/PagerViewProvider.swift index 6c238c3d..c9fff21e 100644 --- a/ios/PagerViewProvider.swift +++ b/ios/PagerViewProvider.swift @@ -164,18 +164,47 @@ final class PagerViewProvider: UIView, RNCPagerViewProviding { private func setPage(index: Int, animated: Bool) { // Blur before the selection changes: resigning during the TabView - // animation breaks the slide. A command for the page already shown moves - // nothing, and blurring on it would close the keyboard during a swipe an - // app echoes back with `setPage`. - if index != props.currentPage { + // animation breaks the slide. A command for the page already arriving + // moves nothing, and blurring on it would close the keyboard during a + // swipe an app echoes back with `setPage`. + if index != (props.pendingPage ?? props.currentPage) { props.resignFirstResponder(outsidePage: index) } - if animated { + guard animated else { + props.pendingPage = nil + props.currentPage = index + return + } + // A slide already waiting takes the page asked for last. + guard props.pendingPage == nil else { + props.pendingPage = index + return + } + // The page already selected moves nothing, and waiting would let this + // undo a swipe that commits in the meantime. The selection is still + // written, as it is without this wait, so nothing downstream sees a + // command go missing. + guard index != props.currentPage else { withAnimation { props.currentPage = index } - } else { - props.currentPage = index + return + } + // Closing the keyboard forces a layout, and SwiftUI drops the animation + // for a selection applied before that layout has settled. One pass is not + // always enough. The wait does not depend on the blur above finding an + // input, since a keyboard already on its way out collides the same way. + props.pendingPage = index + DispatchQueue.main.async { [props] in + DispatchQueue.main.async { + guard let page = props.pendingPage else { + return + } + props.pendingPage = nil + withAnimation { + props.currentPage = page + } + } } }