[Swipe to refresh] Indicator stuck on Android 12 stretch overscroll effect

[chippmann]

Description If you stop the pull motion briefly and then resume the motion, the indicator will stay at the same place and the motion propagates down to the LazyColumn to trigger the overscroll effect.

Note: this does not happen with the legacy overscroll effect (prior to android 12)! It only happens with the "new" stretch overscroll effect.

This only happens if the motion is interrupted and the finger stays on the screen without any movement and then resumes.

Video:

https://user-images.githubusercontent.com/22662033/156323093-906214aa-40d6-4963-aa83-fb9e432fcbe4.mp4

Steps to reproduce

@Composable
fun PagerIssue() {
    var isRefreshing by remember { mutableStateOf(false) }

    SwipeRefresh(
        modifier = Modifier.fillMaxSize(),
        state = rememberSwipeRefreshState(isRefreshing),
        onRefresh = { isRefreshing = true },
    ) {
        LazyColumn(
            Modifier.fillMaxSize()
        ) {
            items(100) { index ->
                Text("I'm item Nr. $index")
            }
        }
    }

    LaunchedEffect(isRefreshing) {
        if (isRefreshing) {
            delay(1000)
            isRefreshing = false
        }
    }
}

Expected behavior Upon resume of the motion, the indication should continue it's movement and continue the pull to refresh motion/action.

Additional context This problem can be worked around by wrapping the content of the SwipeRefresh composable with a CompositionLocalProvider which provides null as LocalOverScrollConfiguration while state.isSwipeInProgress is true:

@Composable
fun SwipeRefreshWithoutOverscroll(
    state: SwipeRefreshState,
    onRefresh: () -> Unit,
    modifier: Modifier = Modifier,
    swipeEnabled: Boolean = true,
    refreshTriggerDistance: Dp = 80.dp,
    indicatorAlignment: Alignment = Alignment.TopCenter,
    indicatorPadding: PaddingValues = PaddingValues(0.dp),
    indicator: @Composable (state: SwipeRefreshState, refreshTrigger: Dp) -> Unit = { s, trigger ->
        SwipeRefreshIndicator(s, trigger)
    },
    clipIndicatorToPadding: Boolean = true,
    content: @Composable () -> Unit,
) {
    SwipeRefresh(
        state = state,
        onRefresh = onRefresh,
        modifier = modifier,
        swipeEnabled = swipeEnabled,
        refreshTriggerDistance = refreshTriggerDistance,
        indicatorAlignment = indicatorAlignment,
        indicatorPadding = indicatorPadding,
        indicator = indicator,
        clipIndicatorToPadding = clipIndicatorToPadding,
    ) {
        val overscrollConfiguration = if (state.isSwipeInProgress) {
            null
        } else {
            LocalOverScrollConfiguration.current
        }
        CompositionLocalProvider(LocalOverScrollConfiguration provides overscrollConfiguration) {
            content()
        }
    }
}

Maybe this is also the solution to take as a PR? Personally i see no reason why the underlying scroll view should have any overscroll effect while isSwipeInProgress is true as the SwipeRefresh composable should handle all motion events.

[bentrengrove]

Thanks for the report. For the proposed solution, wouldn't you still need overscroll for when you hit the bottom of the list?

[bentrengrove]

Oh sorry I misread, it's only while a swipe is in progress. That does seem like a good fix at second glance. Feel free to put up a PR and I will review!

[chippmann]

@bentrengrove I'm working on the PR now but got one question; In your samples i have an issue which strangely enough i don't have in my code; isSwipeInProgress is set to true while regularily scrolling down a lazy column. Upon investigation i found that isSwipeInProgress is set to true inside the NestedScrollConnection's onPostScroll even if the remaining y is negative. Now my question is why is isSwipeInProgress set to true in the NestedScrollConnection's onPostScroll if the remaining y is negative? Shouldn't it be only true on a negative y if also the indicator's offset is greater than 0 indicating that a swipe has happend previously and this down motion basically cancels the swipe?

This basically leads to the issue with my suggested fix that the overscroll is also disabled when the user reaches the end of the list at the bottom.

[chippmann]

Simply wrapping the state.isSwipeInProgress = true inside

        if (available.y > 0) {
            state.isSwipeInProgress = true
        }

or moving it out of onScroll to only be executed in onPostScroll seems to fix the issue but I'm uncertain if this does not impose any other issues. With this fix it would certainly need to be revisited for https://github.com/google/accompanist/pull/541 to take the other direction into account.

[chippmann]

So basically the isSwipeInProgress is not correct with the current implementation if the user starts a pull to refresh, aborts it and starts scrolling down as isSwipeInProgress is only set to false once onPreFling triggers which only happens upon release. But as as i see it, it should also be set to false during the motion once the scrolling of the list begins when the indicator is gone again. This would take care of that:

        if (available.y > 0) {
            state.isSwipeInProgress = true
        } else if (state.indicatorOffset.roundToInt() == 0) {
            state.isSwipeInProgress = false
        }

But again with https://github.com/google/accompanist/pull/541 we would need to take the direction into account.

@bentrengrove should i do two separate PR's for this as these are basically two separate issues or should i do the fix with the PR for this issue? Or do i miss something with my proposed fix for the isSwipeInProgress state not being upToDate/correct?

[bentrengrove]

Your logic seems sounds to me. Do all the tests pass? I didn't write this code though so I can't say if this was done on purpose. Don't worry about the other PR for now, best to fix the bug first.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[demidenko]

Why it is closed? Issue is actual.

[demidenko]

Just tested new build with fix, issue is not fixed fully, sometimes circle still freezing allowing overscroll list animation :(

[phamsarah]

^^^

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[Ironova]

Any news about this issue? It's still happening for me.

[github-actions[bot]]

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[TomBell-Trove]

Related? https://issuetracker.google.com/issues/248274004