[Bug]: UIKit FlowPersistence.persistWhenSkipped causes a flash

[nickkaczmarek]

What happened?

In the UIKit example app there is a view that is persistWhenSkipped. When you go to a page that skips that view, you see a flash of content when the menu displays and then the next view slides over.

Simulator.Screen.Recording.-.iPod.touch.7th.generation.-.2022-03-01.at.14.22.57.mp4

Version

SwiftCurrent: 4.5.20

Relevant code sample

.thenProceed(with: SomeViewController.self, flowPersistence: .persistWhenSkipped)

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[Tyler-Keith-Thompson]

You might find that is device and/or simulator dependent.

[nickkaczmarek]

🤔

[nickkaczmarek]

We're noticing that there are ViewControllers that have shouldLoad inside of them, but in a twist that was unexpected there are proceedInWorkflow calls inside of those shouldLoads. Not sure if this is the intended way of using the shouldLoad api, but will need to investigate more. We were attempting to utilize shouldLoad as a metric for whether a view should animate to the next view, but this is throwing a wrench in it.

Also the mentioned issue is happening both in the simulator and the latest version of iOS on my phone.

I suspect a consumer of the library would find this behavior odd (flash of content) and would endeavor to fix it or just remove the library and that's why we're trying to remove this behavior.

[MattFreiburgAsynchrony]

Another symptom of what may be the same problem is that when you go through the last item in the "Pick A Location" screen (Pickup and Delivery w/ all menu types), and when you click on either Pickup or Delivery on the "Pickup or Delivery" screen, that navigation occurs without an animation, by virtue of the .persistWhenSkipped attribute in the workflowItem definition. If you take that attribute out, the animation occurs as one would expect. I think this happens by virtue of the animation variable logic on line #149 of UIKitPresenter in the displayInstance(:style:view:root:completion) func. I think we need to find a way to enable animation to a destination workflowItem that is marked with .persistWhenSkipped (.hiddenInitially) when the prior workflowItem was not skipped. Finding that getting to that determination and passing it to this bit of code is going to be challenging.

[MattFreiburgAsynchrony]

Also, with regard to the behavior @nickkaczmarek pointed out in the initial post here, wondering if we could not add the skipped workflowItem VC to the nav stack at all (not .push it) then use nav.setViewControllers in order to true up the navigation stack after doing the navigation to the following VC. I.e., given VC1, VC2, VC3 in the workflow, where VC2 is skipped, find a way to just .push from VC1 directly to VC3 with animation True, then immediately afterward call nav.setViewControllers([VC1, VC2, VC3], animated: false) to true it up. The funky flash seems to appear because when we are skipping VC2, we're .push-ing from VC1 to VC2 with animation false then immediately .push-ing from VC2 to VC3 with animation true.

[Tyler-Keith-Thompson]

shouldLoad calling proceedInWorkflow is expected and defined behavior. Effectively the FlowRepresentable is saying "I should not appear in the workflow, but I will contribute to the data". A perfect example is a screen that allows you to select multiple options...but there's only one option because of business logic. So it picks its one option but does not load.

[Tyler-Keith-Thompson]

I do suggest that this issue get marked as "Low Priority" though. It's a great find, but I suspect real-world impact in minimal.

[nickkaczmarek]

I'm not sure how often this behavior would be used, but I do think it's worth looking into because while the view claims to not load, it totally does and you can visually see it. I think if I were to see this visual glitch while developing I'd be tempted to eject the library that caused it and I as a library maintainer would like to reduce that possibility. I think we should time box this, but I still think it's important.

[morganzellers]

Agree - I think it's work looking into for a bit

[nickkaczmarek]

Something else I'm wondering is, "Is using flowPersistence.persistWhenSkipped for anything other than a deep linking scenario the right use case for it?"

Currently what I'm seeing is that a view with that attribute seems to replace the previous view and then an animation occurs that slides over it so it's just sort of a janky behavior. However, if this isn't the intended use case maybe we can just document that so if this comes up we'll be prepared.

[nickkaczmarek]

Talked more about this with @Tyler-Keith-Thompson last week and sort of came to the conclusion that this is a valid use case, but might be slightly contrived in the example. However there is a case in which we'd want to skip over a view based on an action and then make it so that a user can navigate back to said view to modify the selection.

Also this is worth fixing, but boy is it going to be challenging. Probably going to keep this on the back burner as we attempt to fix more pressing issues like _abandon causing memory leaks.

[nickkaczmarek]

Clearing assignees to let it be known that this effort will be handled by the community if they desire the change.