Race condition in Modifier.swipeable [167966118]

Fixed

Bug

Status Update

No update yet.

Description

ca...@google.com

created issue #1

Sep 7, 2020 03:19PM

Currently in Modifier.swipeable whenever the anchors change, we snap the animated float to the offset corresponding to the current value. This makes sure that the offset has the correct value at the start.

However, it might be preferrable to animate instead of snapping (except for the first composition). For example, consider this example in Backdrop.

Also if you do something like:

doSomethingThatChangesTheAnchors()
swipeableState.animateTo(newValue)

this will likely give you a race condition, because the first line will update the anchors and hence run holder.snapTo which will stop any ongoing animation.

I tried to fix this in aosp/1420961 by animating on every composition except the first (i.e. when the previous anchors were empty). Unfortunately that also had problems because it was cancelling the animation and then starting it again with an updated target, which means that onEnd was invoked at the wrong time.

Comments

ca...@google.com <ca...@google.com> #2Sep 11, 2020 02:00PM

I forgot to link this in the CL, but technically aosp/1420961 fixed the first part of the bug (animating when the anchors change).

The race condition is a separate issue however, so I will leave this open. I renamed the bug so you know what it's about.

jo...@google.com <jo...@google.com> #3Apr 8, 2022 05:57PM

Reassigned to jo...@google.com.

Using this as root issue for upcoming Swipeable work.

jo...@google.com <jo...@google.com> #4Aug 25, 2022 10:47AM

Phase 1 CL: https://android-review.googlesource.com/c/platform/frameworks/support/+/2175082

jo...@google.com <jo...@google.com> #5Nov 29, 2022 11:39AM

Status update: Work is ongoing!

We introduced positional and velocity threshold APIs to the overhauled component
Made a slight change in how we will expose the offset (aosp/2239574)
The upcoming release will contain a change that migrates M2's ModalDrawer to use the new APIs aosp/2269266

As well as a few other minor changes. We're currently working on migrating ModalBottomSheetLayout and will update this issue when the migration is done.

jo...@google.com <jo...@google.com> #6Dec 6, 2022 09:21PM

Marked as fixed.

Status update:

We migrated M2's ModalBottomSheetLayout
We are working on migrating other components like Switch
BottomSheetScaffold is slated for after the holidays
Added the previous target value as a param to AnchorChangeHandler

There is still more work - as always. For now, we are not adding a public NestedScrollConnection to swipeable. The implementation depends on the setup of the anchors, so unless we enforce universal rules this will be hard to implement for all components. However, we will be exposing the minOffset and maxOffset which you need to build your own nested scroll connection. We recommend taking a look at ModalBottomSheetLayout's implementation.

The overhauled APIs don't have an equivalent to ResistanceConfig yet, but work to support Overscroll with Swipeable is underway.

The plan is to make the overhauled APIs public in Foundation, but we don't have a timeline for this yet. As this issue serves as blocker for some internal issues, I will close this issue in the meantime and open issues for Overscroll support as well as making the API public in Foundation. These will be linked here in the next few days.

Issue 167966118