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.
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.

2 participants