Repository navigation
Conversation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
onPageSelecteddoesn'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
HorizontalPagerandVerticalPagerget nokey, 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
settledPagecollector reportsonPageSelectedwith the new index.PageHost'supdatereadspages. Fabric applies a move as a remove and an insert of the same view, andremovePageAtdetaches it. The keyed slot keeps the same view without recomposing, so without that read a moved page renders blank.LaunchedEffect(pageCount). ItsscrollToPagedrops 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 thesettledPagecollector reports it.addPageremoves 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
onPageSelectednow fires with the new index. The index change also goes throughonPageScroll, so withkeyboardDismissMode="on-drag"the keyboard closes, the same as any programmatic page change on Android today.onPageSelectedfor a removal now arrives when the pager is shown again.keybehave as before.addPage. If it lands first, the dedupe moves tochildren; 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 Reproto the example app ande2e/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 manyonPageSelectedevents arrived:Selected: 2 (B), B shownSelected: 3 (C), C shownSelected: 2 (N0)Selected: 3 (C)Selected: 2 (D)Selected: 1 (C)Selected: 1 (D)Selected: 0 (C)Selected: 2 (B)Selected: 0 (C)Selected: 2 (D)Selected: 3 (C), then D rendersVerified
updatechange, "Move C first" and "Move C last" fail on a blank page.A B C D E->B X C Y Don 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, exampletypescriptand e2etypecheckpass.Compatibility
Checklist
README.md: not applicable