Skip to content

Create Frame Snapshot from Fetch Response HTML #887

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Create Frame Snapshot from Fetch Response HTML #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/core/frames/frame_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { PageSnapshot } from "../drive/page_snapshot"
import { TurboFrameMissingError } from "../errors"

export class FrameController {
fetchResponseLoaded = (_fetchResponse) => {}
fetchResponseLoaded = (_fetchResponse) => Promise.resolve()
#currentFetchRequest = null
#resolveVisitPromise = () => {}
#connected = false
Expand Down Expand Up @@ -140,7 +140,7 @@ export class FrameController {
}
}
} finally {
this.fetchResponseLoaded = () => {}
this.fetchResponseLoaded = () => Promise.resolve()
}
}

Expand Down Expand Up @@ -315,7 +315,7 @@ export class FrameController {
this.complete = true
session.frameRendered(fetchResponse, this.element)
session.frameLoaded(this.element)
this.fetchResponseLoaded(fetchResponse)
await this.fetchResponseLoaded(fetchResponse)
} else if (this.#willHandleFrameMissingFromResponse(fetchResponse)) {
this.#handleFrameMissingFromResponse(fetchResponse)
}
Expand Down Expand Up @@ -354,10 +354,10 @@ export class FrameController {
const pageSnapshot = PageSnapshot.fromElement(frame).clone()
const { visitCachedSnapshot } = frame.delegate

frame.delegate.fetchResponseLoaded = (fetchResponse) => {
frame.delegate.fetchResponseLoaded = async (fetchResponse) => {
if (frame.src) {
const { statusCode, redirected } = fetchResponse
const responseHTML = frame.ownerDocument.documentElement.outerHTML
const responseHTML = await fetchResponse.responseHTML
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely cause a full page reload because the responseHTML will only have a minimal <head> and therefore the tracked elements won't be identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this @domchristie. I'm not sure how to best resolve the issue mentioned in #1047. Do you have an idea on what to change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seanpdoyle Can this line be reverted or will that break the intention of #867 / hotwired/turbo-rails#428?

Otherwise, I'm beginning to feel that the different visit usages warrant a different modelling.

For example:

  1. A standard Visit that navigates from one location to the next
  2. A same-page anchor Visit
  3. A redirected Visit
  4. A Frame Visit (promoted to a Visit using data-turbo-action

Each has different expectations and outcomes, which hints that they could be different classes that share the same API
#1044 (comment)

This would be a fairly big refactor, but might be worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#430 explores a FrameVisit, but I'm unsure of what changes would be necessary to incorporate a fix for this issue.

Can this line be reverted or will that break the intention of #867 / hotwired/turbo-rails#4

I'm unsure of the implications of reverting it. Is there a conditional that could be introduced to support responses without a <head> and responses from newer turbo-rails versions that send a minimal head?

Copy link
Contributor

@domchristie domchristie Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure of the implications of reverting it.

It seems like v7.3.0 includes #867 so reverting should work (although I'm not sure how!?)

Off the top of my head (pun intended ;), I'm not sure how HeadSnapshot could distinguish between a "minimal layout" head from a frame visit and standard visit, where the page's tracked assets have actually changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I'm unsure of what changes would be necessary to incorporate a fix for this issue.

It doesn't feel right to me that we call session.proposeVisitIfNavigatedWithAction after rendering a frame response. (In the same way it doesn't feel right to propose a second visit after rendering a redirected response.) In these cases, we're not "visiting" a new location, we're just using the side-effects of the visit lifecycle.

In terms of what I had in mind for implementing different visit types: I was thinking that each Visit type could share an interface, and each implementation could respond accordingly (e.g. a RedirectVisit wouldn't dispatch any events because the Visit lifecycle events have already been broadcast by the initial Visit.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any progress?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change broke Restoration Visits with turbo-frame data-turbo-action="advance".

see:

Is this something we can revert or is there a different approach we can take?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also see #1300

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tleish thanks for all the referenced links.

I'm also experiencing this problem where page cache gets messed up when combining turbo frames with turbo action "advance" and navigating back after having navigated forward in the frame.

This is, I think, a pretty common usage pattern for turbo frames. I've seen it promoted on many blog posts and youtube videos like this one) so it calls my attention this hasn't been addressed yet.

The best workaround I've found (lost track of the source by now) is to use a custom app/views/layouts/turbo_rails/frame.html.erb file that includes the whole same head section from the app/views/layouts/application.html.erb template overriding the trimmed-down version of the turbo-rails gem. But I'm not sure that solves all edge cases.

<!-- app/views/layouts/turbo_rails/frame.html.erb -->
<html>
  <head>
    <!-- This same line should be in app/views/layouts/application.html.erb
     and then both layouts would share the same head -->
    <%= render "layouts/application_head" %>
  </head>
  <body>
    <%= yield %>
  </body>
</html>

const response = { statusCode, redirected, responseHTML }
const options = {
response,
Expand Down
20 changes: 9 additions & 11 deletions src/tests/fixtures/frame_preloading.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
<!DOCTYPE html>
<html>

<head>
<meta charset="utf-8">
<title>Page With Preloading Frame</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
</head>

<body>
<turbo-frame id="menu" src="/src/tests/fixtures/frames/preloading.html"></turbo-frame>
</body>

<head>
<meta charset="utf-8">
<title>Frame Preloading</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="menu" src="/src/tests/fixtures/frames/preloading.html"></turbo-frame>
</body>
</html>
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/body_script.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Body Script</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="body-script" data-loaded-from="/src/tests/fixtures/frames/body_script.html">
Expand Down
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/body_script_2.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Body Script 2</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="body-script" data-loaded-from="/src/tests/fixtures/frames/body_script_2.html">
Expand Down
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/eval_false_script.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Eval False Script</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="eval-false-script" data-loaded-from="/src/tests/fixtures/frames/eval_false_script.html">
Expand Down
2 changes: 1 addition & 1 deletion src/tests/fixtures/frames/form-redirect.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Form Redirect</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
Expand Down
2 changes: 1 addition & 1 deletion src/tests/fixtures/frames/form-redirected.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Form Redirected</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
Expand Down
2 changes: 1 addition & 1 deletion src/tests/fixtures/frames/form.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<html>
<head>
<meta charset="utf-8">
<title>Form</title>
<title>Frames: Form</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
Expand Down
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/frame.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Frame</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<h1>Frames: #frame</h1>
Expand Down
17 changes: 14 additions & 3 deletions src/tests/fixtures/frames/frame_for_eager.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
<turbo-frame id="eager-loaded-frame" >
<h2>Eager-loaded frame: Loaded</h2>
</turbo-frame>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Frames: Frame for Eager</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="eager-loaded-frame" >
<h2>Eager-loaded frame: Loaded</h2>
</turbo-frame>
</body>
</html>
2 changes: 1 addition & 1 deletion src/tests/fixtures/frames/hello.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Hello</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
Expand Down
17 changes: 14 additions & 3 deletions src/tests/fixtures/frames/part.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
<turbo-frame id="part">
<h2>Frames: #frame-part</h2>
</turbo-frame>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Frames: Part</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="part">
<h2>Frames: #frame-part</h2>
</turbo-frame>
</body>
</html>
19 changes: 15 additions & 4 deletions src/tests/fixtures/frames/preloading.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
<turbo-frame id="menu">
<a href="/src/tests/fixtures/preloaded.html" id="frame_preload_anchor" data-turbo-preload="true">Visit preloaded
page</a>
</turbo-frame>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Frames: Preloading</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="menu">
<a href="/src/tests/fixtures/preloaded.html" id="frame_preload_anchor" data-turbo-preload="true">Visit preloaded
page</a>
</turbo-frame>
</body>
</html>
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/recursive.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<title>Frames: Recursive</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="recursive">
Expand Down
1 change: 1 addition & 0 deletions src/tests/fixtures/frames/self.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<meta charset="utf-8">
<title>Frames: Self</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="frame" src="/src/tests/fixtures/frames/self.html">
Expand Down
3 changes: 2 additions & 1 deletion src/tests/fixtures/frames/unvisitable.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<meta charset="utf-8">
<title>Frame</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
<meta name="turbo-visit-control" content="reload" />
</head>
<body>
Expand Down
5 changes: 0 additions & 5 deletions src/tests/fixtures/frames/without_layout.html
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/tests/fixtures/rendering.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ <h1>Rendering</h1>
<p><a id="visit-control-reload-link" href="/src/tests/fixtures/visit_control_reload.html">Visit control: reload</a></p>
<p><a id="permanent-element-link" href="/src/tests/fixtures/permanent_element.html">Permanent element</a></p>
<p><a id="permanent-in-frame-element-link" href="/src/tests/fixtures/permanent_element.html" data-turbo-frame="frame">Permanent element in frame</a></p>
<p><a id="permanent-in-frame-without-layout-element-link" href="/src/tests/fixtures/frames/without_layout.html" data-turbo-frame="frame">Permanent element in frame without layout</a></p>
<p><a id="delayed-link" href="/__turbo/delayed_response">Delayed link</a></p>
<p><a id="redirect-link" href="/__turbo/redirect">Redirect link</a></p>
<form>
Expand Down
2 changes: 1 addition & 1 deletion src/tests/fixtures/tabs.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<head>
<meta charset="utf-8">
<title>Tabs</title>
<script src="/dist/turbo.es2017-umd.js"></script>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
Expand Down
27 changes: 19 additions & 8 deletions src/tests/fixtures/tabs/three.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
<turbo-frame id="tab-frame" data-turbo-action="advance">
<div>
<a id="tabs-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
<a id="tabs-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
<a id="tabs-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
</div>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="tab-frame" data-turbo-action="advance">
<div>
<a id="tab-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
<a id="tab-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
<a id="tab-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
</div>

<div id="tab-content">Three</div>
</turbo-frame>
<div id="tab-content">Three</div>
</turbo-frame>
</body>
</html>
27 changes: 19 additions & 8 deletions src/tests/fixtures/tabs/two.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
<turbo-frame id="tab-frame" data-turbo-action="advance">
<div>
<a id="tab-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
<a id="tab-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
<a id="tab-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
</div>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Frame</title>
<script src="/dist/turbo.es2017-umd.js" data-turbo-track="reload"></script>
<script src="/src/tests/fixtures/test.js"></script>
</head>
<body>
<turbo-frame id="tab-frame" data-turbo-action="advance">
<div>
<a id="tab-1" href="/src/tests/fixtures/tabs.html">Tab 1</a>
<a id="tab-2" href="/src/tests/fixtures/tabs/two.html">Tab 2</a>
<a id="tab-3" href="/src/tests/fixtures/tabs/three.html">Tab 3</a>
</div>

<div id="tab-content">Two</div>
</turbo-frame>
<div id="tab-content">Two</div>
</turbo-frame>
</body>
</html>
9 changes: 0 additions & 9 deletions src/tests/functional/rendering_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,15 +433,6 @@ test("test preserves permanent elements within turbo-frames", async ({ page }) =
assert.equal(await page.textContent("#permanent-in-frame"), "Rendering")
})

test("test preserves permanent elements within turbo-frames rendered without layouts", async ({ page }) => {
assert.equal(await page.textContent("#permanent-in-frame"), "Rendering")

await page.click("#permanent-in-frame-without-layout-element-link")
await nextBeat()

assert.equal(await page.textContent("#permanent-in-frame"), "Rendering")
})

test("test restores focus during turbo-frame rendering when transposing the activeElement", async ({ page }) => {
await page.press("#permanent-input-in-frame", "Enter")
await nextBeat()
Expand Down