Create Frame Snapshot from Fetch Response HTML #887

seanpdoyle · 2023-03-02T09:32:04Z

After the changes made in @hotwired/turbo#867 and changes made in @hotwired/turbo-rails#428 (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with Turbo-Frame: headers.

Prior to this commit, the FrameController compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the <html> element's outerHTML.

This commit changes the fetchResponseLoaded callback to read the responseHTML directly from the FetchResponse, since that will be a fully formed HTML document in Turbo v7.3.0 and later.

To support that change, this commit also updates various src/test/fixtures files to render fully-formed HTML documents.

packagethief

This looks good to me, @seanpdoyle. Thanks for tidying up!

BALOTIAS · 2023-03-15T13:12:41Z

I honestly didn't know if I should create an issue or write it here. Since v7.3.0 the turbo frame missing event doesn't break out of the frame anymore, this has a big impact for a project that I'm working on. We had to downgrade to v.7.2.5 again, and would like to have re-add this behavior back. Will this PR also support this?

document.addEventListener('turbo:frame-missing', event => {
  event.preventDefault() // Cancel event to not replace turbo-frame content with "Content missing"

  // With 7.3.0: Break out of the turbo frame and visit the response url -> this sucks since we have to request a second time
  event.detail.visit(event.detail.response.url) // Currently we have to do this 

  // Break out of the turbo frame and replace current view with its response -> doesn't currently work as the responseHTML is missing in the response
  // TODO: Wait for https://github.com/hotwired/turbo/pull/887
  // event.detail.visit(event.detail.response.url, { response: event.detail.response })
})

Maybe I'm unaware of something?

After the changes made in [@hotwired/turbohotwired#867][] and changes made in [@hotwired/turbo-railshotwired#428][] (the canonical server-side implementation), Turbo expects full HTML documents in response to requests with `Turbo-Frame:` headers. Prior to this commit, the `FrameController` compensated for missing pieces of an HTML document by taking an HTML "snapshot" of the current page through the `<html>` element's [outerHTML][]. This commit changes the `fetchResponseLoaded` callback to read the `responseHTML` directly from the `FetchResponse`, since that will be a fully formed HTML document in Turbo v7.3.0 and later. To support that change, this commit also updates various `src/test/fixtures` files to render fully-formed HTML documents. [@hotwired/turbohotwired#867]: hotwired#867 [@hotwired/turbo-railshotwired#428]: hotwired/turbo-rails#428 [outerHTML]: https://developer.mozilla.org/en-US/docs/Web/API/Element/outerHTML

seanpdoyle · 2023-09-10T20:36:33Z

I've rebased this to account for the migration away from TypeScript.

packagethief

Thanks @seanpdoyle. This looks great.

@afcapel, is this something we can merge as part of the work you've been doing? Not sure if you've got a released planned or not.

afcapel · 2023-09-12T08:57:03Z

Yes, it looks good, thanks @seanpdoyle!

domchristie · 2023-10-30T11:08:25Z

src/core/frames/frame_controller.js

        if (frame.src) {
          const { statusCode, redirected } = fetchResponse
-          const responseHTML = frame.ownerDocument.documentElement.outerHTML
+          const responseHTML = await fetchResponse.responseHTML


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.

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?

@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:

A standard Visit that navigates from one location to the next

A same-page anchor Visit

A redirected Visit

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?

#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?

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

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.)

Any progress?

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

see:

Issue: Turbo frame history not navigating properly w/ back button after upgrading from 7 to 8

PR: Support back button (restore) even if page loaded in frame does not contain matching HEAD

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

also see #1300

seanpdoyle force-pushed the frame-response-html branch from d42605e to 61fe92a Compare March 2, 2023 09:37

packagethief approved these changes Mar 8, 2023

View reviewed changes

hebertcisco approved these changes Mar 11, 2023

View reviewed changes

seanpdoyle force-pushed the frame-response-html branch from 61fe92a to 3882c6a Compare July 21, 2023 15:25

seanpdoyle force-pushed the frame-response-html branch from 3882c6a to ed72ba1 Compare September 10, 2023 20:36

seanpdoyle requested a review from packagethief September 10, 2023 20:36

packagethief approved these changes Sep 11, 2023

View reviewed changes

afcapel merged commit d63e198 into hotwired:main Sep 12, 2023

domchristie mentioned this pull request Oct 26, 2023

Promoted Frame Visits cause tracked_element_mismatch reloads #1047

Closed

domchristie reviewed Oct 30, 2023

View reviewed changes

seanpdoyle deleted the frame-response-html branch October 30, 2023 15:41

tpaulshippy mentioned this pull request Jun 18, 2024

Turbo frame history not navigating properly w/ back button after upgrading from 7 to 8 #1241

Open

YoutacRandS-VA mentioned this pull request Jun 19, 2024

[Snyk] Upgrade @hotwired/turbo from 7.3.0 to 8.0.4 YoutacRandS-VA/osv.dev#4

Open

tpaulshippy mentioned this pull request Jun 24, 2024

Support back button (restore) even if page loaded in frame does not contain matching HEAD #1281

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Frame Snapshot from Fetch Response HTML #887

Create Frame Snapshot from Fetch Response HTML #887

seanpdoyle commented Mar 2, 2023

packagethief left a comment

BALOTIAS commented Mar 15, 2023

seanpdoyle commented Sep 10, 2023

packagethief left a comment

afcapel commented Sep 12, 2023

domchristie Oct 30, 2023

seanpdoyle Oct 30, 2023

domchristie Oct 30, 2023

seanpdoyle Oct 30, 2023

domchristie Oct 30, 2023 •

edited

Loading

domchristie Oct 30, 2023

qinmingyuan Nov 26, 2023

tleish Jun 24, 2024

fonji Oct 31, 2024

Create Frame Snapshot from Fetch Response HTML #887

Create Frame Snapshot from Fetch Response HTML #887

Conversation

seanpdoyle commented Mar 2, 2023

packagethief left a comment

Choose a reason for hiding this comment

BALOTIAS commented Mar 15, 2023

seanpdoyle commented Sep 10, 2023

packagethief left a comment

Choose a reason for hiding this comment

afcapel commented Sep 12, 2023

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

domchristie Oct 30, 2023 • edited Loading

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

domchristie Oct 30, 2023 •

edited

Loading