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

Desynchronization of NFlow stack [iOS 15] #3

Closed
chrisballinger opened this issue Oct 5, 2021 · 21 comments
Closed

Desynchronization of NFlow stack [iOS 15] #3

chrisballinger opened this issue Oct 5, 2021 · 21 comments

Comments

@chrisballinger
Copy link

chrisballinger commented Oct 5, 2021

I've noticed that occasionally that the NStack stack array gets desynchronized from the application's actual navigation state. This manifests in behavior like: requiring two taps to navigate, navigating to the wrong screen, or double-navigating to a screen. Have you noticed this at all?

It's not consistently reproducible, but it seems to happen most frequently when navigating quickly back and forth between screens. Trying to dig in a bit deeper to figure out if there's any workarounds.

I've mostly been testing using Xcode 13.0 and the iOS 15.0 simulator. I haven't been able to reproduce so far in Xcode 12.5.1 or Xcode 13.0 when using the iOS 14.5 simulator, which is puzzling. Might be a SwiftUI NavigationLink state bug on iOS 15.

@chrisballinger chrisballinger changed the title Desynchronization of NFlow stack Desynchronization of NFlow stack [iOS 15] Oct 5, 2021
@johnpatrickmorgan
Copy link
Owner

Hi Chris,

I've found that to happen when the stack is increased by more than one screen in quick succession, e.g.:

stack.append(screen1)
stack.append(screen2)

In that case, SwiftUI only shows screen1.

I've also found that it can be problematic to push a new screen while a previous push animation is still in-flight, e.g.:

stack.append(screen1)
DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) {
  stack.append(screen2)
}

In that case, SwiftUI momentarily shows screen2 before popping back to screen1.

Both cases seem to be a limitation of SwiftUI as far as I can tell, as I'm able to reproduce them even without using FlowStacks, e.g.:

import SwiftUI

struct CannotPushMoreThanOneView: View {
  
  class ViewModel: ObservableObject {
    
    @Published var isPushing2 = false
    @Published var isPushing3 = false
    @Published var isPushing4 = false
  }
  
  @ObservedObject var vm = ViewModel()
  
  var body: some View {
    print(vm.isPushing2, vm.isPushing3, vm.isPushing4)
    return NavigationView {
      VStack {
        Button("Push 2 and 3", action: {
          vm.isPushing2 = true
          // SwiftUI does not push 3.
          vm.isPushing3 = true
        })
        Button("Push 2 and then 3", action: {
          vm.isPushing2 = true
          // If delay is less than about 0.45, SwiftUI pushes 2 then 3, then pops back to 2.
          DispatchQueue.main.asyncAfter(deadline: .now() + 0.2) {
            vm.isPushing3 = true
          }
        })
        NavigationLink(
          "Push",
          destination: NavigationLink(
            "Push",
            destination: NavigationLink(
              "Push",
              destination: Text("4"),
              isActive: $vm.isPushing4
            ).navigationTitle("3"),
            isActive: $vm.isPushing3
          ).navigationTitle("2"),
          isActive: $vm.isPushing2
        ).navigationTitle("1")
      }
    }.navigationViewStyle(StackNavigationViewStyle())
  }
}

Do you think these issues are what you've experienced?

@chrisballinger
Copy link
Author

Not quite, we aren't pushing multiple views in quick succession, so far it's just pushing a single screen at a time in response to a user tapping a Button. I tried to make a reproducible example roughly demonstrating the issue, but of course it works just fine:

enum Screen: Equatable, CaseIterable {
    case screen1
    case screen2
    case screen3
    case screen4
    case screen5

    var title: String {
        let index = Self.allCases.firstIndex(of: self) ?? 0
        return "Screen \(index + 1)"
    }
}

final class NavigationCoordinator: ObservableObject {
    @Published var flow = NFlow<Screen>(root: .screen1)

    func pushNext() {
        switch flow.array.last {
        case .screen1:
            flow.push(.screen2)
        case .screen3:
            flow.push(.screen5)
        case .screen4:
            flow.push(.screen5)
        case .screen2, .screen5, .none:
            fatalError("Not supported")
        }
    }
}

struct ContentView: View {
    @StateObject var navigator: NavigationCoordinator = .init()

    var body: some View {
        NavigationView {
            NStack($navigator.flow) { screen in
                DetailScreen(screen: screen)
            }
        }
        .environmentObject(navigator)
    }
}

struct DetailScreen: View {
    @EnvironmentObject var navigator: NavigationCoordinator

    var screen: Screen

    var body: some View {
        VStack(spacing: 20) {
            switch screen {
            case .screen1, .screen3, .screen4:
                Button("Push Next") {
                    navigator.pushNext()
                }
            case .screen2:
                Button("Push \(Screen.screen3.title)") {
                    navigator.flow.push(.screen3)
                }
                Button("Push \(Screen.screen4.title)") {
                    navigator.flow.push(.screen4)
                }
            case .screen5:
                Text("End")
            }
        }
        .navigationTitle(screen.title)
    }
}

My hunch was that the "current" screen flow.array.last that we're reading in pushNext was getting out of sync if I manually navigated too quickly via button presses. But in this trivial example, it works just fine on iOS 15.

Will continue digging to see if I can figure out what's different and triggering the issue.

@chrisballinger
Copy link
Author

I might be jumping the gun, but I may have uncovered something promising. I noticed that the push issues only seemed to ever happen starting from the 2nd screen, never from the first. The main difference on the 2nd screen is that we had some Buttons in a LazyVStack that triggered the navigation to the 3rd screen. By switching from a LazyVStack to a VStack I have much more difficulty reproducing the issue (so far so good!).

I tried also adding a LazyVStack to this example but still wasn't able to reproduce it there.

In any case, reading flow.array.last and making assumptions about it reflecting the "current" screen is likely not safe (even long after a user has been sitting on that screen).

@chrisballinger
Copy link
Author

Hmmm nope, it didn't resolve the issue entirely and still affects another screen (that never used LazyVStack). Nothing too unconventional on that screen other than a GeometryReader around a ScrollView, will see if I can figure out what's going on there.

But at the very least navigation from items in a LazyVStack seems to cause at least some of the issues.

@chrisballinger
Copy link
Author

Spoke to soon, moving to VStack from LazyVStack didn't fix the issue on the other screens, it just made it less likely to happen.

@johnpatrickmorgan
Copy link
Owner

Thanks @chrisballinger, is the project you're seeing this issue on shareable at all please? I'd like to help figure out what's happening if possible.

@chrisballinger
Copy link
Author

Hi! I'd love to share more but am bound by client confidentiality agreements. Will look into our options for a workaround and get back to you!

@chrisballinger
Copy link
Author

@johnpatrickmorgan I was able to make a reproducible test case! Full write up here: https://github.com/Rightpoint/FlowStacks-iOS-15-Bug

I believe it might be related to calling replaceNFlow more than once (not in quick succession though, just at all). Takes a few attempts and relaunches sometimes.

Also submitted a radar FB9685923

@johnpatrickmorgan
Copy link
Owner

@chrisballinger Thanks so much for your time in exploring and documenting this behaviour. I was able to reproduce the issue using your example after a few attempts. I don't have much intuition about what's causing it, but at least I can now investigate it. Thanks!

@zntfdr
Copy link
Contributor

zntfdr commented Oct 8, 2021

Hi @chrisballinger thank you for the test case. I, too, confirm that I could easily reproduce the reported issue.

I played with the project a bit, and it seems that I managed to fix the issue (as in, I launched the app 5-6 times, and I can no longer reproduce the issue).

I've changed the ContentView and DetailScreen definition as following:

struct ContentView: View {
    @StateObject var navigator: NavigationCoordinator = .init()
    
    var body: some View {
        NavigationView {
            NStack($navigator.flow) { screen in
                DetailScreen(screen: screen, onNext: onNext, onPush: onPush, onReset: onReset)
            }
        }
        .environmentObject(navigator)
    }
    
    func onNext() {
        navigator.pushNext()
    }
    
    func onPush(_ screen: Screen) {
        navigator.flow.push(screen)
    }
    
    func onReset(_ screen: Screen) {
        navigator.flow.replaceNFlow(with: [screen])
    }
}

struct DetailScreen: View {
    var screen: Screen
    var onNext: () -> Void
    var onPush: (_ screen: Screen) -> Void
    var onReset: (_ screen: Screen) -> Void
    
    var body: some View {
        Group {
            switch screen {
                case .screen1, .screen3, .screen4:
                    Button("Push Next", action: onNext)
                case .screen2:
                    LazyVStack(spacing: 20) {
                        Button("Push \(Screen.screen3.title)") {
                            onPush(.screen3)
                        }
                        Button("Push \(Screen.screen4.title)") {
                            onPush(.screen4)
                        }
                    }
                case .screen5:
                    Button("Replace root \(Screen.screen6.title)") {
                        onReset(.screen6)
                    }
                case .screen6:
                    Button("Replace root \(Screen.screen1.title)") {
                        onReset(.screen1)
                    }
            }
        }
        .navigationTitle(screen.title)
    }
}

In short, what I did was removing DetailScreen's NavigationCoordinator dependency.
This dependency was probably triggering DetailScreen's body re-evaluation enough times that it'd somehow break SwiftUI's navigation.

Please try and see if this approach solves the issue in your app.

PS

Another theory about what's causing the issue might be connected with this "SwiftUI's Known Issue", from the watchOS 8 release notes:

You can’t push to a third screen after popping from a third screen in the navigation stack. (79076444)

If I'm not mistaken, this issue was present on the iOS/iPadOS 15 beta release notes as well, but was removed when the iOS 15 RC was released (however, from what I see in the community, the problem still persists even on the final iOS/iPadOS 15 releases).

@chrisballinger
Copy link
Author

Thank you both for taking a look!

I'll give that approach a whirl and let you know if that fixes it. I bet you're right that this is related to that same underlying bug in SwiftUI and was only partially resolved in the iOS 15 RC.

@johnpatrickmorgan
Copy link
Owner

Thanks @zntfdr, your suggested changes overcame the issue in my testing, and your explanation makes a lot of sense! 🙌

@maximkrouk
Copy link

maximkrouk commented Oct 11, 2021

Here is a little helper, that helps synchronizing a view with an initial navigation stack

public struct StackNavigationView<Screen, Content: View>: View {
  @Binding var flow: NFlow<Screen>
  @State var isLocked = false
  var content: (Screen) -> Content
  
  public var body: some View {
    NavigationView {
      NStack($flow, buildView: content)
    }
    .allowsHitTesting(!isLocked)
    .onAppear {
      isLocked = true
      $flow.restore {
        isLocked = false
      }
    }
  }
}

extension Binding {
  public func restore<Screen>(completion: (() -> Void)? = nil) where Value == NFlow<Screen> {
    let array = wrappedValue.array
    self.wrappedValue.popToRoot()
    array.dropFirst().enumerated().forEach { index, screen in
      DispatchQueue.main.asyncAfter(deadline: .now() + .milliseconds(500 * (index + 1))) {
        self.wrappedValue.push(screen)
        if index == array.count - 2 {
          completion?()
        }
      }
    }
    if array.count <= 1 {
      completion?()
    }
  }
}

Maybe it is worth adding something like that for the replaceNFlow method
Btw setting an update frequency higher than 2 screens by second breaks the NavigationView

@chrisballinger
Copy link
Author

@maximkrouk Thanks! Looks like a useful snippet, however that is addressing a separate issue (not being able to push more than a single view at once). The bug referenced in this issue is a different SwiftUI internals / layout engine bug

@maximkrouk
Copy link

Yep, it's just a workaround, also, just got an idea: maybe temporary disabling push animation using UINavigationBar.setAnimationsEnabled(false) right before the restoration block and enabling it in the completion may allow to reduce push duration.

@chrisballinger
Copy link
Author

@maximkrouk Yeah that's definitely helpful but it's a workaround for another issue, it's not related to the underlying problem here

@johnpatrickmorgan
Copy link
Owner

Thanks for sharing that @maximkrouk. I'd played around with something similar, but I hadn't thought of exposing that functionality on the binding itself, which neatly allows state to be mutated after a delay. I experimented with your approach on a draft PR #5 and it seems to work well. It's a shame we have to use such workarounds though - hopefully SwiftUI will support such things in the future!

I think the original issue raised by @chrisballinger has been resolved to some extent, so I'll close this issue (but please let me know if you disagree).

@chrisballinger
Copy link
Author

@johnpatrickmorgan I don't think the original issue has been completely resolved, just a workaround has been identified. Since it doesn't look like it's possible to detect "holding it wrong" from within the library itself, I think it might be worth exploring that a bit in the README and example code, to ensure that end users break the cycle and don't end up with child views observing too much parent state. Otherwise you might end up with people opening up a new issue for this same bug, or think that it's no longer an issue. Still probably a bug in the SwiftUI internals that might get fixed at some point.

The workaround I ended up using was less than ideal to minimize refactoring, but something like this:

final class NavigationState: ObservableObject {
    @Published var flow = NFlow<Screen>()
}

final class NavigationCoordinator: ObservableObject {
    /// Do NOT mark this as `@Published`. This is to avoid a SwiftUI bug on iOS 15 where
    /// the navigation state gets desynchronized from the actual state of the nav controller
    var state: NavigationState?

    func showExample() {
        state?.flow.push(.example)
    }
}
struct NavigatedView: View {
    @ObservedObject var navigationState: NavigationState

    var body: some View {
        NavigationView {
            NStack($navigationState.flow) { screen in
// some Child Views
public struct ParentView: View {
    @StateObject var navigator: NavigationCoordinator = .init()
    @StateObject var navigationState: NavigationState = .init()

    public var body: some View {
        NavigatedView(navigationState: navigationState)
        .environmentObject(navigator)        
        .onAppear {
            navigator.state = navigationState
        }
    }
}
public struct ChildView: View {
    @EnvironmentObject var navigator: NavigationCoordinator

    public var body: some View {
        Button(action: {
            navigator.showExample()
        }, label: {
            Text("Show Example")
        })
    }
}

@johnpatrickmorgan
Copy link
Owner

Thanks @chrisballinger I'll try to address the issue in the README.

In case it's useful, when I played with your example project, I ended up using an Unobserved wrapper:

class Unobserved<T: ObservableObject>: ObservableObject {
    let object: T
  
    init(_ object: T) {
        self.object = object
    }
}

so the environment could provide both a Navigator for the parent view and an Unobserved<Navigator> for the child views.

@chrisballinger
Copy link
Author

Oh that's a great idea!

@johnpatrickmorgan
Copy link
Owner

johnpatrickmorgan commented Oct 29, 2021

Thanks, I added a warning to the README to avoid screen's observing navigation state.

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

No branches or pull requests

4 participants