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

Turbo visit with replace action flashes elements #136

Closed
jch opened this issue Aug 11, 2023 · 11 comments
Closed

Turbo visit with replace action flashes elements #136

jch opened this issue Aug 11, 2023 · 11 comments

Comments

@jch
Copy link

jch commented Aug 11, 2023

In local development (isolating slow network), the app is smooth between Turbo replace visits in Safari and Chrome. However, on the simulator and actual hardware, there's a flash during the rendering.

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-10.at.18.15.45.mp4

I was able to reproduce this using the turbo-native-demo app with an additional link to make the flash more obvious.

diff --git a/views/two.ejs b/views/two.ejs
index a5e485f..749402c 100644
--- a/views/two.ejs
+++ b/views/two.ejs
@@ -7,3 +7,4 @@
   <p>This screen was pushed on the navigation stack through an <code>advance</code> action. You can go back to the previous screen by tapping the <em>Back</em> button.</p>
   <p>Go back and try the <em>replace</em> link to see the differenc

https://github.com/hotwired/turbo-ios/assets/19874/ab015a85-e623-4821-a64b-33e3b68859db

e.</p>
 <% } %>
+<p><a href="/two?action=replace" class="button@native space --buttom-flush" data-turbo-action="replace" data-turbolinks-action="replace">Replace with another webpage</a></p>

Another example from my app which has more elements (it's still smooth from safari / chrome)

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-10.at.17.56.00.mp4

https://jch.app/demo for live link for comparison

@jackwurth
Copy link

jackwurth commented Oct 2, 2023

it's because it's trying to show the loading icon as it changes to a whole new web view.

It's not possible to easily get it to behave like it does in safari since the first thing it does is disable the normal Turbo page behaviour.

@seanpdoyle
Copy link

@jayohms @joemasilotti @afcapel @jorgemanrubia What kind of support could the community provide to move this forward?

To give some more background, we're in the process of upgrading an application's web-side Turbo dependency from 7 to 8. A large portion of Turbo 8's value proposition hinges on "replace" Turbo Action navigations (for example, to integrate with Morphing).

The white flashing offsets any interactivity gained from Morphing and draws attention to the hybrid nature of the application.

We've found that #155 might help here. Are there other explorations into resolving this issue that could use some more testing help?

@jorgemanrubia
Copy link
Member

@seanpdoyle we fixed a similar issue with #160 and hotwired/turbo-android#292. Is this one different to those?

@pfeiffer
Copy link
Contributor

On a related note, I've found that Turbo Native doesn't differentiate between replace actions triggered by navigation (eg. a tap on a button or pull-to-refresh) and replace actions triggered programatically, eg. Turbo Stream.

Combined with the fact that Turbo iOS shows a spinner if there's no snapshot cache and that Turbo web exempts a page-to-be-refreshed from snapshot cache:

  refresh(url, requestId) {
    const isRecentRequest = requestId && this.recentRequests.has(requestId)
    if (!isRecentRequest) {
      this.cache.exemptPageFromPreview()
      this.visit(url, { action: "replace" })
    }
  }

.. this means that any refresh stream received will show a spinner and flash of white content while the content is being fetched and morphed. On slow-loading pages subscribing to broadcasts this essentially makes the screen unusable, as it will become unresponsive at any given time. See for instance:

MorphingShowingSpinners.mp4

I think a stream-triggered refresh should not show any blocking spinners, but I haven't found a good way to skip spinners as there's no differentiation today.

I've opened a PR to add a refresh action to Turbo, which I hope could be used for Turbo iOS to not show activity spinners when doing replace or refreshes with morphing.

We'll be happy to help with a PR for the native libraries that would support a better logic around showing activity indicators for replace actions if this could be a way forward?

@pfeiffer
Copy link
Contributor

As a less invasive workaround, I've opened hotwired/turbo#1188 which stores a snapshot of the page before doing a refresh. This at least removes any blocking loading indicators while refreshing, but the visitable views is still disabled during the refresh action (ie. a screenshot is shown instead of the WebView, while the refresh is on-going):

TurboReplaceMorphing.mov

I still believe that the native adapters would need the granularity that an explicit refresh action would provide in order to keep the WebView activated while a refresh is happening.

@jayohms
Copy link
Collaborator

jayohms commented Feb 16, 2024

@pfeiffer I'm going to look at this closer next week so we can get this issue over the finish line.

On a related note, I've found that Turbo Native doesn't differentiate between replace actions triggered by navigation (eg. a tap on a button or pull-to-refresh) and replace actions triggered programatically, eg. Turbo Stream.

Do you see a visit proposal the iOS library when a Turbo Stream replace action is triggered? I don't believe we need a separate refresh action, because if the proposed visit url to the adapter is the same as the current url, we should/can bypass the native adapter flow, which causes the progress indicator + flashing.

Are you able to see if the turbo-android demo app displays the same issue in this branch? hotwired/turbo-android#292. It has a slightly different internal implementation than #160 and bypasses some of the native adapter callbacks.

@pfeiffer
Copy link
Contributor

@jayohms I'm really happy to hear that!

Do you see a visit proposal the iOS library when a Turbo Stream replace action is triggered? I don't believe we need a separate refresh action, because if the proposed visit url to the adapter is the same as the current url, we should/can bypass the native adapter flow, which causes the progress indicator + flashing.

Yeah, both visit proposal and visit delegate methods are received on the native side. The latter is especially important as it makes it hard to bypass the native adapter flow. Eg. even if we skip proposing a refresh to the native side, the spinners will still be shown as delegate methods are called during the visit lifecycle (visitStarted, visitRequest[..]) and these trigger progress indicators etc.

As an example, trying to short-circuit any visit proposals to the native adapters on refresh actions like this will still show spinners:

// turbo.js
visitProposedToLocation(location, options) {
  // Bypass proposals to native
  if (new URL(location).pathname === document.location.pathname && options.action === "replace") {
    Turbo.navigator.startVisit(location, "", { "action": "replace" })
  }

  ...
}

On the native side, we can see that the visit lifecycle methods being called:

2024-02-17 07:15:42 +0000 [Bridge] ← visitStarted ["identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909, "timestamp": 1708154142532, "hasCachedSnapshot": 0] [:]
2024-02-17 07:15:42 +0000 [JavascriptVisit] didStartVisitWithIdentifier http://localhost:45678/one ["identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909", "hasCachedSnapshot": false]
2024-02-17 07:15:42 +0000 [Bridge] ← visitRequestStarted ["identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909, "timestamp": 1708154142534] [:]
2024-02-17 07:15:42 +0000 [JavascriptVisit] didStartRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909", "date": 2024-02-17 07:15:42 +0000]
2024-02-17 07:15:43 +0000 [Bridge] ← visitRequestCompleted ["identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909, "timestamp": 1708154143576] [:]
2024-02-17 07:15:43 +0000 [JavascriptVisit] didCompleteRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909"]
2024-02-17 07:15:43 +0000 [Bridge] ← visitRequestFinished ["identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909, "timestamp": 1708154143576] [:]
2024-02-17 07:15:43 +0000 [JavascriptVisit] didFinishRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909", "date": 2024-02-17 07:15:43 +0000]
2024-02-17 07:15:43 +0000 [Bridge] ← visitCompleted ["identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909, "timestamp": 1708154143580, "restorationIdentifier": 077a9beb-adc4-4369-943e-c8bcd138e912] [:]
2024-02-17 07:15:43 +0000 [JavascriptVisit] didCompleteVisitWithIdentifier http://localhost:45678/one ["restorationIdentifier": "077a9beb-adc4-4369-943e-c8bcd138e912", "identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909"]
2024-02-17 07:15:43 +0000 [Bridge] ← visitRendered ["timestamp": 1708154143612, "identifier": 81cb01b0-bc8a-40ba-b4ab-965e303ed909] [:]
2024-02-17 07:15:43 +0000 [JavascriptVisit] didRenderForVisitWithIdentifier http://localhost:45678/one ["identifier": "81cb01b0-bc8a-40ba-b4ab-965e303ed909"]
2024-02-17 07:15:55 +0000 [Bridge] ← visitStarted ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "hasCachedSnapshot": 0, "timestamp": 1708154155349] [:]
2024-02-17 07:15:55 +0000 [JavascriptVisit] didStartVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574", "hasCachedSnapshot": false]
2024-02-17 07:15:55 +0000 [Bridge] ← visitRequestStarted ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "timestamp": 1708154155350] [:]
2024-02-17 07:15:55 +0000 [JavascriptVisit] didStartRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574", "date": 2024-02-17 07:15:55 +0000]
2024-02-17 07:15:56 +0000 [Bridge] ← visitRequestCompleted ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "timestamp": 1708154156373] [:]
2024-02-17 07:15:56 +0000 [JavascriptVisit] didCompleteRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574"]
2024-02-17 07:15:56 +0000 [Bridge] ← visitRequestFinished ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "timestamp": 1708154156374] [:]
2024-02-17 07:15:56 +0000 [JavascriptVisit] didFinishRequestForVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574", "date": 2024-02-17 07:15:56 +0000]
2024-02-17 07:15:56 +0000 [Bridge] ← visitCompleted ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "restorationIdentifier": c1a0137e-2157-43e7-845e-ac6b5eb07360, "timestamp": 1708154156375] [:]
2024-02-17 07:15:56 +0000 [JavascriptVisit] didCompleteVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574", "restorationIdentifier": "c1a0137e-2157-43e7-845e-ac6b5eb07360"]
2024-02-17 07:15:56 +0000 [Bridge] ← visitRendered ["identifier": 0a92b394-920b-4d20-8d60-bc9c65232574, "timestamp": 1708154156394] [:]
2024-02-17 07:15:56 +0000 [JavascriptVisit] didRenderForVisitWithIdentifier http://localhost:45678/one ["identifier": "0a92b394-920b-4d20-8d60-bc9c65232574"]

The lifecycle methods on the native side are responsible for showing the indicators etc., so in this case - even when trying to bypass the native adapter proposal - indicators are still shown.

The culprint for showing indicators is in Session:

    func visitDidStart(_ visit: Visit) {
        guard !visit.hasCachedSnapshot else { return }
        visit.visitable.showVisitableActivityIndicator()
    }

So here we'd could skip calling showVisitableActivityIndicator if the visit is a refresh triggered eg. by a Turbo Stream.

@pfeiffer
Copy link
Contributor

I've opened a PR in the Turbo Native demo server repo bumping Turbo to version 8 and adding buttons to simulate a Turbo Stream refresh: hotwired/turbo-native-demo#30.

@joemasilotti
Copy link
Member

I wonder if adding a delay to showing the spinner could help. Even something very small, like 10ms.

@pfeiffer
Copy link
Contributor

Opened #177 to fix the issue building on the approach by @afcapel in #160.

@jayohms
Copy link
Collaborator

jayohms commented Feb 23, 2024

#178 resolves flickering issues with same-page replace visits with Turbo 8 + morphing. If there are situations not handled with that setup, please open a new issue with the specifics. Thanks!

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

No branches or pull requests

7 participants