[ThreePaneScaffold] Predictive back doesn't handle cancelation [369899645]

Fixed

Bug

Status Update

No update yet.

Description

ia...@google.com

created issue #1

Sep 26, 2024 09:57PM

1.1 snapshot: 12421028

If you enable predictive back and use a NavigableListDetailPaneScaffold, dragging the gesture to start it and then moving back to the original position to release and cancel it results in a weird limbo state. The modifiers from the list pane will be used but the content won't, so it doesn't seem to reset to details correctly.

Sample code:

@OptIn(ExperimentalMaterial3AdaptiveApi::class)
@Composable
fun ExampleNldps(modifier: Modifier = Modifier) {
    val scaffoldNavigator = rememberListDetailPaneScaffoldNavigator()
    val detailBgColor = remember { mutableStateOf(Color.LightGreen) }
    val scope = rememberCoroutineScope()

    NavigableListDetailPaneScaffold(
        navigator = scaffoldNavigator,
        modifier = modifier,
        listPane = {
            AnimatedPane(
                Modifier
                    .background(Color.LightRed)
                    .safeContentPadding()) {
                Column {
                    Text("This is the list pane", fontSize = 24.sp)
                    Spacer(Modifier.size(24.dp))

                    if (scaffoldNavigator.scaffoldValue[ListDetailPaneScaffoldRole.Detail] == PaneAdaptedValue.Hidden) {
                        Button(onClick = {
                            scope.launch {
                                scaffoldNavigator.navigateTo(ListDetailPaneScaffoldRole.Detail)
                            }
                        }) {
                            Text("Show detail pane")
                        }
                    }
                }
            }
        },
        detailPane = {
            AnimatedPane(
                Modifier
                    .background(detailBgColor.value)
                    .safeContentPadding()
            ) {
                Column {
                    Text("This is the detail pane", fontSize = 24.sp)
                    Spacer(modifier = Modifier.size(24.dp))
                    Text("Anchors at 0%, 50%, and 100%")
                }
            }
        },
        extraPane = {
            AnimatedPane {
                Text(
                    modifier = Modifier
                        .background(Color.LightBlue)
                        .safeContentPadding(),
                    text = "Extra"
                )
            }
        },
        paneExpansionState = rememberPaneExpansionState(
            keyProvider = scaffoldNavigator.scaffoldValue,
            anchors = listOf(
                PaneExpansionAnchor.Proportion(0.0f),
                PaneExpansionAnchor.Proportion(0.5f),
                PaneExpansionAnchor.Proportion(1.0f),
            )
        ),
        paneExpansionDragHandle = { state ->
            PaneExpansionDragHandle(state = state, color = MaterialTheme.colorScheme.outline)
        }
    )
}

val Color.Companion.LightRed: Color
    get() = Color(red = 1f, green = 0.8f, blue = 0.8f, alpha = 1f)
val Color.Companion.LightGreen: Color
    get() = Color(red = 0.8f, green = 1f, blue = 0.8f, alpha = 1f)
val Color.Companion.LightBlue: Color
    get() = Color(red = 0.8f, green = 0.8f, blue = 1f, alpha = 1f)

predictive back.mp4

2.3 MB

Download

Comments

ia...@google.com <ia...@google.com> #2Oct 4, 2024 12:29AM

Exito

tr...@google.com <tr...@google.com> #3Oct 7, 2024 08:40PM

I know Louis has been looking at this already.

tr...@google.com <tr...@google.com> #4Oct 7, 2024 09:12PM

I think, clickable/combinedClickable must be replaced with separated modifiers (as focus.../onFocus...). I propose to make:

new pressable(enabled, interactionSource) modifier which does not have an indication, hoverable and focusable/focusableInNonTouchMode. interactionSource will only be used with PressInteraction.
new modifier(s) for onClickLabel/onClick, onLongClickLabel/onLongClick and onDoubleClick.
focusableInNonTouchMode public.

We can add role separately via semantics.

All this will help to create components with the necessary properties.

ma...@google.com <ma...@google.com> #5Oct 22, 2024 03:04PM

R.e comment #4, do you have a specific use case for this? When would you want a component to be pressable, but not focusable / hoverable?

ma...@google.com <ma...@google.com> Oct 22, 2024 05:50PM

Accepted by ma...@google.com.

ma...@google.com <ma...@google.com> Nov 13, 2024 04:00PM

Marked as fixed.

na...@google.com <na...@google.com> #6Nov 13, 2024 06:19PM

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

commit c3c96aef1d6c366ee1658cd8dfa27588231740c2
Author: Louis Pullen-Freilich <lpf@google.com>
Date: Thu Sep 28 16:59:45 2023

Deprecates rememberRipple and RippleTheme, adds createRippleNode API

This CL adds a new ripple implementation (createRippleNode) built using Modifier.Node APIs, so it can be used inside IndicationNodeFactory.
This leads to large performance improvements and greatly reduced allocations.

IndicationNodeFactory does not support calling Composable functions, so RippleTheme cannot be used to theme ripples built using the new node
APIs. In addition, having one singular library as the source of truth for ripple theme leads to tension and problems between design systems:
for example the default RippleTheme functions are actually the material spec, not material3, so they should have been versioned with the
material library instead. Similarly, if you are wrapping material / material3 with your own theme, both themes will end up 'fighting' over the
RippleTheme - the theme configuration is a property of the design system, not a generic system.

As a result, this CL deprecates rememberRipple and RippleTheme - the intention moving forward is for design system libraries to provide their
own theming APIs for ripples (if desired), and their own ripple Indication implementations that consume their own theme values. This allows us
to simplify the layering and API shape, and allows for more flexibility between custom design systems. createRippleNode encapsulates all of
the ripple logic so design system implementations can just delegate to createRippleNode after querying their theme values in a separate node.

Bug:

b/298048146

Test: RememberRippleTest, RippleModifierNodeTest, RippleBenchmark
Relnote: "Deprecates rememberRipple, RippleTheme and LocalRippleTheme, adds createRippleNode. Instead of using these APIs, use the new ripple Indication and theming APIs in design system libraries such as material / material3. See

developer.android.com for migration / backwards compatibility information and more background information behind this change."
Change-Id: I23f260b2f070e3f0f5b145b87eb1c6a06f3fd861

M compose/material/material-ripple/api/current.txt
M compose/material/material-ripple/api/restricted_current.txt
M compose/material/material-ripple/benchmark/src/androidTest/java/androidx/compose/material/ripple/benchmark/RippleBenchmark.kt
M compose/material/material-ripple/build.gradle
A compose/material/material-ripple/src/androidInstrumentedTest/kotlin/androidx/compose/material/ripple/RememberRippleTest.kt
M compose/material/material-ripple/src/androidInstrumentedTest/kotlin/androidx/compose/material/ripple/RippleContainerTest.kt
A compose/material/material-ripple/src/androidInstrumentedTest/kotlin/androidx/compose/material/ripple/RippleModifierNodeTest.kt
M compose/material/material-ripple/src/androidMain/kotlin/androidx/compose/material/ripple/Ripple.android.kt
M compose/material/material-ripple/src/androidMain/kotlin/androidx/compose/material/ripple/RippleContainer.android.kt
M compose/material/material-ripple/src/commonMain/kotlin/androidx/compose/material/ripple/CommonRipple.kt
M compose/material/material-ripple/src/commonMain/kotlin/androidx/compose/material/ripple/Ripple.kt
M compose/material/material-ripple/src/commonMain/kotlin/androidx/compose/material/ripple/RippleAnimation.kt
M compose/material/material-ripple/src/commonMain/kotlin/androidx/compose/material/ripple/RippleTheme.kt
M compose/material/material-ripple/src/desktopMain/kotlin/androidx/compose/material/ripple/Ripple.desktop.kt

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