Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions e2e/tests/issues/issue-1154-page-change-focus.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
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<void> {
await expect(screen.getByTestId('issue-1154-events')).toHaveText(
Expand Down Expand Up @@ -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');
}
);
11 changes: 10 additions & 1 deletion example/src/gh-issues/Issue1154PageChangeFocusRepro.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ export function Issue1154PageChangeFocusRepro() {
const [avoidsKeyboard, setAvoidsKeyboard] = useState(false);
const inputRefs = useRef<Array<TextInputInstance | null>>([]);
const lastRest = useRef(0);
const scrollsSinceCommand = useRef(0);
const [scrollsAtRest, setScrollsAtRest] = useState(0);
const lateCommand = useRef<ReturnType<typeof setTimeout> | null>(null);
const log = (event: string) =>
setEvents((current) => [...current.slice(-11), event]);
Expand All @@ -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);
Expand Down Expand Up @@ -84,7 +88,7 @@ export function Issue1154PageChangeFocusRepro() {
<Text testID="issue-1154-status" style={styles.status}>
Selected: {selectedPage} | Focused:{' '}
{focusedInput === null ? 'none' : `input ${focusedInput}`} | Keyboard:{' '}
{keyboardVisible ? 'visible' : 'hidden'}
{keyboardVisible ? 'visible' : 'hidden'} | Scrolls: {scrollsAtRest}
</Text>
<Text testID="issue-1154-events" style={styles.events}>
Events: {events.join(', ')}
Expand Down Expand Up @@ -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) {
Expand All @@ -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));
}
}}
Expand Down
14 changes: 11 additions & 3 deletions ios/PagerView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions ios/PagerViewProps.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 36 additions & 7 deletions ios/PagerViewProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q: Why are there 2 DispatchQueue.main calls?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DispatchQueue.main.async {
guard let page = props.pendingPage else {
return
}
props.pendingPage = nil
withAnimation {
props.currentPage = page
}
}
}
}

Expand Down
Loading