Scroll-view: honour auto_scroll_to, gap on Android, and chrome collapse - #27
Scroll-view: honour auto_scroll_to, gap on Android, and chrome collapse#27shanerbaner82 wants to merge 1 commit into
Conversation
Three related scroll-view renderer gaps, all surfaced while building a chat screen: auto_scroll_to had a PHP setter (ScrollView::autoScrollTo()) but no consumer on either platform — it was written into the wire payload and dropped. BenchmarkComponent relies on it to scroll a long list, so that scenario has been measuring a static list. iOS reuses the ScrollViewReader already on the vertical branch and scrolls to the target child's id; Android drives LazyColumn's listState by index. Both jump without animation on first appear and animate later moves, matching the existing scroll-anchor path. An explicit index wins over `scroll-anchor="bottom"`. Out-of-range and negative indices are no-ops. The `gap-*` class was read on iOS (into LazyVStack/LazyHStack spacing) and silently dropped on Android, because a lazy list arranges its own items rather than going through the flex engine. That gap matters more than it looks: the workaround is wrapping content in a <column> to get spacing, which collapses the whole list into a single lazy item and defeats virtualization entirely. Programmatic scrolls emit no nested-scroll deltas, so a collapsing top bar never learned the content moved. Both scroll paths now drive the new ChromeScrollController from nativephp/mobile. Skipped on the initial jump — opening a screen should not animate the chrome shut. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e81eb72 to
2d849ff
Compare
shanerbaner82
left a comment
There was a problem hiding this comment.
Rebased onto main (force-pushed e81eb72 → 2d849ff) and reviewed the result.
The conflict
This branched at 100931a; main is 40+ commits ahead and three of them rewrote exactly what this touches — #48 extracted the iOS vertical path into a verticalScroll(...) helper and added hasFillHeightChild branches on both platforms, and #50 (three commits ago) added atBottom tracking plus repinToBottom for keyboard show/hide.
Resolved by keeping main's structure and re-applying this PR's behaviour into it:
| Resolution | |
|---|---|
| iOS horizontal | Kept the ScrollViewReader wrap; dropped the now-stray outer .scrollDismissesKeyboard (moved inside the reader) |
| iOS vertical | Re-applied the auto-scroll onAppear branch + onChange(of: autoScrollIndex) onto main's verticalScroll helper, preserving atBottom / onDisappear / both keyboard observers |
| iOS locals | autoScrollIndex / hasAutoScroll are now computed properties — verticalScroll is a separate helper and can't see body's locals. Follows the hasFillHeightChild precedent |
| Android | Applied verticalArrangement = Arrangement.spacedBy(gap) to both LazyColumns, including main's new fill-height one, which this PR predates |
swiftc -parse clean, braces balance, prop accessor signatures checked against sibling call sites.
Review
Blocker confirmed. LocalChromeScrollController does not exist on core's main (git grep on origin/main finds nothing) and NativePHP/mobile-air#241 is still open — this will not compile. Worth noting the dependency is broader than the header says: ScrollView / autoScrollTo() live entirely in core, so #241 gates the whole feature, not just the chrome seam.
One issue is new as of the rebase — the iOS keyboard re-pin is dead for auto-scroll lists (inline). The rest predate it.
Details inline.
| // Explicit index targeting (`auto-scroll-to`) wins over bottom | ||
| // anchoring when both are set — the author named a specific child, | ||
| // so honour that rather than yanking them to the end. | ||
| let stickBottom = !hasAutoScroll && node.props.getString("scroll_anchor", default: "") == "bottom" |
There was a problem hiding this comment.
New as of the rebase: this switches off everything #50 just added.
stickBottom is now !hasAutoScroll && ..., and main's repinToBottom guards on stickBottom. So when auto-scroll-to is set:
- the
Color.clearbottom anchor never renders (it's insideif stickBottom) atBottomnever updates- neither
keyboardWillShownorkeyboardWillHideobserver fires
The keyboard shrinks the scroll viewport, autoScrollIndex hasn't changed so no onChange fires, and the target message drifts out from behind the input row — exactly the bug #50 fixed for scroll-anchor="bottom" three commits ago.
The two chat paths have silently diverged, and a chat screen is this PR's motivating use case. Either drive repinToBottom from the auto-scroll target too (re-pin to node.children[autoScrollIndex].id rather than the bottom anchor), or make the keyboard observers fire on stickBottom || hasAutoScroll.
| listState.scrollToItem(autoScrollIndex) | ||
| } else { | ||
| listState.animateScrollToItem(autoScrollIndex) | ||
| chromeScroll?.collapse() |
There was a problem hiding this comment.
collapse() ignores which way the list moved.
On the stickBottom path below, the movement is always toward the bottom, so collapsing the top bar is right. On this path the index is author-controlled and can move backwards — auto-scroll-to jumping to an earlier child scrolls the content up, and collapsing a large title while the user travels toward the top of the list is inverted.
Gating on direction (compare against listState.firstVisibleItemIndex before scrolling, collapse only when the target is further down) would keep the chrome honest.
| LazyRow(modifier = modifier) { | ||
| val rowState = rememberLazyListState() | ||
|
|
||
| LaunchedEffect(autoScrollIndex) { |
There was a problem hiding this comment.
The horizontal branch always animates, including the first scroll.
There's no didInitialScroll gate here, so animateScrollToItem runs on first composition too — opening a horizontal list at an index visibly animates in from wherever it started.
That contradicts the stated design ("Both jump without animation on first appear… and animate later moves"), which the Android vertical branch and both iOS branches do implement. Worth reusing the same didInitialScroll pattern, or a local equivalent.
| val stickBottom = !hasAutoScroll && node.props.getString("scroll_anchor", "") == "bottom" | ||
| val listState = rememberLazyListState() | ||
| val didInitialScroll = remember { mutableStateOf(false) } | ||
| val contentSignal = if (stickBottom) totalDescendants(node) else 0 |
There was a problem hiding this comment.
The auto-scroll path has no content signal at all.
contentSignal is forced to 0 whenever hasAutoScroll is true (since stickBottom is then false), and the LaunchedEffect below keys on stickBottom, contentSignal, autoScrollIndex — so with auto-scroll active, only a change to the index can re-fire it.
If the target index is already valid at first composition but the children arrive afterwards without the index changing, the guard autoScrollIndex in node.children.indices fails on the only run and the scroll never happens. iOS has the same shape (onAppear once, plus onChange on the index only).
Narrow, but it's the async-message-load case — and it's silent when it happens. Keying on totalDescendants(node) unconditionally would cover it.
| // A programmatic scroll emits no nested-scroll deltas, so a | ||
| // collapsing top bar never learns the content moved and a large | ||
| // title stays stranded fully expanded. Drive it explicitly. | ||
| val chromeScroll = LocalChromeScrollController.current |
There was a problem hiding this comment.
Confirmed this doesn't resolve: LocalChromeScrollController is absent from core's main, and mobile-air#241 is still open. Nothing else in this repo references it (grep -rn 'LocalChromeScrollController' resources/ returns only this line), so there's no existing import path to lean on either — the wildcard com.nativephp.mobile.ui.nativerender.* import is doing the work, and it will fail to resolve until #241 lands and ships.
Also, the description says "Both scroll paths now drive core's ChromeScrollController" — only the Android vertical path does. LazyRow doesn't, and iOS doesn't touch chrome anywhere. Worth correcting so the next reader doesn't go looking for the iOS half.
Three scroll-view renderer gaps, all surfaced while building a chat screen.
Important
Depends on NativePHP/mobile-air#241 — this consumes
ChromeScrollControllerfrom core and will not compile without it. Merge and release that first.1.
auto_scroll_towas never implementedScrollView::autoScrollTo()sets the prop, but no renderer on either platform read it — it was written into the wire payload and dropped on the floor.BenchmarkComponent.php:992relies on it to scroll a long list, so that benchmark scenario has been measuring a static list.ScrollViewReaderalready on the vertical branch, scrolling to the target child's id; the horizontal branch gains a reader so it behaves the same.LazyColumn's existinglistStateby index;LazyRowgains a state to drive.Both jump without animation on first appear (opening at a target shouldn't look like a scroll) and animate later moves, matching the existing
scroll-anchorpath. An explicit index wins overscroll-anchor="bottom". Negative and out-of-range indices are no-ops.2.
gap-*was ignored on AndroidThe flex engine lays out ordinary containers, but a lazy list arranges its own items — so
gapwas read on iOS (intoLazyVStack/LazyHStackspacing) and silently dropped on Android.That gap matters more than it looks. Without it the only way to space a list is wrapping it in a
<column>, which collapses the entire list into a single lazy item — killing virtualization. Measured on a real chat log:directChildren=1,canScrollForward=false, andfirstVisibleItemScrollOffsetclimbing past 1000px inside one item.3. Programmatic scrolls left a collapsing top bar expanded
Both scroll paths now drive core's
ChromeScrollControllerafter moving the list. Skipped on the initial jump — opening a screen shouldn't animate the chrome shut.Compatibility — please read
Two user-visible behaviour changes for existing apps:
gapon a scroll-view now spaces on Android. Any app relying on the current no-op will see layouts shift. Arguably the fix, but it is a change.autoScrollTo()now scrolls. Anyone who called it and got nothing will now get scrolling.Also
auto_scroll_totaking precedence overscroll_anchor— previously moot, sinceauto_scroll_todid nothing.Testing status — partial, please review accordingly
auto_scroll_to(both platforms) and the Android gap fix: NOT yet exercised on device. The test app usesscroll-anchor="bottom"with a wrapped log, so neither path was hit. Reviewed against surrounding code, imports verified, but they want a real build before merge.🤖 Generated with Claude Code