Snapshot and deactivate Visitable during it's lifecycle #181

pfeiffer · 2024-02-23T14:13:32Z

This PR fixes #180.

Today, when dismissing eg. a modal, a snapshot is not taken of the WebView before the dismissal. This is different to the conceptual idea of snapshots in Turbo, where a snapshot it taken before any navigation events happen, so that any re-visits could show a snapshot.

This leads to some unexpected behavior, the most simple example being in the Demo app, where opening the modal example multiple times leads to no or stale snapshots being shown, as described in #180.

This PR addresses that by notifying adding two new lifecycle methods to the Session - visitableViewWillDisappear and visitableViewDidDisappear. When a Visitable will disappear (eg. if closing a modal or otherwise navigating away from it natively), a snapshot is taken of the page, ensuring that we have a fresh snapshot to show on next re-visit. When the Visitable did disappear, it's WebView will be deactivated, ensuring that there's no "orphaned", but active WebView.

In a normal single-session setup this should have no effect, but it fixes some of the issues we've seen while using a multi-session setup (eg. with tabs, modals, stacks etc.).

The behavior before in the demo app with a timeout on the /new page and showing a random number:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-23.at.14.23.13.mov

The behavior with the changes in this PR:

AfterPR.mov

CC @jayohms @joemasilotti

…issing a modal

jayohms · 2024-02-23T14:59:35Z

Thanks @pfeiffer! This is definitely a longstanding issue and something we never addressed. The issue presents itself in our 37signals apps when navigating from a native screen -> webview screen -> back to native screen -> revisit webview screen. The webview may not have a snapshot at all or have a stale snapshot before the fresh data is loaded.

My one concern is whether the snapshot caching approach isn't quite smart enough and we'll be double-snapshotting in the normal visit flow in a single session.

Also, FYI, we'll need to address this in turbo-android as well. We'll need similar solutions and with problems like this that impact both platforms, we won't merge PRs until both platforms have been solved in a consistent way. I'll begin on a turbo-android implementation today to prove out the approach there.

jayohms · 2024-02-23T16:58:20Z

@pfeiffer I implemented an initial, very similar solution for turbo-android here: hotwired/turbo-android#303

It does appear to solve the issue nicely. The only concern I have that remains is now both the native library and core turbo.js will trigger a snapshot cache when navigating between web screens in the same session. I don't have a clean solution in mind at the moment to avoid that — and maybe it won't an issue in practice, but it does bother me a bit.

pfeiffer · 2024-02-23T20:19:32Z

The only concern I have that remains is now both the native library and core turbo.js will trigger a snapshot cache when navigating between web screens in the same session. I don't have a clean solution in mind at the moment to avoid that — and maybe it won't an issue in practice, but it does bother me a bit.

@jayohms I think that is a valid concern and I had the same when implementing this. One thing I could think of is if there'd be a potential race condition when eg. manipulating the page before snapshotting, eg. via turbo:before-cache. Could a double-snapshot request interfere with each other?

I suppose what we want is to only snapshot from native side if no other Visitable will/didAppear as part of the will/didDisappear cycle, but I guess that would require a new var to track that. I tried something along the lines of guard topmostVisit === currentVisit else { return } with no luck.

jayohms · 2024-02-23T21:23:00Z

Yeah, it's not exactly straightforward to solve this elegantly. Let's think about it a bit more and see if we can come up with an approach that avoids double-caching for normal web -> web visits.

pfeiffer · 2024-02-26T09:08:01Z

Yeah, it's not exactly straightforward to solve this elegantly. Let's think about it a bit more and see if we can come up with an approach that avoids double-caching for normal web -> web visits.

@jayohms I gave it some thought over the weekend and I have potential, albeit not that elegant solution working, skipping the snapshot caching from native side, when performing a normal JS visit via Turbo with something like:

// Session
private var disappearingVisitNeedingSnapshot: Visit?

public func visitableViewWillAppear(_ visitable: Visitable) {
  self.disappearingVisitNeedingSnapshot = nil
  // ...
}

public func visitableViewWillDisappear(_ visitable: Visitable) {
  self.disappearingVisitNeedingSnapshot = topmostVisit
}

public func visitableViewDidDisappear(_ visitable: Visitable) {
  disappearingVisitNeedingSnapshot?.cacheSnapshot()
  // ..
}

This works as expected; it delays the snapshotting until after a potential new visit has been triggered, in which case the disappearingVisitNeedingSnapshot is reset and snapshotting is skipped.

What are your thoughts? Is it worth it?

jayohms · 2024-02-26T15:48:39Z

@pfeiffer interesting, I don't think this would work when going from a web -> native screen, would it? We want to capture the WebView snapshot when when leaving a web screen, but navigating to a fully native screen, too.

pfeiffer · 2024-02-26T16:08:30Z

@pfeiffer interesting, I don't think this would work when going from a web -> native screen, would it? We want to capture the WebView snapshot when when leaving a web screen, but navigating to a fully native screen, too.

It would; in case of navigating to a native screen, the visitableViewWillAppear is never called for the appearing screen, as it's is not in the same session (it's a native screen), and the snapshot would be taken in the visitableViewDidDisappear as the disappearingVisitNeedingSnapshot hasn't been nil'ed.

…ill appear to avoid double snapshots

pfeiffer · 2024-02-27T05:31:17Z

@jayohms Pushed an update here with the proposed solution, and we now avoid double-snapshotting when moving from web -> web (handled by Turbo JS), while taking a snapshot from native side when moving from web -> native or between two web sessions (ie. modal)

During my testing, I've discovered an unrelated issue with the native -> web navigations, where the Session is not able to pick up that it's a restore visit until the new did appear and thus will display a blank screen during the restore animation when moving from native to web. I'll address that in a separate PR. The issue is unrelated to the snapshotting in this PR and is present today as well.

Is the double-snapshotting also an issue with hotwired/turbo-android#303?

…screen

pfeiffer · 2024-02-27T05:51:15Z

During my testing, I've discovered an unrelated issue with the native -> web navigations, where the Session is not able to pick up that it's a restore visit until the new did appear and thus will display a blank screen during the restore animation when moving from native to web. I'll address that in a separate PR. The issue is unrelated to the snapshotting in this PR and is present today as well.

In order to address that, I actually needed access to the last disappearing visit, so I've pushed the fix to this PR instead in 661b3a9.

An advance -> restore flow between web and native will now snapshot when advancing as well as detect the restore when navigating back, activating the WebView during the transition:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-27.at.06.44.48.mov

pfeiffer · 2024-02-28T06:00:02Z

Source/Session/Session.swift

@@ -225,7 +229,7 @@ extension Session: VisitableDelegate {
        } else if visitable === currentVisit.visitable && currentVisit.state == .started {
            // Navigating forward - complete navigation early
            completeNavigationForCurrentVisit()
-        } else if visitable !== topmostVisit.visitable {
+        } else if visitable !== topmostVisit.visitable || visitable === lastDisappearingVisit?.visitable {


This covers the case of restoring from native screen back to web, which previously wasn't considered a restore. The web view would both topmost and current visit, even when navigating back.

This works as expected.
Since this is an or operation I'd propose to evaluate the latter in a separate else if statement.
This will make it clear that we're dealing with two types of backward navigation.
I've addressed it and added comments in my feedback PR #189

pfeiffer · 2024-03-04T09:44:13Z

Hey @jayohms, is there anything I can help with to land this?

svara · 2024-03-04T13:42:13Z

@pfeiffer I'll wrap up the review today or tomorrow at the latest.

svara

The changes work as expected. Nice work @pfeiffer!
I've left some feedback in #189.

svara · 2024-03-04T16:38:35Z

Source/Session/Session.swift

@@ -225,7 +229,7 @@ extension Session: VisitableDelegate {
        } else if visitable === currentVisit.visitable && currentVisit.state == .started {
            // Navigating forward - complete navigation early
            completeNavigationForCurrentVisit()
-        } else if visitable !== topmostVisit.visitable {
+        } else if visitable !== topmostVisit.visitable || visitable === lastDisappearingVisit?.visitable {


This works as expected.
Since this is an or operation I'd propose to evaluate the latter in a separate else if statement.
This will make it clear that we're dealing with two types of backward navigation.
I've addressed it and added comments in my feedback PR #189

svara · 2024-03-04T16:41:29Z

Source/Session/Session.swift

@@ -35,6 +35,7 @@ public class Session: NSObject {

    private var currentVisit: Visit?
    private var topmostVisit: Visit?
+    private var disappearingVisitForSnapshotting: Visit?


I've renamed disappearingVisitForSnapshotting to previousVisit in the feedback PR #189.
cc @jayohms @joemasilotti What do you think about the name?

hotwired/turbo-ios#181

Snapshot and deactivate Visitable during its lifecycle, eg. when dism…

a63f658

…issing a modal

pfeiffer mentioned this pull request Feb 23, 2024

Snapshots are not taken when VisitableViewController is dismissed (ie. modal session) #180

Closed

jayohms mentioned this pull request Feb 23, 2024

Ensure that a snapshot is cached before navigating to a new location hotwired/turbo-android#303

Open

Session: Only cache snapshot from native side if no other Visitable w…

1390d04

…ill appear to avoid double snapshots

Detect restore visit in viewWillAppear when navigating from a native …

661b3a9

…screen

jayohms mentioned this pull request Feb 27, 2024

Fix snapshot cache issue #173

Closed

pfeiffer commented Feb 28, 2024

View reviewed changes

svara mentioned this pull request Mar 4, 2024

Snapshot and deactivate Visitable during it's lifecycle feedback #189

Merged

svara reviewed Mar 4, 2024

View reviewed changes

jayohms merged commit 661b3a9 into hotwired:main Mar 22, 2024

joemasilotti added a commit to hotwired/hotwire-native-ios that referenced this pull request Mar 23, 2024

Backport turbo-ios PR #181

487f950

hotwired/turbo-ios#181

joemasilotti mentioned this pull request Mar 23, 2024

Backport turbo-ios PR #181 hotwired/hotwire-native-ios#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot and deactivate Visitable during it's lifecycle #181

Snapshot and deactivate Visitable during it's lifecycle #181

pfeiffer commented Feb 23, 2024

jayohms commented Feb 23, 2024

jayohms commented Feb 23, 2024

pfeiffer commented Feb 23, 2024

jayohms commented Feb 23, 2024

pfeiffer commented Feb 26, 2024 •

edited

Loading

jayohms commented Feb 26, 2024

pfeiffer commented Feb 26, 2024 •

edited

Loading

pfeiffer commented Feb 27, 2024

pfeiffer commented Feb 27, 2024

pfeiffer Feb 28, 2024

svara Mar 4, 2024

pfeiffer commented Mar 4, 2024

svara commented Mar 4, 2024

svara left a comment

svara Mar 4, 2024

svara Mar 4, 2024

Snapshot and deactivate Visitable during it's lifecycle #181

Snapshot and deactivate Visitable during it's lifecycle #181

Conversation

pfeiffer commented Feb 23, 2024

jayohms commented Feb 23, 2024

jayohms commented Feb 23, 2024

pfeiffer commented Feb 23, 2024

jayohms commented Feb 23, 2024

pfeiffer commented Feb 26, 2024 • edited Loading

jayohms commented Feb 26, 2024

pfeiffer commented Feb 26, 2024 • edited Loading

pfeiffer commented Feb 27, 2024

pfeiffer commented Feb 27, 2024

pfeiffer Feb 28, 2024

Choose a reason for hiding this comment

svara Mar 4, 2024

Choose a reason for hiding this comment

pfeiffer commented Mar 4, 2024

svara commented Mar 4, 2024

svara left a comment

Choose a reason for hiding this comment

svara Mar 4, 2024

Choose a reason for hiding this comment

svara Mar 4, 2024

Choose a reason for hiding this comment

pfeiffer commented Feb 26, 2024 •

edited

Loading

pfeiffer commented Feb 26, 2024 •

edited

Loading