`Modifier.draggable` is reporting wrong fling velocity if the component is moving [216582726]

Fixed

Bug

Status Update

No update yet.

Description

ma...@gmail.com

created issue #1

Jan 27, 2022 12:27PM

Issue

TLDR; watch the attached video.

Was trying to make a free flowing bottom sheet like view, that allows for the sheet content to also be scrollable.

Was able to create this using .draggable() and a nested scroll connection. Code

Drags are working fine. But flings aren't, when the sheet is still moving. Because the fling velocity being reported in NestedScrollConnection.onPreFling() is wrong. Sometimes even in the opposite direction, if not the wrong magnitude.

Root Cause

I suspect this is because the velocity calculation is based on touch position relative to the component handling the pointer input.

Documentation of PointerInputChange states

position - The Offset of the current pointer event, relative to the containing element

Modifier.draggable which is also used in Modifier.scrollable effectively uses VelocityTracker.addPointerInputChange()

fun VelocityTracker.addPointerInputChange(event: PointerInputChange) {
    @OptIn(ExperimentalComposeUiApi::class)
    event.historical.fastForEach {
        addPosition(it.uptimeMillis, it.position)
    }
    addPosition(event.uptimeMillis, event.position)
}

Source: cs.android.com/VelocityTracker.kt

Potential Fix

If PointerInputChange also had absolutePosition which is relative to the window/screen, and the above function is modified to use absolutePosition instead of position(relative), then we will see the right velocity being calculated when drag has stopped

val event = if (isDragSuccessful) {
  val velocity =
    velocityTracker.calculateVelocity().toFloat(orientation)
  DragStopped(velocity * if (reverseDirection) -1 else 1)
}

Source: cs.android.com/Draggable.kt

I am not sure if we will need relative velocity of the user's fling to the component anywhere. It should be safe to move the implementation VelocityTracker.addPointerInputChange() to use absolutePosition always.

Other components that are affected (currently working around it)

Because of this issue, even Swipeable.kt doesn't use fling velocity to infer the direction of swipe, instead uses the diff of current offset and last offset - Swipeable.kt

Attachments

Code for reproducing the issue: okmanideep/flingvelocityissue
PFA video explaining the issue

draggable-fling-velocity-issue-demo.mp4

24 MB

Download

Comments

ma...@google.com <ma...@google.com> #2Jan 31, 2022 12:19PM

Reassigned to mo...@google.com.

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

commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019

Change the way to detect mismatch between POJO and query

This fixes cursor mismatch warnings with expandProjection.

Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8

M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt

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

https://goto.google.com/android-sha1/b90079595f33f58fece04026a97faa0d243acdb1

mo...@google.com <mo...@google.com> #3Jan 31, 2022 07:34PM

Reassigned to ma...@google.com.

The following changes were cherrypicked through Coastguard (

https://android-build.googleplex.com/coastguard/)
Release Track: (

https://android-build.googleplex.com/coastguard/dashboard/6093116828680192/#/cherrypickHistory/all)
Created: 2019-10-14 17:23:42.610596+00:00
Changes: aosp/1123258

mo...@google.com <mo...@google.com> #4Jan 31, 2022 07:38PM

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

commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020

Re-factor how errors are dismissed when query is re-written

This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (

b/140759491

).
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.

This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.

Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f

M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt

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

Message last modified on Jan 31, 2022 07:38PM

ma...@google.com <ma...@google.com> #5Feb 1, 2022 09:47AM

Reassigned to le...@google.com.

Oh, I see, that makes sense. Basically on every PointerInputChange I have to "adjust the past" if needed by accounting for a previousPosition change in the current change. That should work.

Over to Levi to look proper implementation fix.

st...@google.com <st...@google.com> #6May 5, 2022 03:12PM

Hi! any updates regarding this issue? Thanks!

le...@google.com <le...@google.com> #7May 5, 2022 03:16PM

Hi Alejandra, no updates yet sadly. This is a bit more complicated than expected, I believe it is related to other issues with the VelocityTracker https://buganizer.corp.google.com/hotlists/4179493. My plan is to prioritise this in the coming months so stay tuned and I'll report back. Thanks!

ap...@google.com <ap...@google.com> #8May 19, 2022 03:44PM

Marked as fixed.

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

commit 5b42a22514d0847dbce104a9f12216afa555b2bd
Author: Levi Albuquerque <levima@google.com>
Date: Fri May 13 13:06:45 2022

Improvements on how to calculate velocity based on pointer position

In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.

Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803

Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"

Change-Id: I51ec384a0424829b680b4666c7d3ce49227f45de

M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt

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

le...@google.com <le...@google.com> May 29, 2022 10:42AM

Status: Assigned (reopened)

ap...@google.com <ap...@google.com> #9Jun 8, 2022 09:03AM

Marked as fixed.

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

commit a66e75c2cb06d1d754055093e47cbdb6162d7aeb
Author: Levi Albuquerque <levima@google.com>
Date: Fri May 27 10:59:21 2022

Improvements on how to calculate velocity based on pointer position

In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.

Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803

Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"

Change-Id: If9ef3b9069e8f15b9b049bc3cd8b08bfd91edd49

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt

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

do...@gmail.com <do...@gmail.com> #10Jun 28, 2022 04:17PM

Please set this issue status as not fixed.

Both changes were reverted. https://android-review.googlesource.com/c/platform/frameworks/support/+/2104846 https://android-review.googlesource.com/c/platform/frameworks/support/+/2119246

le...@google.com <le...@google.com> Jun 28, 2022 04:18PM

Status: Assigned (reopened)

ap...@google.com <ap...@google.com> #11Jun 28, 2022 07:29PM

Marked as fixed.

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

commit 4e1e7aa76fe04d05362fa5cc319bd490401dfa61
Author: Levi Albuquerque <levima@google.com>
Date: Thu Jun 09 13:47:56 2022

Improvements on how to calculate velocity based on pointer position

In this CL I updated the way we add data from an InputEventChange into
the velocity tracker. Now we add points deltas instead of event positions.
This will guarantee that points are consistent even if the target element
is moving, which was not considering when adding the input event position.

Test: Added tests to check the behaviour in scrolling.
Fixes: 216582726
Bug: 223440806
Bug: 227709803

Relnote: "When adding InputEventChange events to Velocity Tracker we will
consider now deltas instead of positions, this will guarantee the velocity
is correctly calculated for all cases even if the target element moves"

Change-Id: Icea9d76a43643a6b17da11f3c539d27cb8fa6f6e

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/lazy/list/LazyListTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ScrollableTest.kt
M compose/ui/ui/src/commonMain/kotlin/androidx/compose/ui/input/pointer/util/VelocityTracker.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/lazy/list/BaseLazyListTestWithOrientation.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/input/nestedscroll/NestedScrollModifierTest.kt

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

to...@undo.app <to...@undo.app> #12Jul 15, 2022 09:13AM

The changes to VelocityTracker.kt are not to be found in the release of androidx.compose.ui:ui:1.3.0-alpha01.
They were reverted 5 weeks ago.

The fling behaviour is still not working properly in this scenario:
ViewParent (MotionLayout) holding ComposeView (LazyColumn)

The release-notes details in 1.3.0-alpha01 (

https://developer.android.com/jetpack/androidx/releases/compose-ui#1.3.0-alpha01) specifies this:
"When adding InputEventChange events to Velocity Tracker we will consider now deltas instead of positions,
this will guarantee the velocity is correctly calculated for all cases even if the target element moves (If9ef3,

b/216582726

b/223440806

b/227709803

)"

Can you provide an example of how this achieved in the scenario previously described?

Regards,
Toby.

Message last modified on Jul 15, 2022 09:14AM

le...@google.com <le...@google.com> #13Jul 15, 2022 09:21AM

Hi Toby, thanks for reaching out about this. It seems there has been an issue with the release notes and they were not removed for the previous version. Since this change was merged on June 26th, it didn't make the cut for 1.3.0-alpha01. I'd expect it to come in the next release though, so stay tuned :)

to...@undo.app <to...@undo.app> #14Jul 15, 2022 09:30AM

Alright, thanks for the quick response. :)

Do you have an ETA for the next release?

le...@google.com <le...@google.com> #15Aug 17, 2022 01:37PM

Hi Toby, the changes are available on 1.3.0-alpha03, let me know if this fixes your issue.

to...@undo.app <to...@undo.app> #16Aug 24, 2022 02:54PM

Hi again.

I am still not getting the expected behaviour.
When I do a fling on the list (LazyColumn), the parent view (MotionLayout) does not respond properly to this.

The Motionlayout only responds correctly when I am dragging the list.
When I try to fling the list it does not respond correctly. It either stops transitioning between the MotionLayout states or the transition between the two states is not done fluently.

I've been able to set up the same scenario, that has worked perfectly, with a RecyclerView instead of a ComposeView holding LazyColumn.

I've set up the LazyColumn with rememberNestedScrollInteropConnection().
It is set up like this.

binding.list.setContent {
LazyColumn(
state = listState,
modifier = Modifier
.fillMaxWidth()
.nestedScroll(rememberNestedScrollInteropConnection())

) {
items(listItems) {
......
}
}
}

The list is being set up in a MotionLayout (XML) via a ComposeView

<androidx.compose.ui.platform.ComposeView
android:id="@+id/list"
android:layout_width="0dp"
android:layout_height="0dp"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="@id/listTopGuideline"/>

The scene for MotionLayout has two constraint sets: "Start" and "End" where it should transition the listTopGuideline
app:layout_constraintGuide_percent from 0.59 to 0.1

<OnSwipe
app:dragDirection="dragUp"
app:touchAnchorId="@id/list" />

......
// Start ConstraintSet
<Constraint
android:id="@+id/listTopGuideline"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:orientation="horizontal"
app:layout_constraintGuide_percent="0.59" />

.....
// End ConstraintSet
<Constraint
android:id="@+id/listTopGuideline"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:orientation="horizontal"
app:layout_constraintGuide_percent="0.1" />

le...@google.com <le...@google.com> #17Aug 24, 2022 03:53PM

Hi Tobias, thanks for reporting this. This seems like an interop issue between Compose and MotionLayout, could you open a different ticket for this? This ticket was related to velocity tracker in Modifier.draggable and I'm not sure the two are connected. Thanks!

Issue 216582726

Description

Issue

Root Cause

Potential Fix

Other components that are affected (currently working around it)

Attachments

Issue summary

Comments

ma...@google.com <ma...@google.com> #2Jan 31, 2022 12:19PM

mo...@google.com <mo...@google.com> #3Jan 31, 2022 07:34PM

mo...@google.com <mo...@google.com> #4Jan 31, 2022 07:38PM

ma...@google.com <ma...@google.com> #5Feb 1, 2022 09:47AM

st...@google.com <st...@google.com> #6May 5, 2022 03:12PM

le...@google.com <le...@google.com> #7May 5, 2022 03:16PM

ap...@google.com <ap...@google.com> #8May 19, 2022 03:44PM

le...@google.com <le...@google.com> May 29, 2022 10:42AM

ap...@google.com <ap...@google.com> #9Jun 8, 2022 09:03AM

do...@gmail.com <do...@gmail.com> #10Jun 28, 2022 04:17PM

le...@google.com <le...@google.com> Jun 28, 2022 04:18PM

ap...@google.com <ap...@google.com> #11Jun 28, 2022 07:29PM

to...@undo.app <to...@undo.app> #12Jul 15, 2022 09:13AM

le...@google.com <le...@google.com> #13Jul 15, 2022 09:21AM

to...@undo.app <to...@undo.app> #14Jul 15, 2022 09:30AM

le...@google.com <le...@google.com> #15Aug 17, 2022 01:37PM

to...@undo.app <to...@undo.app> #16Aug 24, 2022 02:54PM

le...@google.com <le...@google.com> #17Aug 24, 2022 03:53PM

Add comment

Issue metadata