Skip to content

fix(android): keep the visible page when pages change before it - #1188

Open
Luccacvb wants to merge 1 commit into
callstack:mainfrom
Luccacvb:fix/android-keep-page-on-insert
Open

Luccacvb wants to merge 1 commit into
callstack:mainfrom
Luccacvb:fix/android-keep-page-on-insert

Conversation

@Luccacvb

Copy link
Copy Markdown
Contributor

Summary

Fixes the Android side of #965 (the iOS fix is #1078). Related: #808.

On Android 9.x, inserting, removing or moving pages before the current one changes the visible page. The pager keeps the old index, so whatever page moves into that index takes the current page's place, and onPageSelected doesn't fire. In #965's case, a gallery that loads older photos at the start jumps to another photo. 8.x kept the page: ViewPager2 anchors the current item and reports its new index. #1078 does the same for iOS.

reproduction

pager-view-965-before-after.mp4

What happened

  • HorizontalPager and VerticalPager get no key, so Compose keys each page by its index. After an insert at index 0, index 2 is still the current page, and it now shows the page that was at index 1.

Fix

  • Key both pagers by the page view's id, its React tag. Compose then keeps the current page by key and moves the index with it, and the existing settledPage collector reports onPageSelected with the new index.
  • PageHost's update reads pages. Fabric applies a move as a remove and an insert of the same view, and removePageAt detaches it. The keyed slot keeps the same view without recomposing, so without that read a moved page renders blank.
  • Remove the explicit out-of-range clamp in LaunchedEffect(pageCount). Its scrollToPage drops the key, and when it ran before the next measure (for example while a native-stack screen covered the pager) removing pages before the current one landed on the wrong page. Measure already clamps an out-of-range index, and the settledPage collector reports it.
  • addPage removes the view before inserting it, so keys stay unique if Fabric ever re-adds a view it never removed. In the normal remove + insert path this is a no-op.

Behavior notes

  • When pages change before the current one, onPageSelected now fires with the new index. The index change also goes through onPageScroll, so with keyboardDismissMode="on-drag" the keyboard closes, the same as any programmatic page change on Android today.
  • An insert at the current index keeps the current page and moves its index by one, like 8.x. Main showed the new page.
  • While the pager is covered or detached, the onPageSelected for a removal now arrives when the pager is shown again.
  • Unchanged: removing the current page shows the page that lands at its index, removing the last page while on it moves to the previous one, and children without a key behave as before.
  • Prepending more than about 100 pages in one update falls back to the old index, since Compose only matches keys near the current page.
  • iOS is untouched; fix(ios): persist position during page list head changes #1078 covers it. The new e2e test is Android-only.
  • fix: exit pager animation #1180 also changes addPage. If it lands first, the dedupe moves to children; I haven't tested the two together.

Test Plan

What's required for testing (prerequisites)?

Android emulator or device.

What are the steps to reproduce (after prerequisites)?

Added Issue #965 Pages Before Current Repro to the example app and e2e/tests/issues/issue-965-pages-before-current.e2e.ts. The pager starts on page C of A, B, C, D, and the status shows the selected page and how many onPageSelected events arrived:

Action Before After
Insert first Selected: 2 (B), B shown Selected: 3 (C), C shown
Insert before C Selected: 2 (N0) Selected: 3 (C)
Remove first Selected: 2 (D) Selected: 1 (C)
Remove first two Selected: 1 (D) Selected: 0 (C)
Move C first Selected: 2 (B) Selected: 0 (C)
Move C last, swipe back Selected: 2 (D) Selected: 3 (C), then D renders

Verified

  • On main all 6 new tests fail with the values above. With the fix they pass on every run (6 runs). Without the update change, "Move C first" and "Move C last" fail on a blank page.
  • Removing the first two pages while a native-stack screen covers the pager: with the explicit clamp it comes back on D, without it on C.
  • Full Android e2e suite with the fix: 26/26 tests pass. Android 16 emulator (arm64), release build.
  • Checked by hand: fix(ios): persist position during page list head changes #1078's transition A B C D E -> B X C Y D on C, the pages around a moved page, offscreenPageLimit={0}, orientation="vertical", an insert while covered, and a nested pager on the current page.
  • format:check, lint (no new warnings), typecheck, prepare, test, example typescript and e2e typecheck pass.

Compatibility

OS Implemented
iOS n/a
Android ✅

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md: not applicable
  • I updated the typed files (TS and Flow): not applicable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant