nathantannar4 / Transmission

Bridges UIKit presentation APIs to a SwiftUI API so you can use presentation controllers, interactive transitions and more.
BSD 2-Clause "Simplified" License
378 stars 13 forks source link

NavigationLink presented in top level controller with sheet presentation and custom navigation controller #20

Closed PhilipDukhov closed 4 weeks ago

PhilipDukhov commented 9 months ago

Steps to reproduce:

  1. Push
  2. Present
  3. Push

Expected result: view pushed inside the sheet Actual result: view pushed in the root controller

https://github.com/nathantannar4/Transmission/assets/6103621/6e5e81e8-f633-4227-9fb6-3dc52857a7e6

struct Test: View {
    @State var flag = false

    var body: some View {
        NavigationContainer {
            VStack {
                Button("present") {
                    withAnimation {
                        flag = true
                    }
                }
                NavigationLink("push") {
                    Test()
                }
            }
            .presentation(
                transition: .sheet(
                    options: .init(
                        detents: .init([.medium])
                    )
                ),
                isPresented: $flag
            ) {
                Test()
            }
        }
    }
}

struct NavigationContainer<Content: View>: UIViewControllerRepresentable {
    @ViewBuilder let content: () -> Content

    func makeUIViewController(context: Context) -> UINavigationController {
        UINavigationController(rootViewController: UIHostingController(rootView: content()))
    }

    func updateUIViewController(_ uiViewController: UINavigationController, context: Context) {
    }
}
nathantannar4 commented 9 months ago

I think the newer NavigationLink(_ value: Value) APIs are unsupported, those are only expected to work with NavigationStack since it requires a navigationDestination to be registered.

PhilipDukhov commented 9 months ago

I think the newer NavigationLink(_ value: Value) APIs are unsupported, those are only expected to work with NavigationStack since it requires a navigationDestination to be registered.

This is not a new NavigationLink version, this one uses string as button title. NavigationLink(isActive) (which is marked as deprecated) works the same here.

With the system sheet, the NavigationLink button is disabled, indicating that it's not available in this context - it seems to ignore the sheet navigation controller. We may need to implement our version of NavigationLink to make custom navigation work in this scenario.

nathantannar4 commented 9 months ago

Oh yes you're right. Hmm I could have sworn this used to work.

nathantannar4 commented 9 months ago

Ah ok so I'm not sure how to fix this use case given that NavigationLink is a SwiftUI type. But the example that demonstrates this issue is not a "real world" use case example. This is because the NavigationLink pushes another instance of Test which creates a 2nd NavigationContainer. So something about this doubly nested UINavigationController messes up SwiftUI's NavigationLink.

The code example below works as expected. Notice the NavigationLink does not push a new NavigationContainer, only the PresentationLink

struct Issue20: View {
    var body: some View {
        NavigationContainer {
            NavigationLink {
                PresentationLink(
                    transition: .sheet(detents: [.medium])
                ) {
                    NavigationContainer {
                        NavigationLink {
                            Color.red
                        } label: {
                            Text("push")
                        }
                    }
                } label: {
                    Text("present")
                }
            } label: {
                Text("push")
            }
        }
    }
}
PhilipDukhov commented 8 months ago

You're right, my original code was not real world - it was kind of a luck that it reproduced the issue =)

But the bug is not related to two nested navigation controllers, is't just about UIViewControllerRepresentable inside navigation controller - and that's what we have in real world.

struct ContentView: View {
    @State var isPresented = false

    var body: some View {
        NavigationContainer {
            TestController()
        }
    }
}

struct TestController: UIViewControllerRepresentable {
    func makeUIViewController(context: Context) -> UIViewController {
        UIHostingController(rootView: Test())
    }

    func updateUIViewController(_ uiViewController: UIViewController, context: Context) {

    }
}

struct Test: View {
    @State var flag = false

    var body: some View {
        VStack {
            Button("present") {
                withAnimation {
                    flag = true
                }
            }
            NavigationLink("push") {
                TestController()
            }
        }
        .presentation(
            transition: .sheet(
                options: .init(
                    detents: .init([.medium])
                )
            ),
            isPresented: $flag
        ) {
            NavigationContainer {
                TestController()
            }
        }
    }
}
nathantannar4 commented 8 months ago

NavigationLink seems pretty broken in a few situations. Partially why I created DestinationLink for direct control. In 1.0.4 I fixed the delegate issue you mentioned.

NavigationLink is very broken even for the standard .sheet modifier, but for transmission it breaks when a Container is added as the root view of the NavigationLink. SwiftUI must mess something up.

struct Issue20: View {
    var body: some View {
        NavigationContainer {
            VStack {
                NavigationLink("push (broken)") {
                    Container { // <-- This causes issues
                        SheetLink()
                    }
                }

                NavigationLink("push") {
                    SheetLink()
                }
            }
        }
    }

    struct SheetLink: View {
        @State var isPresented = false

        var body: some View {
            HStack {
                Button("present w/ default (broken)") {
                    withAnimation {
                        isPresented = true
                    }
                }
                .sheet(
                    isPresented: $isPresented
                ) {
                    destination
                }

                PresentationLink(
                    transition: .sheet(detents: [.medium])
                ) {
                    destination
                } label: {
                    Text("present w/ transmission (working)")
                }
            }
        }

        var destination: some View {
            NavigationContainer {
                NavigationLink("push") {
                    Text("Hello, World")
                }
            }
        }
    }

    struct NavigationContainer<Content: View>: UIViewControllerRepresentable {
        let content: Content

        init(@ViewBuilder content: () -> Content) {
            self.content = content()
        }

        func makeUIViewController(context: Context) -> UINavigationController {
            let uiViewController = UINavigationController(rootViewController: UIHostingController(rootView: content))
            return uiViewController
        }

        func updateUIViewController(_ uiViewController: UINavigationController, context: Context) { }
    }

    struct Container<Content: View>: UIViewControllerRepresentable {
        let content: Content

        init(@ViewBuilder content: () -> Content) {
            self.content = content()
        }

        func makeUIViewController(context: Context) -> UIHostingController<Content> {
            let uiViewController = UIHostingController(rootView: content)
            return uiViewController
        }

        func updateUIViewController(_ uiViewController: UIHostingController<Content>, context: Context) { }
    }
}

What is the purpose of your TestController? I did find that if I swap it for a UIViewRepresentable then the issue does not reproduce.

struct Container<Content: View>: UIViewRepresentable {
        let content: Content

        init(@ViewBuilder content: () -> Content) {
            self.content = content()
        }

        func makeUIView(context: Context) -> _UIHostingView<Content> {
            let uiView = _UIHostingView(rootView: content)
            return uiView
        }

        func updateUIView(_ uiView: _UIHostingView<Content>, context: Context) { }
    }
PhilipDukhov commented 8 months ago

In real life TestController is chat conversation controller that contains collection view and a bunch of UIKit stuff around it, not sure if it's going to be easy to convert it to UIViewRepresentable.

The updated DestinationLink seems to work fine, thanks! One last thing with it is that it overrides the background color set by NavigationContainer - I added an environment variable which I use to determine which value to pass to transition: .default(options: .init(preferredPresentationBackgroundColor:, it's fine, not sure if lib needs a change at this point.