Implement ScrollView::autoScrollTo() in both renderers - #67
Open
MhAhmadAli wants to merge 1 commit into
Open
Conversation
`ScrollView::autoScrollTo($index)` has been a dead API: the PHP builder sets an `auto_scroll_to` prop, it serializes and crosses the wire intact, and then neither renderer ever reads it. Nothing scrolls. Read the prop on both platforms and drive the list from it. Semantics, matched across iOS and Android: - The index names a DIRECT child, as the docs describe. - Absent or negative means "no target" — the author isn't driving the scroll position at all. - An index past the end is CLAMPED, not dropped. A list that is still filling in can legitimately be shorter than the index for a frame or two; clamping lands on the last child now and re-fires as the real target arrives, rather than leaving the list parked. - The scroll fires when the RESOLVED index changes, not on every publish. A re-render carrying the same index leaves a reader who has scrolled away exactly where they were. A clamped target still re-fires on its own once the list grows past it. - First application jumps, later ones animate — the same rule `scroll-anchor="bottom"` already follows. - An explicit `auto_scroll_to` takes precedence over `scroll-anchor="bottom"`; both drive the same list state, and the author naming a specific child is the more specific instruction. - The target parks at the start of the viewport on both platforms (`.top` / `.leading` to match Compose's `scrollToItem`). Horizontal scroll views are covered too: the iOS one gains a `ScrollViewReader` and the Android `LazyRow` an explicit list state, neither of which it had. 2D (`axis="both"`) is deliberately excluded — children there are layered at their own frames rather than sequenced, so an index has no position to scroll to.
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 the dead API reported in NativePHP/mobile-air#366.
The problem
ScrollView::autoScrollTo($index)builds a prop, the prop crosses the wire intact, and then nothing reads it. Confirmed end to end:auto_scroll_toappears zero times in this repo — neitherNativeUIScrollViewRenderer.swiftnorScrollViewRendererinContainerRenderers.ktlooks at it.scroll_anchorright next to it is read on both platforms; this one was just never wired up. The docs describe it, and mobile-air's ownBenchmarkComponentdrives its "Large List 10k FPS" scenario with it — so that benchmark has been measuring a list that never moves.The fix
Read the prop on both platforms and drive the list from it. Semantics are matched across iOS and Android:
scroll-anchorfollows)scroll-anchor="bottom"auto_scroll_towins — both drive the same list state.top/.leading, matching Compose'sscrollToItem)Two choices worth calling out:
Clamping rather than ignoring an out-of-range index. PHP publishes the index and the children in the same frame, but a list still filling in (paginated history, a streamed response) can legitimately be shorter than the index for a frame or two. Clamping lands on the last child now and re-fires as the real target appears; dropping it would leave the list parked wherever it was.
Keying the effect on the resolved index, not the raw prop. This is what stops a re-publish from yanking the reader: a screen that re-renders for an unrelated reason carries the same index and nothing fires. It also makes a clamped target re-fire on its own once the list grows past it — the resolved value moves even though the prop didn't.
Horizontal scroll views are covered too — the iOS one gains a
ScrollViewReaderand the AndroidLazyRowan explicit list state, neither of which it had. 2D (axis="both") is deliberately excluded and commented as such: children there are layered at their own frames rather than sequenced, so an index has no position to scroll to.Testing
Everything below ran in Docker.
Repo CI, at parity
pest— 217 passed (735 assertions)pint --test— 104 files passphp -loversrc/— 86 files cleanswiftc -parseover all 46resources/ios/*.swift— clean (Swift 6.1)Beyond CI — the resolution logic, executed. The resolver was extracted verbatim from the committed files into standalone programs and run, so the shipped expressions are what get exercised. Both platforms were given the same 13-case truth table (in range, boundary, one-past-end, far-past-end, negative, empty children, single child, and the benchmark's own 1-child/index-9999 shape) and agree on every case.
A publish-sequence test then replays realistic frame sequences through the real resolver and counts the scrolls the list would perform:
Kotlin type-check against real Compose. The new functions plus the exact call-site wiring were compiled against real
androidx.compose.*artifacts (Compose Multiplatform 1.7.3 + Compose compiler plugin, Kotlin 2.1.0): zero errors, zero warnings from frontend analysis. Full-file compilation ofContainerRenderers.ktproduces the same four error classes before and after this change (all classpath-absence artefacts), so nothing structural was introduced.Negative controls, because a green check that can't fail proves nothing. Injected bugs were each caught: a wrong import, a wrong argument type, a wrong parameter name, and a suspend call outside a coroutine — plus mutation of the clamp and the negative-guard, which produced 5, 4 and 6 failing assertions respectively.
What I could not test: actual on-device scroll behaviour. There is no simulator or emulator here, and full compilation needs the consuming Xcode/Gradle project. The SwiftUI and Compose wiring is reviewed and type-checked, not run — worth a look on a device before merge, particularly the interaction with
scroll-anchor="bottom".Not included
Two follow-ups belong in mobile-air, not here:
auto-scroll-toBlade attribute, so<native:scroll-view>still can't express this — only the PHP builder can.scroll-anchoris mapped inNativeElementCollector; this isn't.BenchmarkComponent::renderLargeListFpsScreen()callsautoScrollTo($itemCount - 1)on a scroll view whose only direct child is a wrappingColumn, so index 9999 clamps to the single child. That call site needs restructuring for the benchmark to measure what it claims to.