Repository navigation
Conversation
| // 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 |
There was a problem hiding this comment.
q: Why are there 2 DispatchQueue.main calls?
There was a problem hiding this comment.
One hop isn't enough in practice. Counting scroll frames per command on a release build, where a slide reports about 18 and a jump 1, with the keyboard open:
- one hop: slide dropped in 5 of 20 runs, always the second keyboard-up move
- two hops: 1 of 50
I also tried waiting for the Core Animation commit explicitly, with a one-shot CFRunLoopObserver on .beforeWaiting: also 1 of 50, for about 18 more lines, so I kept the simpler version.
It isn't a commit boundary, incidentally logging the order gives setPage -> hop1 -> hop2 -> commit. The second hop just yields once more so the keyboard dismissal and its layout drain before the selection changes.
We can switch to the observer, or to put these numbers in the comment if you would like.
|
I think the CI failures are infra Could you re-run them? Thanks! |
|
Of the three CI fail on of them isn't infra. It fails |
Summary
Closes #1223.
An imperative
setPagestopped animating when the pager resizes with the keyboard. #1193 resigns the input when the pager leaves a page, so the keyboard closes and a pager sized around it springs back to full height about 17 ms later, and SwiftUI drops the slide for a selection applied before that layout lands.The fix leaves the blur where it is and lets the layout land before changing the selection. Commands arriving together share that wait, so the pager animates once, to the page asked for last. A command naming the page already selected writes the selection and returns without waiting, so it cannot undo a swipe committing meanwhile, and a waiting selection is clamped if its page is removed.
The wait is two main-queue hops, which lasts as long as the main thread is busy: 21 ms while the keyboard closes, so the relayout at 17 ms lands first. It does not cover a resize the app starts in the same handler as the command, where the hops finish in under a millisecond and a relayout 13 ms later still kills the slide. Waiting for Core Animation's commit with a run-loop observer measured the same, so this keeps the simpler version.
Measured by counting scroll frames per command, where a slide reports about 18 and a jump 1: with the pager inside a
KeyboardAvoidingView, three keyboard-up moves report 1, 1, 1 on main and 18, 18, 18 with this change. Removing the blur fromsetPageon main restores the slide too, which is where the regression comes from.That measurement was taken by hand, with the repro screen's root wrapped in a
KeyboardAvoidingView. Turning it into a test also needs the screen'sEvents:line to stop reflowing, and with that fixed height a separate race shows up instead, where a redirect ends on the earlier page in 11 of 23 runs onmain. So the test waits on that race rather than on this change. The strand the fixed height also exposed is fixed in #1234.Every animated
setPagenow waits, soonPageSelectedarrives correspondingly later.Test Plan
What's required for testing (prerequisites)?
The keyboard tests need the simulator to show its own keyboard, which a device that has seen a Mac keyboard will not. Clear both flags on every booted device after building (also in
e2e/README.md):What are the steps to reproduce (after prerequisites)?
On Issue #1154 Page Change Focus Repro: tap the input on page 0 and then Go 1, then the input on page 1 and Go 2. That second move used to swap pages with no slide. On this screen the resize comes from the event log reflowing rather than from the keyboard, which is the same mechanism by another route.
The new case moves twice with the keyboard open and checks both moves slid by reading the
Scrolls: Ncounter the repro shows: a jump readsScrolls: 1. On an iPhone 18 Pro simulator, iOS 27, React Native 0.88, the file passes 14 of 14, twice over.The SwiftPM job here fails
a setPage that interrupts another still lands on the later page, which is #1230 and not caused by this change. A fix for it is open as #1234, and this PR should go green once that merges.Compatibility
Checklist
README.md- no documented behaviour changes