Skip to content

Implement ScrollView::autoScrollTo() in both renderers - #67

Open
MhAhmadAli wants to merge 1 commit into
NativePHP:mainfrom
MhAhmadAli:fix/scroll-view-auto-scroll-to
Open

Implement ScrollView::autoScrollTo() in both renderers#67
MhAhmadAli wants to merge 1 commit into
NativePHP:mainfrom
MhAhmadAli:fix/scroll-view-auto-scroll-to

Conversation

@MhAhmadAli

Copy link
Copy Markdown

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:

ScrollView::make(...)->autoScrollTo(1)->toArray(...)
  => type: scroll_view
     props: {"auto_scroll_to":1}

auto_scroll_to appears zero times in this repo — neither NativeUIScrollViewRenderer.swift nor ScrollViewRenderer in ContainerRenderers.kt looks at it. scroll_anchor right next to it is read on both platforms; this one was just never wired up. The docs describe it, and mobile-air's own BenchmarkComponent drives 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:

Behaviour Rule
Meaning of the index A direct child, as documented
Absent / negative No target — the author isn't driving scroll position
Index past the end Clamped, not dropped (see below)
When it fires When the resolved index changes — not on every publish
First vs. later First application jumps, later ones animate (same rule scroll-anchor follows)
vs. scroll-anchor="bottom" Explicit auto_scroll_to wins — both drive the same list state
Resting position Start of the viewport (.top / .leading, matching Compose's scrollToItem)

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 ScrollViewReader and the Android LazyRow an 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 pass
  • php -l over src/ — 86 files clean
  • swiftc -parse over all 46 resources/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:

same index republished 5x           -> [7]        (one scroll, reader not yanked)
index changes 0,0,4,4,9             -> [0, 4, 9]  (one per distinct target)
target 5 while list grows 2->4->6   -> [1, 3, 5]  (clamped target re-fires as it fills)
fixed target 2, list grows 10->12   -> [2]        (content churn doesn't drag the user)
no prop across publishes            -> []

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 of ContainerRenderers.kt produces 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:

  1. There is no auto-scroll-to Blade attribute, so <native:scroll-view> still can't express this — only the PHP builder can. scroll-anchor is mapped in NativeElementCollector; this isn't.
  2. BenchmarkComponent::renderLargeListFpsScreen() calls autoScrollTo($itemCount - 1) on a scroll view whose only direct child is a wrapping Column, so index 9999 clamps to the single child. That call site needs restructuring for the benchmark to measure what it claims to.

`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.
@gwleuverink

Copy link
Copy Markdown

Nice work tracing this one end to end, and thanks for being upfront about not having a device. I put the branch on an iPhone 17 Pro simulator and an API 36 emulator to close that gap. The index maths is right, and on Android everything behaves the way your table says. Three things behave differently on a real screen. The first two have fixes I've built and run, the third is a design call I'd rather make with you.

How I read the results, so the numbers below mean something: every row in the test screen is a solid block of colour that encodes its own number, so a screenshot tells you exactly which row is at the top and how far into it the list has scrolled.

1. On iOS the list goes where you told it last time

Change the index and nothing else, over and over:

set 3  -> list doesn't move
set 6  -> list goes to 3
set 20 -> list goes to 6
set 8  -> list goes to 20
set 8  -> list doesn't move   (right, the index didn't change)
set 14 -> list goes to 8

It is always one behind, on three separate builds, and the same on a horizontal list. If a screen opens with no index and gets one later, it never scrolls at all, because that first change is measured against nothing.

The cause is .onChange(of:perform:). When the value changes, it hands your closure the new index but runs it against the previous version of the view, so applyAutoScroll looks up autoScrollIndex and node.children again and gets the old ones. .onAppear reads the current view, which is why opening a screen lands correctly and only later changes are wrong.

The two-parameter version reads the current view, and the app targets 18.2 so it's available:

.onChange(of: autoScrollIndex) { _, _ in
    applyAutoScroll(proxy: proxy, anchor: .top, animated: true)
}

Both handlers need it, the .leading one too. With that in, the sequence above lands 6 for 6, a target that was out of reach still fires once the list grows to it, a render that swaps the rows and the index at the same time lands, and the same holds inside a bottom sheet and a sheet pane.

Two things worth knowing. Passing the new index into applyAutoScroll instead isn't enough on its own, because the row list is old in that same closure, so a render that changes rows and index together still lands wrong. And swiftc -parse was never going to catch this: it doesn't check deprecations, so it missed the warnings too, and the bug lives in the SwiftUI event wiring rather than in the maths your standalone test exercised.

2. Adding or removing rows moves the reader

Tying the scroll to the row it settles on is the right instinct, but that row moves when the content changes, not just when the author changes their mind.

Removing rows: index 25 of 30, I scrolled back to the top by hand, then the list dropped to 20 rows with the index untouched. The reader gets pulled down to the new last row.

Adding rows: index 20 held fixed while the list filled 5, 10, 15, 21. It scrolls twice on the way before landing, so a list that streams in stutters after the reader instead of arriving once.

Both come from clamping an index that's past the end. If you ignore it instead, you keep the good part, the list still jumps to the row the moment it exists, and both symptoms go:

private var autoScrollIndex: Int? {
    let requested = node.props.getInt("auto_scroll_to", default: -1)
    guard requested >= 0, requested < node.children.count else { return nil }

    return requested
}
private fun resolveAutoScrollTarget(node: NativeUINode): Int {
    val requested = node.props.getInt("auto_scroll_to", -1)
    if (requested < 0 || requested >= node.children.size) return -1

    return requested
}

I built that and ran it on both platforms:

  • Removing rows: same setup, reader at the top, list drops 30 to 20. They stay at the top. Before, they were dragged to the end.
  • Adding rows: same 5, 10, 15, 21 fill. One scroll, at 11.5 seconds, the moment the 21st row made index 20 real. Before, it scrolled at 8.4 and 11.4 seconds on the way there.
  • Nothing else changed: a normal index still parks that row at the top, changing from one valid index to another still lands, a negative or missing index still does nothing, horizontal lists are unaffected, scroll-anchor="bottom" still loses to an explicit index, and the fill-height case is untouched. Same on iOS.

Two knock-on effects, better decided than discovered. An index past the end now does nothing at all, so autoScrollTo(99) on 30 rows no longer lands on the last one. And on a list that also has scroll-anchor="bottom", an index that's still out of reach leaves the anchor in charge until the row exists: I watched it sit at the bottom with 14 rows, then jump to row 25 once the list reached 30. For a chat that's probably what you'd want, but it does mean the "clamped" row of your table needs rewriting alongside the code.

3. Coming back to a list starts it over

This is the one I'd most like your thinking on. It needs no unusual layout, and the fix in point 1 doesn't touch it.

A horizontal row of cards inside a normal vertical feed. The row opened on index 10, I swiped it across to 15, scrolled the feed down past it and back up, and it was sitting on 10 again. Twice. On iOS the same thing happens when the row comes back into view. So wherever the reader had dragged that row to is thrown away every time it leaves the screen and returns.

It shows up again when you leave a screen and come back, and when you close and reopen a panel, where it isn't even consistent between panels:

panel opens on the current index index changed while open close and reopen
BottomSheet yes iOS no, Android yes starts over, reader's position lost
Modal yes iOS no, Android yes starts over
SheetPane yes iOS no, Android yes stays where it was

Sheets and modals build their contents fresh each time they open. The sheet pane builds once at its tallest size and only slides, so it never starts over.

Underneath it's the same thing in each case: the flag that remembers "I've already scrolled this list" belongs to the on-screen view, so it dies when that view goes away and is back to zero when it returns. For it to survive, it has to belong to the list itself rather than to the view showing it. On Android rememberSaveable gets you most of the way; on iOS it needs something keyed off node.id. That's a bigger decision than the other two and it's yours to make, so I stopped at the diagnosis rather than guessing. Tell me which way you'd take it and I'll test it.

One more for while you're in there: on iOS the one-behind problem is worse inside a panel, because the index it remembers is the one the panel opened with. So a sheet that's meant to open a list and scroll to a particular message shows nothing at all rather than being one step off. Point 1 fixes that too, I checked.

Smaller things

axis="both" isn't excluded on both platforms. iOS skips it on purpose, but Android has nothing to skip: it never looks at axis, and both() only sets axis and never horizontal, so the view falls through to the vertical path and scrolls anyway. Index 3 with six 300pt children scrolled to the third one. Not yours to fix, but the description reads as though both sides opt out.

The benchmark still won't move, on either platform. With the rows wrapped in a single Column the index means "the first and only child" today, and means nothing at all if you take point 2, so either way it's worth keeping out of any merge note that sounds like the FPS scenario is fixed. You already say so in "Not included".

Two I couldn't reach: the drawer wouldn't open for me without touching the screen, and accordions, carousels and tab bodies all throw their contents away when hidden, so I'd expect them to behave like the sheet row above without having watched it happen.

Everything else behaves

Jumping to a row once it exists, doing nothing for a negative or missing index, beating scroll-anchor="bottom" and handing control back when the index is removed, horizontal lists parking the row at the left edge, the fill-height case, and leaving a reader alone through 50 re-renders and a hand swipe. The first scroll on a screen arrives instantly and later ones animate over about a quarter second. And a scroll view with no index set renders pixel for pixel the same as before your change on both platforms, so the new ScrollViewReader and list state cost nothing on the untouched path.

Requesting changes for points 1 and 2, both small and both verified on a device. Point 3 is the one worth settling together before this lands.

@gwleuverink gwleuverink left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only submit review with a comment. See my last message 👆🏻

@gwleuverink

Copy link
Copy Markdown

One more, from wondering what a search field would do to this.

Asking for the same row twice never fires, so a list can't be sent back to the top once its results are replaced.

The screen is a search where the results are the scroll view's children and the author wants the top of the results for every query:

public function render(): Element
{
    return ScrollView::make(...$this->resultsFor($this->query))
        ->autoScrollTo(0)
        ->fillWidth()
        ->flexGrow(1);
}

resultsFor() returns different rows each time the query changes. autoScrollTo(0) never changes, because the author's intent never changes.

Driven on Android:

  1. First query returns 16 rows. The list opens at the top. Correct.
  2. The reader scrolls down, about two thirds of the way.
  3. Second query returns 16 completely different rows. The list doesn't move. The reader is two thirds of the way down results they've never seen.
  4. Third query returns 14 rows, so the count changes too. Still doesn't move.

Index 0 to index 0 isn't a change, so nothing fires, and there's no way for the author to ask again. I drove the queries from a probe that swaps the row set on command rather than a real text field, but the tree the renderer receives is what a search screen would publish.

Keying on the identity of the row at that index doesn't help. Element ids are positional, so two different result sets hand the renderer the same ids:

results A (alpha, beta, gamma)   -> child ids 2,3,4
results B (delta, epsilon, zeta) -> child ids 2,3,4

I built that version and it changes nothing on a device. It also survives the earlier suggestion about ignoring out-of-range indices, since 0 stays 0 either way.

Something that does work

Let the author re-arm it with a token the renderer watches alongside the index. On Android that's reading the prop, threading it into AutoScrollToEffect, and adding it to the effect key:

val autoScrollToken = node.props.getString("auto_scroll_token", "")
// ... passed through to
LaunchedEffect(targetIndex, token) {

On iOS it's one computed property:

private var autoScrollTrigger: String? {
    guard let index = autoScrollIndex else { return nil }

    return "\(index)|" + node.props.getString("auto_scroll_token", default: "")
}

watched by .onChange(of: autoScrollTrigger) in place of .onChange(of: autoScrollIndex). On the PHP side the author would pass whatever already identifies the query, something like:

ScrollView::make(...$this->resultsFor($this->query))
    ->autoScrollTo(0, token: $this->query)

I built this and ran it on both platforms.

Android, index held at 0 throughout: each new query with a new token puts the list at the top of the new results, including from two thirds of the way down. Then with the token unchanged, 15 re-renders and a content growth left the reader exactly where they were.

iOS, index held at 5, starting from a list too short to scroll so that firing and not firing land in different places: with the token changed the list moved to row 5 of the new results, with the token unchanged it stayed at the top. Same behaviour on both.

The builder argument is in mobile-air rather than here, so this is only half a suggestion. But the renderer half is small, and it's the only shape I found that fixes the search case without also re-scrolling on content changes the author didn't ask about.

Other things I checked

Two things I checked that turned out fine: rows inserted above the list while a sheet is closed shift which row the index names, which is the prop behaving as documented, and rotating the device keeps the reader where they were rather than snapping back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants