Scroll-view: honour auto_scroll_to, gap on Android, and chrome collapse - #27
shanerbaner82 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
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.
Nothing links focus to scrolling. `NativeUIScrollViewRenderer` scrolls on `keyboardWillShow`, but only for `scroll-anchor="bottom"` and only to the bottom anchor — a chat policy — and `NativeUITextInputCore` has no scroll proxy at all. SwiftUI shrinks the screen by the keyboard height, which is enough when the field is the last thing on the page: shrink the viewport and the composer lands just above the keyboard. That is why this has never come up. It is not enough anywhere else. Shrinking guarantees the field is inside the SCROLLABLE CONTENT, not that it is on screen — so on a login form, tapping the password field mid-page leaves it exactly where it was, now underneath the keyboard, and the user types blind into a masked field. The vertical scroll view already builds a `ScrollViewReader` for bottom anchoring, so this needs no new machinery: publish that proxy into the environment, and let the focused input scroll itself to `.center`. Deliberate choices, each of which is the reason this is safe: - The proxy is WITHHELD from a bottom-anchored scroll view. That mode already owns a keyboard policy and it is a different one — a chat log wants the latest message above the keyboard, not the composer centered in what is left. Two policies driving one proxy would race and the later would win by accident. Withholding it also means the environment value is nil in every case where scrolling isn't this descendant's business: sheets, modals, fixed screens, chat logs. - The scroll runs on FOCUS, not on `keyboardWillShow`. Moving from the email field to the password field never re-shows the keyboard, and that is precisely when a form needs this. - It is deferred ~0.35s, past the keyboard's own presentation, so it centers against the already-shrunk viewport. Centering against the full height first would place the field in the middle of a screen about to lose its bottom half — back under the keyboard. - `.id(node.id)` on the field is what makes it addressable. That id is already the ForEach identity of every node in the tree, so it is stable across republishes, and it is applied to the view `body` returns rather than to the struct, so it cannot reset the value/sync `@State`. Android is NOT covered, and I believe it does not need to be: Compose's `BasicTextField` already requests bring-into-view on focus, and the IME resizes the window, so a field inside a scrollable column is brought up without help. If that turns out to be wrong on the M3 text fields this repo uses, it should be its own change rather than a guess bolted onto this one. Note for whoever merges this alongside #27: that PR touches the same `verticalScroll` body and adds an `auto-scroll-to` index the author drives explicitly. The two mechanisms are independent — this one publishes the proxy, that one consumes it locally — but an author who sets both is asking two things to scroll one view, and the same argument used above for `scroll-anchor="bottom"` says the explicit index should win. The one-line guard is `stickBottom || hasAutoScroll`. Co-authored-by: Simon Hamp <simon.hamp@me.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
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