Only update history when Turbo visit is renderable #601

manuelpuyol · 2022-06-14T16:27:32Z

Follow up to #571, which ended up just moving around the scroll bug
Should close #387

Context

We've been experiencing some bugs when Turbo visits fail (e.g due to tracked element mismatches or visit-control reload), where Turbo was keeping the previous page scroll position after the page was updated:

Screen.Recording.2022-06-13.at.5.47.27.PM.mov

Problem

From what I see here, Turbo always manually updates the browser state using visit.changeHistory(). This will update the URL before we navigate to the new page. This is fine when Turbo replaces the body, since Turbo will manage everything for us.
The problem lies when a visit is not possible, causing a turbo:reload event. In this case, Turbo was calling window.location.reload() (after the URL was updated), which works like hitting the refresh button in the browser.
Since the reload was called while the previous page was still loaded and scrolled, reload will try to keep the scroll state after the new page loads, causing the bug.

Proposed solution

Turbo should only update the browser history when it knows that it can handle the request. Since we don't actually need the new URL until we replace the body, we can wait for renderPageand renderError to know if the request is renderable or not.
If the request is a success and Turbo can visit, it updates the history like before and continues on.
If the request fails for any reason (404, 500, visit control is reload, element mismatch....), Turbo will delegate to the browser to navigate to the new page, which now is done via window.location.href.

Current state

Now, when we click an element that causes a full reload, the browser will either navigate to the top of the new page or to the location of the targeted element. Notice that when hitting the back button, the previous scroll position is preserved, improving navigation.

Screen.Recording.2022-06-14.at.12.00.30.PM.mov

koddsson

I'm not on the Turbo team and have a bias towards getting this merged but this looks good to me 😄

koddsson · 2022-06-15T11:16:37Z

src/core/drive/page_view.ts

@@ -16,15 +17,18 @@ export class PageView extends View<Element, PageSnapshot, PageViewRenderer, Page
  lastRenderedLocation = new URL(location.href)
  forceReloaded = false

-  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true) {
+  renderPage(snapshot: PageSnapshot, isPreview = false, willRender = true, visit?: Visit) {


Probably in a follow-up refactor but it's starting to look like this function should take in a "options" object rather than individual arguments.

koddsson · 2022-06-15T11:17:40Z

src/tests/fixtures/visit_control_reload.html

+    <hr class="push-off-screen">
+
+    <h2 id="middle">Middle the page</h2>
+    <hr class="push-off-screen">
+    <h2>Down the page</h2>


Very nit-pick but:

Suggested change

<hr class="push-off-screen">

<h2 id="middle">Middle the page</h2>

<hr class="push-off-screen">

<h2>Down the page</h2>

<hr class="push-off-screen">

<h2 id="middle">Middle the page</h2>

<hr class="push-off-screen">

<h2>Down the page</h2>

dhh · 2022-06-15T12:19:07Z

cc @packagethief

packagethief · 2022-06-15T19:10:26Z

src/core/native/browser_adapter.ts

    visit.loadCachedSnapshot()
    visit.issueRequest()
-    visit.changeHistory()


I'm not positive, but moving the history changing to PageView might have implications for the native adapters for iOS and Android. The adapters will still be calling visit.changeHistory themselves, so it will be called twice:

https://github.com/hotwired/turbo-ios/blob/2277f7d/Source/WebView/turbo.js#L71

https://github.com/hotwired/turbo-android/blob/14ad418/turbo/src/main/assets/js/turbo_bridge.js#L70

The second should be a no-op because history will have been flagged as changed, but I think we'll still have the same problem.

how do we avoid calling it twice now?

It's only called once by each adapter now. If we move it out of the browser adapter, we may need to move it out of the other adapters as well. Perhaps we don't (it's been a while since I've looked at the native frameworks), but we need to make sure it behaves as expected on iOS and Android.

Any tips on how to test that properly? Looking at https://github.com/manuelpuyol/turbo/blob/2eb9ea2c7282f1ec2b41c75705d7e43af7e2076a/src/core/drive/visit.ts#L173-L174 I'd expect it to not do a double update

You're right, it will be a no-op because we've already flagged history as having changed. My concern is that we're changing the behavior by moving this out of the adapter, so we just need to ensure it's all good on iOS and Android 👍

@jayohms Can you verify whether this would bring any complication on native?

I don't foresee any issues on the mobile side, since as mentioned, the second visit.changeHistory call will be a no-op, so the mobile adapters will continue to work as before. It'd be nice to update the mobile adapters to reflect this change, however, we've maintained backwards/forwards compatibility for all iterations of turbo/links API changes.

I don't think we'll be able to reasonably make corresponding changes in the mobile adapters without exposing an additional API, so the adapters can know to conditionally change history (or not). This shouldn't be a problem in the short-term, but it may be a nuance that's difficult for others to understand as time goes on.

jayohms · 2022-06-21T13:29:49Z

src/core/native/browser_adapter.ts

    visit.loadCachedSnapshot()
    visit.issueRequest()
-    visit.changeHistory()


I don't foresee any issues on the mobile side, since as mentioned, the second visit.changeHistory call will be a no-op, so the mobile adapters will continue to work as before. It'd be nice to update the mobile adapters to reflect this change, however, we've maintained backwards/forwards compatibility for all iterations of turbo/links API changes.

I don't think we'll be able to reasonably make corresponding changes in the mobile adapters without exposing an additional API, so the adapters can know to conditionally change history (or not). This shouldn't be a problem in the short-term, but it may be a nuance that's difficult for others to understand as time goes on.

jayohms · 2022-06-21T13:32:46Z

src/core/native/browser_adapter.ts

+
+    if (!this.location) return
+
+    window.location.href = this.location.toString()


Is there any potential that this causes issues with or prevents reloads for same-page anchors? I'm not clear if that situation is possible here.

I can't imagine a case where a same-page anchor causes a full page reload

* main: Drive Browser tests with `playwright` (hotwired#609) Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612) Only update history when Turbo visit is renderable (hotwired#601) Support development ChromeDriver version overrides (hotwired#606) Turbo stream source (hotwired#415) Expose Frame load state via `[complete]` attribute (hotwired#487)

* main: Allow frames to scroll smoothly into view (hotwired#607) Export Type declarations for `turbo:` events (hotwired#452) Add .php as a valid isHTML extension (hotwired#629) Add original click event to 'turbo:click' details (hotwired#611) Drive Browser tests with `playwright` (hotwired#609) Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612) Only update history when Turbo visit is renderable (hotwired#601) Support development ChromeDriver version overrides (hotwired#606) Turbo stream source (hotwired#415) Expose Frame load state via `[complete]` attribute (hotwired#487) fix(ie/edge): form.method='delete', raises Invalid argument. (hotwired#586) Do not declare global types/constants (hotwired#524) Defensively create custom turbo elements (hotwired#483) Use `replaceChildren` in StreamActions.update (hotwired#534)

Closes hotwired#762 Changes made in [hotwired#601][] had unintended side-effects that prevented same-page anchor navigations from writing to the history. This commit adds a call to `Visit.updateHistory()` within `Visit.goToSamePageAnchor()` along with coverage in `src/tests/functional/navigation_tests.ts` to guard against future regressions. [hotwired#601]: hotwired#601

Closes #762 Changes made in [#601][] had unintended side-effects that prevented same-page anchor navigations from writing to the history. This commit adds a call to `Visit.updateHistory()` within `Visit.goToSamePageAnchor()` along with coverage in `src/tests/functional/navigation_tests.ts` to guard against future regressions. [#601]: #601

manuelpuyol added 7 commits June 14, 2022 10:23

use location.replace instead of location.reload

6dcf1eb

update when we change the history

f6456da

get location from visitStart

b3c08c3

always pass the visit to renderError/Page

ed11325

rollback name update

fe0d60e

revert visit_control view hack

6b726a5

await nextBeat for the URL to be ready

3dc8ac7

manuelpuyol changed the title ~~Improve scroll position when navigating~~ Only update history when Turbo visit is renderable Jun 14, 2022

manuelpuyol marked this pull request as ready for review June 14, 2022 17:02

remove console log

2eb9ea2

koddsson approved these changes Jun 15, 2022

View reviewed changes

dhh requested a review from packagethief June 15, 2022 12:20

packagethief reviewed Jun 15, 2022

View reviewed changes

dhh modified the milestones: 7.1.0, 7.2.0 Jun 19, 2022

jayohms reviewed Jun 21, 2022

View reviewed changes

dhh merged commit 98cdc40 into hotwired:main Jun 22, 2022

kevinmcconnell mentioned this pull request Oct 13, 2022

Location not updated when clicking on same-page fragment links #762

Closed

seanpdoyle mentioned this pull request Oct 13, 2022

Update history during same-page Visits #763

Merged

manuelpuyol mentioned this pull request Dec 12, 2022

Turbo history navigation may behave oddly in Safari #810

Open

domchristie mentioned this pull request Oct 24, 2023

Do not dispatch multiple visit events on redirect visit #1044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only update history when Turbo visit is renderable #601

Only update history when Turbo visit is renderable #601

manuelpuyol commented Jun 14, 2022 •

edited

Loading

koddsson left a comment

koddsson Jun 15, 2022

koddsson Jun 15, 2022

dhh commented Jun 15, 2022

packagethief Jun 15, 2022

manuelpuyol Jun 16, 2022

packagethief Jun 16, 2022

manuelpuyol Jun 16, 2022

packagethief Jun 16, 2022

dhh Jun 18, 2022

jayohms Jun 21, 2022

jayohms Jun 21, 2022

jayohms Jun 21, 2022

manuelpuyol Jun 22, 2022


		if (!this.location) return

		window.location.href = this.location.toString()

Only update history when Turbo visit is renderable #601

Only update history when Turbo visit is renderable #601

Conversation

manuelpuyol commented Jun 14, 2022 • edited Loading

Context

Problem

Proposed solution

Current state

koddsson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhh commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelpuyol commented Jun 14, 2022 •

edited

Loading