Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested Scroll Connaction blocks scrolling after upgrade to Compose Multiplatform 1.6.0 #4395

Closed
Vaibhav2002 opened this issue Feb 29, 2024 · 13 comments
Labels
android bug Something isn't working ios scroll upstream (AOSP) Issue is in Google issue tracker wait for upstream

Comments

@Vaibhav2002
Copy link

Describe the bug
Nested Scroll connection seems to be dropping scroll events and stopping scroll in Compose Multiplatform 1.6.0
When downgrading, it works fine

Affected platforms
Select one of the platforms below:

  • All

If the bug is Android-only, report it in the Jetpack Compose tracker

Versions

  • Kotlin version*: 1.9.22
  • Compose Multiplatform version*: 1.6.0

Expected behavior
Before upgrading to Compose Multiplatform 1.6.0, my custom layout which uses a nested scroll connection worked pretty good

Screen_Recording_20240229_210718.mp4

Now after upgrading to Compose Multiplatform 1.6.0, without any change of code it broke and stops scroll when swiping

Screen_Recording_20240229_211010.mp4

Source Code:

@Composable
fun CollapsingLayout(
    modifier: Modifier = Modifier,
    collapsingTop: @Composable BoxScope.() -> Unit,
    bodyContent: @Composable BoxScope.() -> Unit,
) {
    var collapsingTopHeight by rememberSaveable { mutableStateOf(0f) }

    var offset by rememberSaveable { mutableStateOf(0f) }

    fun calculateOffset(delta: Float): Offset {
        val oldOffset = offset
        val newOffset = (oldOffset + delta).coerceIn(-collapsingTopHeight, 0f)
        offset = newOffset
        return Offset(0f, newOffset - oldOffset)
    }

    val nestedScrollConnection = remember {
        object : NestedScrollConnection {
            override fun onPreScroll(available: Offset, source: NestedScrollSource): Offset =
                when {
                    available.y >= 0 -> Offset.Zero
                    offset == -collapsingTopHeight -> Offset.Zero
                    else -> calculateOffset(available.y)
                }

            override fun onPostScroll(
                consumed: Offset,
                available: Offset,
                source: NestedScrollSource,
            ): Offset =
                when {
                    available.y <= 0 -> Offset.Zero
                    offset == 0f -> Offset.Zero
                    else -> calculateOffset(available.y)
                }
        }
    }

    Box(
        modifier = modifier
            .fillMaxSize()
            .background(MaterialTheme.colorScheme.background)
            .nestedScroll(nestedScrollConnection),
    ) {
        Box(
            modifier = Modifier
                .onSizeChanged { size ->
                    collapsingTopHeight = size.height.toFloat()
                }
                .offset { IntOffset(x = 0, y = offset.roundToInt()) },
            content = collapsingTop,
        )
        Box(
            modifier = Modifier.offset {
                IntOffset(
                    x = 0,
                    y = (collapsingTopHeight + offset).roundToInt()
                )
            },
            content = bodyContent,
        )
    }
}
@Vaibhav2002 Vaibhav2002 added bug Something isn't working submitted labels Feb 29, 2024
@MatkovIvan
Copy link
Member

Select one of the platforms below:
All

@Vaibhav2002 Android and Desktop is affected too? Are you sure that it's not iOS specific?

@Vaibhav2002
Copy link
Author

Select one of the platforms below:
All

@Vaibhav2002 Android and Desktop is affected too? Are you sure that it's not iOS specific?

I am facing this issue in iOS and Android. Dont know about desktop as my targets are ios and android only

@MatkovIvan
Copy link
Member

Thanks for clarification. We'll look into it, but at the same time, I'd recommend you to create this issue on Google's tracker too - if Android is affected, it means this related change was merged from AOSP repo to our fork. Also, Compose Multiplatform uses original Google's binaries on Android, so it requires Google's attention and fix too.

Please post the link to that issue here that we can be in sync. Thanks

@ASalavei
Copy link
Collaborator

ASalavei commented Feb 29, 2024

@Vaibhav2002 , Hi there! I can't reproduce it on 1.6.0. I used your layout like this:

    CollapsingLayout(
        collapsingTop = {
            Box(modifier = Modifier.background(Color.Red).fillMaxWidth().height(150.dp))
        },
        bodyContent = {
            Column(modifier = Modifier.verticalScroll(rememberScrollState())) {
                Box(modifier = Modifier.background(Color.Green).fillMaxWidth().height(500.dp))
                Box(modifier = Modifier.background(Color.Blue).fillMaxWidth().height(500.dp))
                Box(modifier = Modifier.background(Color.Gray).fillMaxWidth().height(500.dp))
                Box(modifier = Modifier.background(Color.White).fillMaxWidth().height(500.dp))
            }
        }
    )

You can see the result below (same on Android Emulator):
https://github.com/JetBrains/compose-multiplatform/assets/78535708/459c11b6-421f-4498-bdaa-df075e6f8130

What should be changed there to make it reproducible?

@Vaibhav2002
Copy link
Author

@ASalavei, I found out that this is happening only when we have a Horizontal Pager
This is the minimal reproducible sample,

@OptIn(ExperimentalFoundationApi::class)
@Composable
fun ProfileTab() {
    val tabs = 2
    val pagerState = rememberPagerState(0) { tabs }
    CollapsingLayout(
        collapsingTop = {
            Box(modifier = Modifier.fillMaxWidth().height(350.dp).background(Color.Green))
        }
    ) {
        HorizontalPager(
            state = pagerState,
            modifier = Modifier.fillMaxSize(),
        ) {
            LazyColumn(modifier = Modifier.fillMaxWidth()) {
                items(6) {
                    Box(
                        modifier = Modifier.fillMaxWidth().height(200.dp)
                            .background(MaterialTheme.colorScheme.primary)
                    )
                    Spacer(modifier = Modifier.height(16.dp))
                }
            }
        }
    }
}

@Vaibhav2002
Copy link
Author

Multiplatform uses original Google's binaries on Android, so it requires Google's attention and fix too.

Thanks for clarification. We'll look into it, but at the same time, I'd recommend you to create this issue on Google's tracker too - if Android is affected, it means this related change was merged from AOSP repo to our fork. Also, Compose Multiplatform uses original Google's binaries on Android, so it requires Google's attention and fix too.

Please post the link to that issue here that we can be in sync. Thanks

Sure, I will create an issue there too

@ASalavei
Copy link
Collaborator

ASalavei commented Mar 1, 2024

The source of problem is here (It is Compose change, so Android apps will definetely be affected as well): https://github.com/JetBrains/compose-multiplatform-core/blob/6b1cbe1bb844c7a0c0602031289b273431e58f28/compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/pager/Pager.kt#L899

I can propose the following change for CMP, however I'm not aware the way Google will fix it: JetBrains/compose-multiplatform-core#1154

@MatkovIvan
Copy link
Member

however I'm not aware the way Google will fix it

@ASalavei let's just upstream your fix

@Vaibhav2002
Copy link
Author

Thanks for clarification. We'll look into it, but at the same time, I'd recommend you to create this issue on Google's tracker too - if Android is affected, it means this related change was merged from AOSP repo to our fork. Also, Compose Multiplatform uses original Google's binaries on Android, so it requires Google's attention and fix too.

Please post the link to that issue here that we can be in sync. Thanks

Link to Google Issue Tracker
https://issuetracker.google.com/issues/327687953

@Vaibhav2002
Copy link
Author

@ASalavei Thanks for fixing this
@MatkovIvan by when can we expect this fix to be released, this is blocking the release of my app to production.

@MatkovIvan
Copy link
Member

@Vaibhav2002 We're going to release 1.6.1 in ~2w, this fix should be included. But our changes will fix only the iOS side - we're NOT rebuilding Android and use redirection to original Google's binaries.
I'm not sure about the Android release plans at the moment, I'll contact to Google colleges about that. I'll post an update here once I have more info

MatkovIvan pushed a commit to JetBrains/compose-multiplatform-core that referenced this issue Mar 4, 2024
Fix scroll interruption by Pager component when the scroll happens in
perpendicular direction

## Proposed Changes

Do not interrupt scrolling if it occurs in a perpendicular direction to
the Pager.

## Testing

Test: see description here:
JetBrains/compose-multiplatform#4395

## Issues Fixed

Fixes: The bug on
JetBrains/compose-multiplatform#4395
@MatkovIvan
Copy link
Member

@Vaibhav2002 the fix was too late for the next patch on Google side, so it will be released only in ~2w. On our side, we decided to sync with it and do not ship the fix for iOS separately. It means that it won't be included to 1.6.1, but only to 1.6.2

Closing, as it was fixed

@igordmn igordmn added the upstream (AOSP) Issue is in Google issue tracker label Mar 26, 2024
@okushnikov
Copy link

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android bug Something isn't working ios scroll upstream (AOSP) Issue is in Google issue tracker wait for upstream
Projects
None yet
Development

No branches or pull requests

6 participants