Ripple gets "stuck" after a long press [186223077]

Fixed

Bug

Status Update

No update yet.

Description

[Deleted User]

created issue #1

Apr 23, 2021 08:56PM

Jetpack Compose release version: 1.0.0-beta05

I'm using a modifier like this:

Modifier.combinedClickable(interactionSource, indication, onClick = {}, onLongClick = {
    ...
})

When the long click listener is called I show a popup dialog which causes the ripple to be stuck in the pressed state until I touch the item again.

Comments

so...@google.com <so...@google.com> Apr 26, 2021 11:08AM

Assigned to lp...@google.com.

lp...@google.com <lp...@google.com> #2Apr 29, 2021 03:56PM

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

[Deleted User] <[Deleted User]> #3May 3, 2021 05:48PM

Here is the issue reproduced:

@Preview
@Composable
fun StuckRipple() {
    var showPopup by remember { mutableStateOf(false) }
    val pressInteractions = remember { mutableStateListOf<PressInteraction.Press>() }
    var popupOffset by remember { mutableStateOf(IntOffset(0, 0)) }
    val haptics = LocalHapticFeedback.current
    val interactionSource = remember { MutableInteractionSource() }
    val indication = rememberRipple()

    LaunchedEffect(interactionSource) {
        interactionSource.interactions.collect { interaction ->
            when (interaction) {
                is PressInteraction.Press -> pressInteractions.add(interaction)
                is PressInteraction.Release -> pressInteractions.remove(interaction.press)
                is PressInteraction.Cancel -> pressInteractions.remove(interaction.press)
            }
        }
    }
    val pressedPosition by derivedStateOf { pressInteractions.firstOrNull()?.pressPosition }

    ListItem(
        text = { Text("Click me") },
        modifier = Modifier.combinedClickable(interactionSource, indication, onClick = {}, onLongClick = {
            haptics.performHapticFeedback(HapticFeedbackType.LongPress)
            popupOffset = pressedPosition?.round() ?: popupOffset
            showPopup = true
        })
    )
    if (showPopup) {
        Popup(
            properties = PopupProperties(focusable = true),
            offset = popupOffset,
            onDismissRequest = {
                showPopup = false
            }
        ) {
            Card(Modifier.padding(16.dp)) {
                Text("Hello")
            }
        }
    }
}

I my example I've also placed this inside a scaffold and a vertically scrollable container.

lp...@google.com <lp...@google.com> May 3, 2021 09:27PM

Reassigned to ma...@google.com.

lp...@google.com <lp...@google.com> #4May 3, 2021 10:39PM

Thanks for the reproduction - I can reproduce too.

I think the problem is that we wait for an up signal to emit a PressInteraction.Release / PressInteraction.Cancel event, and hence cancel the ripple (tryAwaitRelease). To do this we wait for an up event:

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt;l=132

And because we create a Popup during long press, something changes in dispatch - events seem to be coming through, so I think there is something strange happening with awaitPointerEvent - from some brief logging it looks like we never resume after:

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/gestures/TapGestureDetector.kt;l=316

So maybe we aren't correctly dispatching the events after the popup has launched, in the correct pass? Matvei, can you take a look?

Note: since we migrated to use RippleDrawable I noticed a small issue that means this doesn't reproduce cleanly, since when window focus changes the RippleDrawable has its ripple cleared. You can reproduce on ToT by doing:

CompositionLocalProvider(LocalRippleNativeRendering provides false) {
}

around the ripple - but the actual root bug here is we are not emitting the PressInteraction release / cancel events.

ma...@google.com <ma...@google.com> #5May 5, 2021 12:46PM

Reassigned to mo...@google.com.

Thanks for a lot of useful info Louis. Assigning to George to take a look at the popup overlap situation, since I believe we faced the similar ones in the past. Also cc-ing Andrey how had dealt with it as well.

am...@google.com <am...@google.com> #6May 14, 2021 12:49PM

Interactive Preview is also affected by that, and the ripple gets stuck sometimes when users interact with some buttons. See issue 188112048.

lp...@google.com <lp...@google.com> #7May 14, 2021 12:56PM

Re: comment #6

That bug appears to be a separate issue with Interactive Preview - this bug only occurs when a long press opens a new window.

mo...@google.com <mo...@google.com> #8May 14, 2021 04:03PM

I'm sorry, I haven't been able to get to this bug with all the other things on my plate. If someone else wants to take it, please do.

lp...@google.com <lp...@google.com> #9May 20, 2021 09:50PM

Reassigned to lp...@google.com.

Ok, finally managed to find the root problem - we don't use rememberUpdatedState() to remember the long click / double click lambdas, so if a long click triggers a recomposition, and those lambdas aren't explicitly memoized / have unstable types that cannot be memoized automatically, new lambda instances will be passed to combinedClickable during recomposition. This means that the keys provided to Modifier.pointerInput change, so we cancel the old coroutine, and we never finish waiting for the up event, and awaitPointerEvent never returns because things are cancelled / no longer being dispatched here.

ap...@google.com <ap...@google.com> #10May 21, 2021 03:52PM

Marked as fixed.

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

commit 9653864b3c491ed7371ef841bdd292160cbf244e
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Fri May 21 00:55:12 2021

Fixes ripples / indication sometimes getting stuck on a long click

If a long click causes a recomposition and causes the instance of the long click lambda to change, this would cause us to cancel and restart the pointer input scope in Modifier.pointerInput(). This means that we would miss the corresponding up event for that long click, causing no PressInteraction.Release to be emitted, and hence causing ripples / other indication to get 'stuck'.

Now we only cancel and restart this scope if we change from having a long click lambda to not having one (null), and in which case we will now correctly emit PressInteraction.Cancel to clean up any existing long clicks if needed.

Fixes:

b/186223077

Test: ClickableTest
Relnote: "Fixed an issue where ripples / other indication would sometimes get stuck on a long click when using Modifier.combinedClickable"
Change-Id: I2298ce564e3940875c3f3525255424da25dc9414

M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/ClickableTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/Clickable.kt

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