fix: fixed crash when selected text scrolls offscreen#1045
Conversation
Signed-off-by: Jonas Kalderstam <[email protected]>
Signed-off-by: Jonas Kalderstam <[email protected]>
Signed-off-by: Jonas Kalderstam <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR addresses crashes when selecting text in long articles that scroll offscreen by switching the article reader layout away from a lazy list to a fully composed scrollable column, and attempts to preserve anchor-link scrolling.
Changes:
- Replace
LazyColumn-based reader body withColumn+verticalScrollinside aSelectionContainer. - Add
ColumnArticleContentto renderLinearArticleelements in a non-lazy column and report element positions. - Update anchor-link handling to scroll via
ScrollStaterather thanLazyListState.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
app/src/main/java/com/nononsenseapps/feeder/ui/compose/html/LinearArticleContent.kt |
Adds ColumnArticleContent and element-position reporting for anchor scrolling in a non-lazy layout. |
app/src/main/java/com/nononsenseapps/feeder/ui/compose/feedarticle/ReaderView.kt |
Replaces lazy list + scrollbar with Column + verticalScroll under SelectionContainer to prevent selection-related crashes. |
app/src/main/java/com/nononsenseapps/feeder/ui/compose/feedarticle/ArticleScreen.kt |
Wires new scroll state, uses ColumnArticleContent, and implements anchor scrolling using recorded element positions. |
Comments suppressed due to low confidence (1)
app/src/main/java/com/nononsenseapps/feeder/ui/compose/feedarticle/ArticleScreen.kt:529
- There’s a block of dead, commented-out
LoadingItemcode left at the bottom of the file. Since the lazy-list approach has been removed, this can be deleted to keep the file clean and avoid confusion about the expected pattern.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ColumnArticleContent( | ||
| articleContent = viewState.articleContent, | ||
| onLinkClick = { link, index -> | ||
| if (index != null) { | ||
| coroutineScope.launch { | ||
| articleListState.animateScrollToItem(offsetCounter + index) | ||
| if (index != null && elementPositions.containsKey(index)) { | ||
| // Anchor link - scroll to the element position | ||
| val yPosition = elementPositions[index] | ||
| if (yPosition != null) { | ||
| coroutineScope.launch { | ||
| articleScrollState.animateScrollTo(yPosition.toInt()) | ||
| } | ||
| } | ||
| } else { | ||
| // External link - open in browser/custom tab | ||
| activityLauncher.openLink( | ||
| link = link, | ||
| toolbarColor = toolbarColor, | ||
| ) | ||
| } | ||
| }, | ||
| onElementPositioned = { index, yPosition -> | ||
| elementPositions[offsetCounter + index] = yPosition | ||
| }, |
There was a problem hiding this comment.
Anchor-link scrolling can’t work as written: elementPositions is populated with keys offsetCounter + index, but the click handler checks/reads elementPositions[index]. This means anchors will often be treated as external links (or scroll to the wrong element) once there are header sections (summary/enclosure/image) before the article content. Use a consistent keying scheme (either store and look up by raw article-element index, or consistently include the offset).
| // Track Y positions of article elements by index for anchor link scrolling | ||
| val elementPositions = remember { mutableMapOf<Int, Float>() } |
There was a problem hiding this comment.
elementPositions is remembered without being keyed to the current article. If the same ArticleContent composable instance is reused for a different viewState.articleId, the map can retain stale positions from the previous article and cause incorrect anchor scrolling. Consider keying the remember to viewState.articleId (or clearing when the article changes).
| // Track Y positions of article elements by index for anchor link scrolling | |
| val elementPositions = remember { mutableMapOf<Int, Float>() } | |
| // Track Y positions of article elements by index for anchor link scrolling. | |
| // Keyed by articleId so positions are reset when a different article is shown. | |
| val elementPositions = remember(viewState.articleId) { mutableMapOf<Int, Float>() } |
| if (index != null && elementPositions.containsKey(index)) { | ||
| // Anchor link - scroll to the element position | ||
| val yPosition = elementPositions[index] | ||
| if (yPosition != null) { | ||
| coroutineScope.launch { | ||
| articleScrollState.animateScrollTo(yPosition.toInt()) | ||
| } |
There was a problem hiding this comment.
animateScrollTo(yPosition.toInt()) is using a Y value produced from positionInRoot(). That coordinate is relative to the root Compose layout (and changes with the current scroll offset), while ScrollState.animateScrollTo() expects a scroll offset within the scrollable content. This will produce incorrect anchor scrolling (especially when not at scroll=0, or if the scroll container isn’t at y=0). A more robust approach is to use BringIntoViewRequester per element (store requesters by index) or compute a content-relative offset (e.g., adjust by the scroll container’s top position and current scroll value).
| BoxWithConstraints( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .onGloballyPositioned { coordinates -> | ||
| onElementPositioned(index, coordinates.positionInRoot().y) | ||
| }, |
There was a problem hiding this comment.
ColumnArticleContent attaches onGloballyPositioned for every element even when the default onElementPositioned no-op is used. For long articles this adds measurable layout overhead on every scroll/layout pass. Consider making the callback nullable/optional and only adding onGloballyPositioned when anchor scrolling is actually needed.
Signed-off-by: Jonas Kalderstam <[email protected]>
Incorporate upstream changes from master (v2.17.0, v2.18.0) while preserving all AI features developed on ai-features branch. Upstream features incorporated: - Paging mode with volume buttons and tap zones (spacecowboy#1062) - Home screen widget support (spacecowboy#1018) - Blocklist apply-to-summaries toggle (spacecowboy#1003) - AGP 9.0.0 + OkHttp 5.3.2 upgrade (spacecowboy#1051) - LazyColumn to Column migration for text selection crash fix (spacecowboy#1045) - Read item opacity (spacecowboy#996) - Bug fixes: sync deletion, compact layout scroll, network crash - 20+ language i18n updates via Weblate AI features preserved: - Multi-provider AI architecture (OpenAI + Anthropic) - Per-paragraph translation with circular progress - Mikepenz markdown rendering for summaries - Text selection menu customization - Summary/translation cancellation support Resolved 10 merge conflicts (3 HIGH, 3 MEDIUM, 4 LOW). Deleted superseded openai/ package and LeakCanary references. Re-indexed SettingsViewModel combine() params (35 total). Added mainActivityViewModel parameter to all 13 navigation destinations.
fixes #898
fixes #686