fix(rtl): correct horizontal RTL on native (blank, wrong edge, flicker)#494
fix(rtl): correct horizontal RTL on native (blank, wrong edge, flicker)#494Radwan-Albahrani wants to merge 1 commit into
Conversation
2c871d8 to
4ab4e41
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20d72a27fd
ℹ️ 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".
Horizontal lists were blank / rendered from the wrong edge / flickered in a native-RTL app (I18nManager.isRTL) on iOS and Android new architecture. - Don't JS-mirror item positions on native. RN's doLeftAndRightSwapInRTL (on by default) already rewrites 'left' to 'start' so Yoga mirrors positions inside an RTL container; mirroring again in JS double-mirrors items off screen (LegendApp#477, LegendApp#458). Web keeps the mirror since Container forces direction: ltr there. - Pin the platform-default scroll mode instead of the per-frame distance heuristic. A single overscroll-bounce frame (negative rawOffset) pinned 'negative' and the next frame fell through to the heuristic and could reclassify 'inverted' -> 'normal', mirroring the visible-range math mid-scroll and blanking the list. Negative raw is bounce: clamp, never switch modes. - Convert the offset in scrollToFallbackOffset so the initial-scroll watchdog/retry doesn't dispatch an unconverted (mirror) offset and ping-pong the converting dispatch (LegendApp#476) - the ping-pong is the visible flicker. Verified on real iOS and Android devices (new architecture).
4ab4e41 to
62bab15
Compare
|
Both good catches — fixed in the latest push. 1. Per-list if (Platform.OS !== "web" && I18nManager.isRTL) {
return logicalPosition; // native tree is RTL → RN already mirrored
}
return Math.max(0, listSize - logicalPosition - itemSize); // web, or prop-forced RTL on an LTR native tree2. Negative web scroll offsets. Right: the deterministic pin should be native-only. Added regression tests for both (prop-forced native mirror; web negative offset), plus the pinned-mode-across-frames case. Full suite green (1460), biome + tsc clean. |
Problem
In a native-RTL app (
I18nManager.isRTL === true, e.g. Arabic/Hebrew) a horizontalLegendListon the new architecture is broken on both iOS and Android:onViewableItemsChangedkeeps firing (items exist, positioned off screen) — issue Horizontal list renders blank in a native-RTL app (I18nManager.isRTL), Android #477LTR apps, and RTL apps that only use the
rtlprop on an otherwise-LTR app, are unaffected.Root cause
The library emulates RTL entirely in JS (mirror item positions, convert scroll offsets) and assumes the native scroll view behaves like a physically-LTR scroller. On native new-arch that assumption doesn't hold, and it collides with what React Native already does for RTL:
doLeftAndRightSwapInRTLis on by default on iOS and Android. Inside an RTL container it rewrites an absolutely-positioned child'sleftinset tostart, which Yoga resolves from the right edge — so native layout already mirrors the item positions.toPhysicalHorizontalItemPositionthen mirrors them a second time, pushing every item off screen → blank list.contentOffsetin both directions (maxOffset - x, self-inverse), so from JS the offset is deterministically "inverted". The classifier intoLogicalHorizontalOffsetinstead guessed per frame: a single overscroll-bounce frame (negativerawOffset) pinned"negative", then the next positive frame fell through to the distance heuristic and could reclassify"inverted" → "normal", mirroring the visible-range math mid-scroll → blank on swipe.scrollToFallbackOffsetdispatched the unconverted logical offset, so the initial-scroll watchdog/retry fought the convertingdoScrollTodispatch — the visible flicker, and it could settle on the mirror index (Horizontal RTL: initialScrollIndex near the end settles on the mirror index #476).Fix
toPhysicalHorizontalItemPosition— skip the JS mirror on native; RN'sleft→startswap already mirrors. Web keeps the mirror (Container forcesdirection: ltrthere, soleftstays physical).toLogicalHorizontalOffset— pin the platform default mode (native"inverted", web"normal") instead of the per-frame heuristic. A negativerawOffsetis treated as overscroll bounce (clamped), never a mode switch.scrollToFallbackOffset— convert the logical offset withtoNativeHorizontalOffset, matching the normal dispatch, so the watchdog/retry can't dispatch the mirror offset. (Same change as fix(rtl): horizontal initialScrollIndex near the end settles on the mirror index #478 for this function.)Testing
bun test— 1458 pass, 0 fail (updated the RTL unit tests that encoded the old mirror/heuristic behavior; added a regression test that the mode stays pinned across frames).biome checkandtsc --project tsconfig.src.jsonclean.Related
Fixes #477, #458, #476. Overlaps the
scrollToFallbackOffsethalf of #478; supersedes thedirection: ltrapproach in #483 (forcing the scroll container LTR disables RN's native RTL machinery and then needs constant re-dispatch — this instead works with the native behavior).