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

System WindowInsets Not Consumed on iOS Targets When using Nested Navigators #484

Open
mofeejegi opened this issue Sep 14, 2024 · 14 comments · May be fixed by #500
Open

System WindowInsets Not Consumed on iOS Targets When using Nested Navigators #484

mofeejegi opened this issue Sep 14, 2024 · 14 comments · May be fixed by #500

Comments

@mofeejegi
Copy link

mofeejegi commented Sep 14, 2024

This is a bit complex to explain, but to summarise, the WindowInsets aren't consumed when it is called from a Nested Navigator (navigator.level > 0) on an iOS device with ignoreSafeArea set to .all in ContentView.swift.

This error specifically happens from Voyager 1.1.0-alpha03 (including newer versions) alongside the Compose Multiplatform Plugin 1.7.0-beta02 version. Perhaps it is some compose multiplatform plugin compatibility problem.
Basically:
Compose MP 1.7.0-beta02 upwards + Voyager 1.1.0-alpha03 upwards = Issue occurs ❌
Compose MP 1.7.0-beta01 downwards + Voyager (any version) = Working as expected ✅
Compose MP (any version) + Voyager 1.1.0-alpha02 downwards = Working as expected ✅

To highlight the defect, you can check out this sample reproduction on Github.

Here's the code to explain the problem:

@Composable
@Preview
fun App() {
    MaterialTheme {
        Surface {
            Navigator(screen = FirstScreen())
        }
    }
}

class FirstScreen : Screen {
    @Composable
    override fun Content() {
        Box(modifier = Modifier.fillMaxSize().background(color = Color.Blue)) {
            Navigator(screen = SecondScreen())
        }
    }
}

class SecondScreen : Screen {
    @Composable
    override fun Content() {
        Box(modifier = Modifier.fillMaxSize().systemBarsPadding().background(color = Color.Green)) {}
    }
}

Correct Result

Wrong Result

NOTES:

  • If the insets systemBarsPadding() is called in the First Navigator, the insets padding is applied normally.
  • iOSApp ContentView.swift has its code like .ignoresSafeArea(.all) in order to draw from edge to edge.
  • I'm not sure if this a Voyager issue or Compose Multiplatform issue (or perhaps both).
@mofeejegi mofeejegi changed the title System WindowInsets Not Consumed on iOS Targets using Nested Navigators System WindowInsets Not Consumed on iOS Targets When using Nested Navigators Sep 14, 2024
@hristogochev
Copy link

Can confirm, this is a a huge issue for me and my team.

@mofeejegi
Copy link
Author

I did some more research and checked out #366 and #355 . It seems like this is some kind of compatibility issue between the latest Compose MP plugin and the Compose MP plugin referenced by Voyager. It also seems to be somewhat recurrent across versions ever since Voyager upgraded to Compose MP 1.6.0 in version 1.1.0-alpha03. Reverting my codebase to use Compose 1.7.0-beta01 isn't the best option right now because of the issues in that version, so I may have to wait for a resolution.

@hristogochev
Copy link

hristogochev commented Sep 15, 2024

I've tried forking voyager and updating its compose plugin to version 1.7.0-beta02 but unfortunately that doesn't seem to solve the issue.

@hristogochev
Copy link

I did some research and think the issue might be related to the new implementation of iOS platformLayers:
JetBrains/compose-multiplatform-core#1515

@mofeejegi
Copy link
Author

I saw that change too. If I recall correctly, there were changes to the way platform layers were used in Compose MP on iOS over the past few versions.

It would be quite coincidental if the changes to the use of Platform Layers in Compose MP on iOS somehow led to this issue with Voyager's Nested Navigators. Note that this issue has been happening in various combinations of Voyager and Compose MP. That said, I still can't find how this affects Voyager Nested Navigators, I was hoping to discover the link and attempt resolving it myself.

@hristogochev
Copy link

Although not ideal, I've come up with a workaround for this issue:
https://gist.github.com/hristogochev/41c7d2f12447e3e4854705d9f5b4e5dc

If you want navigation bars or status bars instead of system bars you can change the code accordingly.

@hristogochev
Copy link

Compose 1.7.0 just got released and the issue still persists. If I could understand how WindowInsets get passed between different navigators I could open a PR but looking at the code I can't find any relevant info.

@hristogochev
Copy link

hristogochev commented Oct 16, 2024

Update:

WindowInsets get passed until this line in Navigator.
The issue is fixed by adding LocalWindowInfo provides LocalWindowInfo.current immediately after it so it results in:

CompositionLocalProvider(
        LocalNavigatorStateHolder providesDefault rememberSaveableStateHolder(),
        LocalWindowInfo provides LocalWindowInfo.current
    ) {
//  Rest of the code
}

However, I haven't yet tested if this results in any unwanted side effects. Perhaps there is a better solution, as this one involves a dependency on compose.foundation.

@hristogochev
Copy link

hristogochev commented Oct 17, 2024

I've opened a PR that fixes this issue and does not involve a dependency on compose.foundation.

@mofeejegi
Copy link
Author

Amazing @hristogochev ! 🎉
I assumed that a fix will happen on the iosMain side, to think that a change on commonMain was the way to go.
Did it also run smoothly on Android platforms as well?

@hristogochev
Copy link

Yes, its essentially the same code but written differently.

For some reason calling providesDefault twice stops any composable updates that happen before the first call from reaching inside the second.
providesDefault should only provide a local if the local hasn't already been provided but it also leads to this weird side effect.
Replacing the default providing with a simple if check if the local has already been provided removes this side effect.

This is probably a bug on the Compose Multiplatform side, not in Voyager, but this should help until its resolved.

@hristogochev
Copy link

I've reported the issue to Jetbrains here, in this case the issue affects iOS nested navigator paddings because in 1.7.0-beta02 their iOS implementation depends on a wrapper LocalProvider.

@mofeejegi
Copy link
Author

I've been watching the issue you reported here @hristogochev
Thanks for the effort you put into this. I see that it's fixed now on Google's end and I'm monitoring the compose runtime releases waiting for it to be added.
It'll probably be included as part of 1.8.0-alpha06 or 1.8.0-beta01 and perhaps in a release even further in the future for Compose Multiplatform.
When this happens, then we can hopefully test and close this issue once and for all 🙏

@hristogochev
Copy link

hristogochev commented Nov 7, 2024

In the meantime you can clone my fork of voyager which only contains the pull request for this issue and a configuration for local deployment.

Simply clone it, switch to the local-deploy branch and run the task publishToMavenLocal.
Then in your project, include mavenLocal() as a source for your dependencies and set the version of voyager as unspecified.

And voila! You are able to use the newest version of Compose Multiplatform with the newest version of Voyager!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants