ScrollableState.animateScrollBy does not preserve velocity when interrupting a previous call [220119990]

Bug

Status Update

No update yet.

Description

kl...@google.com

created issue #1

Feb 17, 2022 03:07PM

If ScrollableState.animateScrollBy is called and then before it completes it is called again in another coroutine, the first animation will be cancelled before starting the second one. However, unlike calls to Animatable.animateTo, the velocity of the previous animation will not be preserved.

Possible solutions

This isn't a trivial fix because ScrollableState itself does not keep any animation or velocity state – animateScrollBy is an extension function. It also does not provide any signal about whether it's being scrolled by an animation or the user, so if we track the velocity externally then we could end up in a weird state if the user interrupts an animation.

Option 1: Move `animateScrollBy` into `ScrollableState`

This would allow ScrollableState to keep track of animated scroll velocity internally. It should also allow it to be reset to zero when the animation is interrupted by a user scroll. However, ScrollableState is a public interface so to avoid breaking binary compatibility we would need to wait for Kotlin 1.6.20 and then give it a default implementation. The default implementation could simply be the current implementation.

Option 2: Expose scroll velocity from `ScrollableState`

I'm not sure how feasible this is – tracking velocity requires tracking timestamps, which ScrollableState does not currently do and would probably make the API super gross.

Comments

kl...@google.com <kl...@google.com> #2Jun 14, 2022 06:06PM

Accepted by kl...@google.com.

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

ap...@google.com <ap...@google.com> #3Jul 7, 2022 08:56PM

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

commit cb90e2addc50ff6f4e7f48df0dc0a03343898ccb
Author: Zach Klippenstein <klippenstein@google.com>
Date: Wed Jun 15 12:13:03 2022

Fix scrollable focused child tracking.

When calculating whether we need to scroll to keep the focused child
in-view after the scrollable viewport is resized, we were using the
bounds of the focused child to figure out if it was previously visible.
That was incorrect, because if the viewport is animating while the
scroll animation is running, it's possible for the viewport to shrink
fast enough that the focused child isn't actually visible, but would
have eventually became visible after the animation, so we need to
continue to animate.

This approach is documented as Option 0 in the design doc:

https://docs.google.com/document/d/16yYJOIcde0Pm7vB--ExSdrY7lrZQ2rqHIn6nYWb7ccg/edit#heading=h.ytkawqefedyn

Note that while this fixes

b/230756508

and will cause the animations to
end up in the correct final state, it makes

b/220119990

more visible,
since it looks like the scroll animation doesn't start at all until the
viewport animation stops.

Fixes:

b/230756508

Test: Manually using the 'Foundation > Scrollable with focused child"
and "Text > Input fields > Inside scrollable" demos."
Test: ./gradlew :compose:f:f:cDAT
Relnote: "When a scrollable has a focused child, it will now correctly
scroll to keep the focused child in view when its size is decreased,
even when the size is animated."
Change-Id: I8086717b14174e566b299f2643f1dd6c0b250773

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableFocusableInteractionTest.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/ScrollableFocusedChildDemo.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/Scrollable.kt

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

kl...@google.com <kl...@google.com> #4Aug 5, 2022 01:51AM

Assigned to le...@google.com.

This is no longer blocking me, assigning to Levi to figure out if it's something we want to support.

le...@google.com <le...@google.com> May 22, 2023 01:22PM

Status: New

Issue 220119990

Description

Possible solutions

Option 1: Move animateScrollBy into ScrollableState

Option 2: Expose scroll velocity from ScrollableState

Issue summary