Wrong scroll position if page is loaded when only the placeholders are visible on screen [170027529]

Fixed

Bug

androidx-bug-bounty

Status Update

No update yet.

Description

me...@yhpark.io

created issue #1

Oct 5, 2020 04:11AM

Component used: androidx.paging:paging-runtime

Version used: 3.0.0-alpha07

Discovered a bug where the scroll position is not preserved when page data is loaded outside visible area while only the placeholders are visible. This can happen when a PagingSource with placeholders enabled takes a while to load the data while user is scrolling through the recyclerview.

Sample project: Issue A from https://github.com/yhpark/androidx-paging3-issues

Comments

yb...@google.com <yb...@google.com> Feb 22, 2021 11:30PM

Reassigned to yb...@google.com.

yb...@google.com <yb...@google.com> #2Feb 25, 2021 06:05AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

yb...@google.com <yb...@google.com> #3Mar 9, 2021 06:15AM

ok finally have a prototype that fixes all cases in your application.

https://android-review.googlesource.com/c/platform/frameworks/support/+/1620992/3

i'll update the bug once it is merged.

yb...@google.com <yb...@google.com> #4Mar 9, 2021 06:18AM

btw, a bug in your sample (in case it is present in your real app)

https://github.com/yhpark/androidx-paging3-issues/blob/master/app/src/main/java/com/example/myapplication/TestPagingSource.kt#L35

for prepend, you are returning the data in reverse. you are not expected to do that. the bug does not show up in your sample because you are constantly refreshing the list but w/o that, you would see the list loading in weird order if it ever started from bottom.

ap...@google.com <ap...@google.com> #5Mar 17, 2021 12:19AM

Project: platform/frameworks/support
Branch: androidx-main

commit 4eabdc68446c5268a9a6f1d80546703eba18d3b1
Author: Yigit Boyar <yboyar@google.com>
Date: Fri Mar 05 10:47:07 2021

Improve NullPaddedList diffing

This CL updates NullPaddedList diffing heuristic to better optimize for
common cases.

There two common cases we optimize for:
a) For completely distinct lists, we dispatch them as change animations
whenever possible based on positions (as long as there is no item in the
same global position in the old and new list)
b) For cases where lists partially overlap, we use available
placeholders to dispatch inserts or convert removals to placeholders
WHEN those updates happen at the boundaries of the list. This especially
handles cases where placeholders are loaded and prevents recyclerview
from scrolling to keep placeholders on screen.

See NullPaddedDiffing.md document for more details.

Bug: 170027529
Bug: 177338149
Test: NullPaddedListDiffWithRecyclerViewTest
Test: AsyncPagedListDifferTest
Test: NullPaddedListDiffHelperTest
Relnote: "We've rewamped how placeholders are handled when list is
reloaded to prevent unexpected jumps in RecyclerView. See
NullPaddedDiffing.md for details."

Change-Id: If1490f5bc833a61793d27eeaae9b37b26153df87

M paging/common/api/public_plus_experimental_3.0.0-beta03.txt
M paging/common/api/public_plus_experimental_current.txt
M paging/common/src/main/kotlin/androidx/paging/PagingDataDiffer.kt
M paging/runtime/build.gradle
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagedListDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/AsyncPagingDataDifferTest.kt
M paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffHelperTest.kt
A paging/runtime/src/androidTest/java/androidx/paging/NullPaddedListDiffWithRecyclerViewTest.kt
M paging/runtime/src/main/java/androidx/paging/AsyncPagedListDiffer.kt
A paging/runtime/src/main/java/androidx/paging/NullPaddedDiffing.md
M paging/runtime/src/main/java/androidx/paging/NullPaddedListDiffHelper.kt

https://android-review.googlesource.com/1621325

yb...@google.com <yb...@google.com> Mar 17, 2021 12:20AM

Marked as fixed.

Issue 170027529