fix(rtl): horizontal RTL initial scroll mispositions on mount (mirror at end, blank at index 0) - #478
Open
Mohamed-kassim wants to merge 4 commits into
Conversation
… RTL initialContentOffset seed A horizontal RTL list opened with initialScrollIndex near the end can settle on the RTL mirror index. The initial-scroll watchdog/retry (scrollToFallbackOffset) dispatched the *unconverted* logical offset, and the initialContentOffset seed was also taken unconverted for RTL. Convert both, mirroring the normal doScrollTo dispatch path. Fixes LegendApp#476 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 405cb2c8d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Addresses Codex review: the previous skip applied to all horizontal RTL lists, so an offset-only RTL initial scroll (initialScrollOffset without initialScrollIndex) dropped its offset and finished at 0. Scope the RTL skip to bootstrap sessions (initialScrollIndex / initialScrollAtEnd), where the converting bootstrap dispatch re-applies the position; offset-only sessions keep their resolved offset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…llIndex 0
startBootstrapInitialScrollOnMount short-circuits ("finish at origin
without scrolling") when the logical target offset is 0. For a horizontal
RTL list the native origin is the opposite edge, so index 0 (e.g. a fresh
mount at the first item) is never dispatched and renders blank; the first
user scroll then reconciles to the mirror end. Skip the shortcut for
horizontal RTL so the bootstrap dispatch runs and converts to native
coordinates, composing with the scrollToFallbackOffset conversion already
in this PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontal RTL The reveal-settle path has a second "finish at origin without scrolling" shortcut, guarded to non-web/non-Android (iOS). It finishes when the logical resolved offset (0 for initialScrollIndex 0) is within 1px of the seed and the observed native offset — but for horizontal RTL the native origin is the opposite (max-offset) edge, so that's a false positive and index 0 is left blank on iOS. Android is already excluded and always dispatches, which is why this only reproduced on iOS. Gate with !isHorizontalRTL so RTL always dispatches (converting logical 0 to the native coordinate), mirroring the shouldFinishAtOrigin guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Fixes #476.
Problem
A horizontal RTL
LegendListpositions its initial scroll incorrectly on mount. Legend List keeps logical (LTR) scroll offsets and converts to the native RTL coordinate viatoNativeHorizontalOffset; four spots on the initial-scroll path skipped or bypassed that conversion:scrollToFallbackOffset(the initial-scroll watchdog/retry incheckFinishedScroll.ts) dispatched the unconverted logical offset directly to the nativescrollTo. When the watchdog wins the race against the bootstrap scroll's nativeonScrollround-trip (more likely on a large jump / slow device / withrecycleItems), a near-the-endinitialScrollIndexlands on the mirror index (e.g. request item 595 of 604 → settles on item 10 =total − 1 − 594).initialContentOffsetseed (the first nativecontentOffset) was also taken unconverted for RTL.startBootstrapInitialScrollOnMountshort-circuits (shouldFinishAtOrigin→ "finish at origin without scrolling") when the logical target offset is0. For horizontal RTL the native origin is the opposite edge, soinitialScrollIndex={0}(a fresh mount at the first item) is never dispatched and renders blank; the first user scroll then reconciles to the mirror end. This is the low-index counterpart of fix: dropexportsandcommitlintin built package file #1's near-the-end mirror — same subsystem, and the fix depends on fix: dropexportsandcommitlintin built package file #1 (once index 0 stops short-circuiting, its dispatch/retry relies on the convertedscrollToFallbackOffsetto land correctly).Platform.OS !== "web" && !== "android") compares the logical resolved offset against the seeded/observed native contentOffset and, when they match within 1px, callsfinishBootstrapInitialScrollWithoutScroll— finishing without ever dispatching a scroll. For horizontal RTL, logical0and native0are opposite edges, soinitialScrollIndex={0}passes the comparison and finishes while the viewport actually sits at the native origin (the last item's edge) → blank on iOS even with fixes 1–3 applied (which is why the bug survived on iOS after the Android verification passed: Android always takes the dispatch path here).Fix
scrollToFallbackOffset(mirroring the normaldoScrollTodispatch).initialContentOffsetseed so the converting initial-scroll dispatch positions the list.shouldFinishAtOriginshortcut for horizontal RTL so index-0 targets flow through the bootstrap dispatch (which converts to native coordinates).finishBootstrapInitialScrollWithoutScrollshortcut for horizontal RTL for the same reason — a logical-vs-native offset comparison is meaningless under RTL inversion, so RTL always takes the dispatch path.All four are scoped to horizontal RTL (
isHorizontalRTL/isHorizontalRTLProps/horizontalguards) — non-RTL and vertical paths are unaffected. No extra frames/retries are added for index 0; it simply uses the same bootstrap → dispatch → convert path every other index already uses.Verification
bun run tsc:src✅,bunx biome check✅,bun test→ 1369 pass / 1 fail (the pre-existing old-arch bootstrap process-isolation test, fails onmainunchanged).@legendapp/list@3.0.6, RN 0.85.3, Android) — without the fix the list settles on the mirror index (deterministic, 5/5 cold starts); with the fix it settles on the requested item.mushaf-book-legend-list) —initialScrollIndex={0}cold-mount rendered blank before, renders the first item after; verified via a patch of this change on@legendapp/list3.2.0 and 3.3.2 (the bug function is byte-identical across those versions).scrollTo → maxOffset) and the first item renders.Repro: https://github.com/Mohamed-kassim/legend-list-rtl-repro