diff --git a/.claude/skills/review-and-test/SKILL.md b/.claude/skills/review-and-test/SKILL.md index cf83b38d0..9af24361d 100644 --- a/.claude/skills/review-and-test/SKILL.md +++ b/.claude/skills/review-and-test/SKILL.md @@ -338,6 +338,12 @@ Run through relevant entries after any fix or review. This is the single source - [ ] `firstItemOffset` after fix — confirm it equals `ListHeaderComponent` height/width - [ ] `measureParentSize(view)` returns `x=0, y=0` on RN 0.84 Fabric — the #2017 bug may only manifest on other RN versions +### Snapping + maintainVisibleContentPosition (Android) +- [ ] `snapToInterval` / `snapToOffsets` / `pagingEnabled` carousel on **Android** — swipe forward until recycling starts, then swipe backward. Landing on index 0 instead of the previous card is issue #2427. Use the `Snap Carousel Repro` fixture screen; its "settled on" readout makes it unmissable. +- [ ] Prepending to a snapping list still holds the visible item in place (`Horizontal MVCP` screen with a `snapToInterval` added) +- Android snapping is gated by one predicate in RN's `ScrollView.js`: `pagingEnabled === true || snapToInterval != null || snapToOffsets != null` becomes the native `pagingEnabled`. Anything keying off "does this list snap" must match it — `snapToInterval` alone misses paged carousels. +- FlashList hands the **native** `maintainVisibleContentPosition` to the ScrollView (`RecyclerView.tsx`). Android's `MaintainVisibleScrollPositionHelper` then re-anchors on every layout change of the first visible **cell** — not of `ScrollAnchor` — so a recycling list triggers it constantly. Each one calls `scrollToPreservingMomentum` → `recreateFlingAnimation(x, Integer.MAX_VALUE)`, cancelling the snap animator. **Nothing on the JS side can suppress this; only withholding the native prop stops it.** + ### Performance - [ ] Benchmark screen shows no FPS regression (use `ManualBenchmarkExample`) @@ -345,6 +351,11 @@ Run through relevant entries after any fix or review. This is the single source ## Common Issues +- **A green unit suite is not evidence the fix works** — a guard can be correct, well-tested, and still sit on a code path the bug never takes. Before believing a fix for a device-only symptom, reproduce it on device, then A/B it: `git checkout main && yarn build` (bug present) vs the fix branch (bug gone). If you cannot reproduce it first, you cannot claim you fixed it. To check whether a JS path even runs, drop a temporary `console.log` and read it back with `adb logcat` — and validate the probe itself (confirm the string is in the served bundle and that some other `ReactNativeJS` line reaches logcat) before trusting a zero count. +- **Test the wiring, not just the helper** — a pure predicate can be perfect while the call site ignores it. Assert on the prop the ScrollView actually receives, then delete the gate and confirm that test goes red. +- **Metro port 8081 may be taken by another project** — do not kill it. Start ours with `yarn start --port 8092`; `adb reverse` will not help because RN on an emulator dials `10.0.2.2`, not `localhost`. Point the app at it by pushing a prefs file instead: + `adb push prefs.xml /data/local/tmp/ && adb shell "run-as cp /data/local/tmp/prefs.xml /data/data//shared_prefs/_preferences.xml"` with `10.0.2.2:8092`. +- **Android build filling the disk** — `./gradlew assembleDebug -PreactNativeArchitectures=arm64-v8a` builds only the emulator ABI, roughly a quarter of the NDK output. - **Tests pass but device shows bug** — did you `yarn build` and relaunch? The dist/ folder may be stale - **Switched branches but behavior didn't change** — `dist/` is NOT rebuilt on branch switch. You MUST run `yarn build` after every `git checkout`. Verify with `grep` in `dist/` that the expected code is present before testing. - **RTL looks wrong but LTR is fine** — did you set `forceRTL(true)` in `index.js` and do a full kill+relaunch? diff --git a/fixture/react-native/src/ExamplesScreen.tsx b/fixture/react-native/src/ExamplesScreen.tsx index 7554937f7..d77d1bab1 100644 --- a/fixture/react-native/src/ExamplesScreen.tsx +++ b/fixture/react-native/src/ExamplesScreen.tsx @@ -30,6 +30,7 @@ export const ExamplesScreen = () => { { title: "Sticky Header Example", destination: "StickyHeaderExample" }, { title: "Horizontal List", destination: "HorizontalList" }, { title: "Carousel", destination: "Carousel" }, + { title: "Snap Carousel Repro", destination: "SnapCarouselRepro" }, { title: "Grid", destination: "Grid" }, { title: "Masonry", destination: "Masonry" }, { title: "Complex Masonry", destination: "ComplexMasonry" }, diff --git a/fixture/react-native/src/NavigationTree.tsx b/fixture/react-native/src/NavigationTree.tsx index 1cdfffd7b..332031032 100644 --- a/fixture/react-native/src/NavigationTree.tsx +++ b/fixture/react-native/src/NavigationTree.tsx @@ -29,6 +29,7 @@ import DynamicItems from "./DynamicItems"; import RecyclerViewHandlerTest from "./RecyclerViewHandlerTest"; import MovieList from "./MovieList"; import Carousel from "./Carousel"; +import SnapCarouselRepro from "./SnapCarouselRepro"; import { LayoutOptions } from "./LayoutOptions"; import ShowcaseApp from "./ShowcaseApp"; import LotOfItems from "./lot-of-items/LotOfItems"; @@ -47,6 +48,10 @@ const NavigationTree = () => { + String(i + 1)); +const CARD_GAP = 12; + +export const SnapCarouselRepro = () => { + const { width: screenWidth } = useWindowDimensions(); + const cardWidth = screenWidth - 48; + const snapInterval = cardWidth + CARD_GAP; + + const [active, setActive] = useState(0); + const [offset, setOffset] = useState(0); + + const onSettle = useCallback( + (event: NativeSyntheticEvent) => { + const x = event.nativeEvent.contentOffset.x; + setOffset(Math.round(x)); + setActive(Math.round(x / snapInterval)); + }, + [snapInterval] + ); + + return ( + + + settled on: {active + 1} + + offset: {offset} + item} + horizontal + showsHorizontalScrollIndicator={false} + snapToInterval={snapInterval} + decelerationRate="fast" + onMomentumScrollEnd={onSettle} + onScrollEndDrag={onSettle} + renderItem={({ item }) => ( + + {item} + + )} + /> + + ); +}; + +const styles = StyleSheet.create({ + container: { flex: 1, paddingTop: 60, backgroundColor: "#111" }, + readout: { + fontSize: 34, + fontWeight: "bold", + color: "#fff", + paddingHorizontal: 24, + }, + offset: { fontSize: 18, color: "#9bd", paddingHorizontal: 24, marginTop: 4 }, + card: { + height: 320, + marginRight: CARD_GAP, + marginTop: 20, + backgroundColor: "tomato", + alignItems: "center", + justifyContent: "center", + borderRadius: 12, + }, + cardText: { fontSize: 96, fontWeight: "bold", color: "#fff" }, +}); + +export default SnapCarouselRepro; diff --git a/src/__tests__/RecyclerView.test.tsx b/src/__tests__/RecyclerView.test.tsx index c01af4834..9d1937b47 100644 --- a/src/__tests__/RecyclerView.test.tsx +++ b/src/__tests__/RecyclerView.test.tsx @@ -5,6 +5,7 @@ import { render } from "@quilted/react-testing"; import { FlashListRef } from "../FlashListRef"; import { FlashList } from ".."; +import { PlatformConfig } from "../native/config/PlatformHelper"; // Mock measureLayout to return fixed dimensions jest.mock("../recyclerview/utils/measureLayout", () => { @@ -514,4 +515,69 @@ describe("RecyclerView", () => { expect(scrollToEndSpy).toHaveBeenCalled(); }); }); + describe("native maintainVisibleContentPosition on snapping lists", () => { + const platform = PlatformConfig as unknown as { + nativeMvcpBreaksSnapFling: boolean; + }; + + afterEach(() => { + platform.nativeMvcpBreaksSnapFling = false; + }); + + // The prop the ScrollView is actually handed. Android's + // MaintainVisibleScrollPositionHelper only runs when this is present, so + // this is the single thing that decides whether a snap fling survives. + const nativeMvcpPropOf = (root: ReturnType) => { + const scrollable = root.findWhere((node: any) => node.props.onScroll); + if (!scrollable) throw new Error("Could not find scrollable component"); + return scrollable.prop("maintainVisibleContentPosition" as never); + }; + + const renderCarousel = (snapProps: Record) => { + const data = Array.from({ length: 30 }, (_, i) => i); + const result = render( + String(item)} + renderItem={({ item }) => {item}} + /> + ); + jest.runAllTimers(); + return result; + }; + + it("withholds it from a snapToInterval list where the fling would be retargeted", () => { + platform.nativeMvcpBreaksSnapFling = true; + + expect(nativeMvcpPropOf(renderCarousel({ snapToInterval: 100 }))).toBe( + undefined + ); + }); + + it("withholds it from a pagingEnabled list too", () => { + platform.nativeMvcpBreaksSnapFling = true; + + expect(nativeMvcpPropOf(renderCarousel({ pagingEnabled: true }))).toBe( + undefined + ); + }); + + it("still passes it to a list that does not snap", () => { + platform.nativeMvcpBreaksSnapFling = true; + + expect(nativeMvcpPropOf(renderCarousel({}))).toEqual( + expect.objectContaining({ minIndexForVisible: 0 }) + ); + }); + + it("still passes it to a snapping list on platforms that are not affected", () => { + platform.nativeMvcpBreaksSnapFling = false; + + expect(nativeMvcpPropOf(renderCarousel({ snapToInterval: 100 }))).toEqual( + expect.objectContaining({ minIndexForVisible: 0 }) + ); + }); + }); }); diff --git a/src/__tests__/snapping.test.ts b/src/__tests__/snapping.test.ts new file mode 100644 index 000000000..b180201f1 --- /dev/null +++ b/src/__tests__/snapping.test.ts @@ -0,0 +1,65 @@ +import { + isSnappingList, + supportsNativeMaintainVisibleContentPosition, +} from "../recyclerview/utils/snapping"; +import { PlatformConfig } from "../native/config/PlatformHelper"; + +describe("isSnappingList", () => { + // Mirrors ScrollView.js: pagingEnabled === true || snapToInterval != null || + // snapToOffsets != null. Anything looser or tighter would cover a different + // set of lists than Android's native snapping path does. + it.each([ + ["pagingEnabled snaps", { pagingEnabled: true }, true], + ["snapToInterval snaps", { snapToInterval: 300 }, true], + ["snapToOffsets snaps", { snapToOffsets: [0, 300] }, true], + ["an empty snapToOffsets array still snaps", { snapToOffsets: [] }, true], + ["a zero snapToInterval still snaps", { snapToInterval: 0 }, true], + ["no snapping props does not snap", {}, false], + ["pagingEnabled false does not snap", { pagingEnabled: false }, false], + ["a null snapToInterval does not snap", { snapToInterval: null }, false], + ["a null snapToOffsets does not snap", { snapToOffsets: null }, false], + ])("%s", (_label, props, expected) => { + expect(isSnappingList(props)).toBe(expected); + }); +}); + +describe("supportsNativeMaintainVisibleContentPosition", () => { + const setPlatform = (nativeMvcpBreaksSnapFling: boolean) => { + ( + PlatformConfig as unknown as { nativeMvcpBreaksSnapFling: boolean } + ).nativeMvcpBreaksSnapFling = nativeMvcpBreaksSnapFling; + }; + + afterEach(() => { + setPlatform(false); + }); + + it("withholds the native prop from a snapping list on Android", () => { + setPlatform(true); + + expect( + supportsNativeMaintainVisibleContentPosition({ snapToInterval: 300 }) + ).toBe(false); + expect( + supportsNativeMaintainVisibleContentPosition({ pagingEnabled: true }) + ).toBe(false); + expect( + supportsNativeMaintainVisibleContentPosition({ snapToOffsets: [0, 300] }) + ).toBe(false); + }); + + it("keeps the native prop for a list that does not snap on Android", () => { + setPlatform(true); + + expect(supportsNativeMaintainVisibleContentPosition({})).toBe(true); + }); + + it("keeps the native prop on platforms whose fling survives a re-anchor", () => { + setPlatform(false); + + expect( + supportsNativeMaintainVisibleContentPosition({ snapToInterval: 300 }) + ).toBe(true); + expect(supportsNativeMaintainVisibleContentPosition({})).toBe(true); + }); +}); diff --git a/src/native/config/PlatformHelper.android.ts b/src/native/config/PlatformHelper.android.ts index 8a957c04a..07d922873 100644 --- a/src/native/config/PlatformHelper.android.ts +++ b/src/native/config/PlatformHelper.android.ts @@ -2,6 +2,7 @@ import { isRN083OrAbove } from "./versionCheck"; const PlatformConfig = { defaultDrawDistance: 250, + nativeMvcpBreaksSnapFling: true, supportsOffsetCorrection: true, trackAverageRenderTimeForOffsetProjection: true, isRN083OrAbove: isRN083OrAbove(), diff --git a/src/native/config/PlatformHelper.ios.ts b/src/native/config/PlatformHelper.ios.ts index fba643d07..68e5dbe50 100644 --- a/src/native/config/PlatformHelper.ios.ts +++ b/src/native/config/PlatformHelper.ios.ts @@ -2,6 +2,7 @@ import { isRN083OrAbove } from "./versionCheck"; const PlatformConfig = { defaultDrawDistance: 250, + nativeMvcpBreaksSnapFling: false, supportsOffsetCorrection: true, trackAverageRenderTimeForOffsetProjection: false, isRN083OrAbove: isRN083OrAbove(), diff --git a/src/native/config/PlatformHelper.ts b/src/native/config/PlatformHelper.ts index aa4b8ed67..a036b3452 100644 --- a/src/native/config/PlatformHelper.ts +++ b/src/native/config/PlatformHelper.ts @@ -1,5 +1,6 @@ const PlatformConfig = { defaultDrawDistance: 250, + nativeMvcpBreaksSnapFling: false, supportsOffsetCorrection: false, trackAverageRenderTimeForOffsetProjection: false, isRN083OrAbove: true, diff --git a/src/native/config/PlatformHelper.web.ts b/src/native/config/PlatformHelper.web.ts index ba9ca1024..53ec6e17d 100644 --- a/src/native/config/PlatformHelper.web.ts +++ b/src/native/config/PlatformHelper.web.ts @@ -1,5 +1,6 @@ const PlatformConfig = { defaultDrawDistance: 500, + nativeMvcpBreaksSnapFling: false, supportsOffsetCorrection: false, trackAverageRenderTimeForOffsetProjection: false, isRN083OrAbove: true, diff --git a/src/recyclerview/RecyclerView.tsx b/src/recyclerview/RecyclerView.tsx index 3c25e7e89..15564fbc6 100644 --- a/src/recyclerview/RecyclerView.tsx +++ b/src/recyclerview/RecyclerView.tsx @@ -49,6 +49,7 @@ import { useBoundDetection } from "./hooks/useBoundDetection"; import { adjustOffsetForRTL } from "./utils/adjustOffsetForRTL"; import { useSecondaryProps } from "./hooks/useSecondaryProps"; import { getInvertedTransformStyle } from "./utils/getInvertedTransformStyle"; +import { supportsNativeMaintainVisibleContentPosition } from "./utils/snapping"; import { StickyHeaders, StickyHeaderRef } from "./components/StickyHeaders"; import { ScrollAnchor, ScrollAnchorRef } from "./components/ScrollAnchor"; import { useRecyclerViewController } from "./hooks/useRecyclerViewController"; @@ -486,15 +487,27 @@ const RecyclerViewComponent = ( const shouldMaintainVisibleContentPosition = recyclerViewManager.shouldMaintainVisibleContentPosition(); + // A snapping list on Android cannot be given the native prop without its + // flings being retargeted, so offset corrections fall back to scrollTo there. + const canUseNativeMaintainVisibleContentPosition = + supportsNativeMaintainVisibleContentPosition(props); + const maintainVisibleContentPositionInternal = useMemo(() => { - if (shouldMaintainVisibleContentPosition) { + if ( + shouldMaintainVisibleContentPosition && + canUseNativeMaintainVisibleContentPosition + ) { return { ...maintainVisibleContentPosition, minIndexForVisible: 0, }; } return undefined; - }, [maintainVisibleContentPosition, shouldMaintainVisibleContentPosition]); + }, [ + maintainVisibleContentPosition, + shouldMaintainVisibleContentPosition, + canUseNativeMaintainVisibleContentPosition, + ]); const shouldRenderFromBottom = recyclerViewManager.getDataLength() > 0 && diff --git a/src/recyclerview/hooks/useRecyclerViewController.tsx b/src/recyclerview/hooks/useRecyclerViewController.tsx index 3689830d9..2ff2fd358 100644 --- a/src/recyclerview/hooks/useRecyclerViewController.tsx +++ b/src/recyclerview/hooks/useRecyclerViewController.tsx @@ -21,6 +21,7 @@ import { adjustOffsetForRTL } from "../utils/adjustOffsetForRTL"; import { RVLayout } from "../layout-managers/LayoutManager"; import { ScrollAnchorRef } from "../components/ScrollAnchor"; import { PlatformConfig } from "../../native/config/PlatformHelper"; +import { supportsNativeMaintainVisibleContentPosition } from "../utils/snapping"; import { WarningMessages } from "../../errors/WarningMessages"; import { useUnmountFlag } from "./useUnmountFlag"; @@ -181,7 +182,16 @@ export function useRecyclerViewController( !recyclerViewManager.animationOptimizationsEnabled ) { // console.log("diff", diff, firstVisibleItemKey.current); - if (PlatformConfig.supportsOffsetCorrection) { + // The anchor nudge only moves the scroll position through the + // native maintainVisibleContentPosition the ScrollView was given. + // A snapping list on Android is not given it (see snapping.ts), so + // there the correction has to drive the scroll position directly. + if ( + PlatformConfig.supportsOffsetCorrection && + supportsNativeMaintainVisibleContentPosition( + recyclerViewManager.props + ) + ) { // console.log("scrollBy", diff); scrollAnchorRef.current?.scrollBy(diff); } else { diff --git a/src/recyclerview/utils/snapping.ts b/src/recyclerview/utils/snapping.ts new file mode 100644 index 000000000..8319991a9 --- /dev/null +++ b/src/recyclerview/utils/snapping.ts @@ -0,0 +1,54 @@ +import { PlatformConfig } from "../../native/config/PlatformHelper"; + +interface SnapProps { + pagingEnabled?: boolean | null; + snapToInterval?: number | null; + snapToOffsets?: number[] | null; +} + +/** + * Whether the list snaps, using the same predicate ScrollView.js applies to + * decide the native `pagingEnabled` it hands Android: + * + * ```js + * pagingEnabled: Platform.select({ + * android: + * this.props.pagingEnabled === true || + * this.props.snapToInterval != null || + * this.props.snapToOffsets != null, + * }) + * ``` + * + * That flag is what gates `ReactHorizontalScrollView.flingAndSnap`, so matching + * it exactly covers every list that snaps and none that don't. + */ +export function isSnappingList(props: SnapProps): boolean { + return ( + props.pagingEnabled === true || + props.snapToInterval != null || + props.snapToOffsets != null + ); +} + +/** + * Whether the underlying ScrollView can be handed RN's native + * `maintainVisibleContentPosition`. + * + * Android's `MaintainVisibleScrollPositionHelper` re-anchors on every layout + * change of the first visible child, and a recycling list repositions its cells + * constantly. Each re-anchor calls `scrollToPreservingMomentum` -> + * `recreateFlingAnimation(x, Integer.MAX_VALUE)`, which cancels the in-flight + * snap animator and re-flings with the velocity `flingAndSnap` boosted 10x - + * safe only while that call also clamped it with `minX == maxX == targetOffset`. + * Unclamped, a backward swipe runs all the way to offset 0 instead of landing on + * the previous snap point. + * + * The helper watches the cells themselves, not FlashList's ScrollAnchor, so + * there is nothing to suppress on the JS side. Keeping the prop off for snapping + * lists is what stops it. + */ +export function supportsNativeMaintainVisibleContentPosition( + props: SnapProps +): boolean { + return !(PlatformConfig.nativeMvcpBreaksSnapFling && isSnappingList(props)); +}