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

Include minimal layout in frame responses #428

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

kevinmcconnell
Copy link
Contributor

Previously, responses to frame requests were rendered without any layout. Since Turbo does not need the content outside of the frame, reducing the amount that is rendered can be a useful optimisation.

However, this optimisation prevents responses from specifying head content as well. There are cases where it would be useful for Turbo to have access to items specified in head, like the meta tags used for specifying visit and cache control.

To get around this, we change the optimisation to use a minimal layout for frame responses, rather than no layout. With this, applications can render content into the head if they want, and the Turbo Rails helpers like turbo_page_requires_reload will also work.

@seanpdoyle
Copy link
Contributor

While I think this is a helpful change to support ongoing @hotwired/turbo development, I think it's worth considering the changes proposed in #232.

Conceptually, a new and separate minimized page layout provides an additional source of deviation and drift when weighed against rendering frame responses in the exact same way as other responses.

In the past, @hotwired/turbo has worked-around the fact that it receives incomplete pages for the Turbo-Frame response by synthesizing a full document out of the page's outerHTML. A change like this would affect that process. If Turbo could expect full HTML documents for text/html responses, that work around wouldn't be necessary.

@kevinmcconnell
Copy link
Contributor Author

I think the advantage to using a minimal layout wrapper, as opposed to a full layout, is that we'd still get the benefits of the optimisation, including the reduced rendering effort. And the small responses are easy to read when debugging, which is minor, but quite handy. The extra html/head elements here don't really impact that.

If there's a case for sending full-layout responses to frame requests, maybe we should provide people with an easy & obvious way to turn the optimisation off? (You can already do that by specifying an explicit layout, but we could make it more explicit).

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Feb 6, 2023

I think the advantage to using a minimal layout wrapper, as opposed to a full layout, is that we'd still get the benefits of the optimisation, including the reduced rendering effort.

If there's a case for sending full-layout responses to frame requests, maybe we should provide people with an easy & obvious way to turn the optimisation off?

What do you think about implementing the reverse. Full responses by default, then a turn-key configuration to render with the optimized minimal layout?

Regardless of defaults, I do agree that a well-documented configuration makes a lot of sense here.

Copy link
Contributor

@afcapel afcapel left a comment

Choose a reason for hiding this comment

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

@kevinmcconnell looks great 🙌

What do you think about implementing the reverse. Full responses by default, then a turn-key configuration to render with the optimized minimal layout?

@seanpdoyle I think minimal layout by default makes more sense for two reasons:

  1. It's closer to the original behavior, so less likely to introduce surprises when people upgrade.
  2. Rendering a turbo_rails/frame also adds an extension point without any need for extra configuration. If you'd rather render the application layout you can create an app/views/layouts/turbo_rails/frame.html.erb and render application.html.erb(or whatever) from there. This would not work the other way araound. Changing the minimal response is a fairly edge case, so I think it's ok for the option to be available but not very prominent. Most people won't need any changes and they'll get optimized version out of the box.

#
# This is merely a rendering optimization. Everything would still work just fine if we rendered everything including the layout.
# Turbo Frames knows how to fish out the relevant frame regardless.
# When that header is detected by the controller, we substitute our own minimal layout in place of the
Copy link
Contributor

Choose a reason for hiding this comment

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

What about mentioning the minimal layout is turbo_rails/frame.html.erb? People can still override the default if the want creating their own app/views/layouts/turbo_rails/frame.html.erb file. Probably not a very common case, but still doable if someone wants to do it.

Choose a reason for hiding this comment

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

What about mentioning the minimal layout is turbo_rails/frame.html.erb? People can still override the default if the want creating their own app/views/layouts/turbo_rails/frame.html.erb file.

I like that idea. Action Text's documentation makes a similar suggestion for folks needing to customize the default layout in https://guides.rubyonrails.org/action_text_overview.html#rendering-rich-text-content

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Feb 6, 2023

@afcapel that's a great point!

If you'd rather render the application layout you can create an app/views/layouts/turbo_rails/frame.html.erb and render application.html.erb(or whatever) from there.

Is the idea that you'd declare something like:

<%# app/views/layouts/turbo_rails/frame.html.erb %>
<%= render layout: "application" do %>
  <%= yield %>
<% end %>

If that's the case, could we also introduce test coverage of some kind that exercises that behavior to make sure it's possible?

Copy link

@packagethief packagethief left a comment

Choose a reason for hiding this comment

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

Thanks @kevinmcconnell. I think this strikes a good balance. It ensures that frame responses contain a full HTML document, but it avoids any performance penalty from rendering a potentially-expensive application layout.

I wanted to add that I agree with @seanpdoyle's arguments in #232 and elsewhere about the benefits of returning full documents instead of fragments. I think the problem of "breaking out" of frames is a perfect illustration: as soon as frame responses are documents, things become simpler. Earlier breakout proposals are often working around the fact that there was no document to configure, and the suitability of turbo-visit-control was never apparent.

Hotwire's premise is that shipping HTML over the wire is fine, and that dropping down to fragments is an unnecessary optimization that in the worst case can lead to the invention of an ad-hoc, informally-specified, bug-ridden, slow implementation of half of a document format. Best to steer clear 😅

@kevinmcconnell
Copy link
Contributor Author

Thanks for the feedback @seanpdoyle, @afcapel & @packagethief!

I agree that we should document the way to extend that default template. I don't think it's be something that people should need often, but we should make it clear for any cases where they do. I've added an explanation for that, and a corresponding test.

Previously, responses to frame requests were rendered without any
layout. Since Turbo does not need the content outside of the frame,
reducing the amount that is rendered can be a useful optimisation.

However, this optimisation prevents responses from specifying
`head` content as well. There are cases where it would be useful for
Turbo to have access to items specified in head, like the meta tags used
for specifying visit and cache control.

To get around this, we change the optimisation to use a minimal layout
for frame responses, rather than no layout. With this, applications can
render content into the `head` if they want, and the Turbo Rails helpers
like `turbo_page_requires_reload` will also work.
@kevinmcconnell kevinmcconnell merged commit a2384ae into main Feb 7, 2023
@kevinmcconnell kevinmcconnell deleted the frames/include-minimal-layout branch February 7, 2023 11:48
@davidalejandroaguilar
Copy link
Contributor

@kevinmcconnell Maybe I'm missing something, but is this not going to cause some scripts to be loaded twice?

@kevinmcconnell
Copy link
Contributor Author

@davidalejandroaguilar I'm not sure what you mean. Can you elaborate a bit?

@davidalejandroaguilar
Copy link
Contributor

@kevinmcconnell So I'm thinking that if you're showing messages/index.html.erb that has <% content_for :head %> that adds some scripts to your layout (which has yield :head). If a turbo frame response from this page also has yield :head it would cause the browser to load the same scripts twice.

@kevinmcconnell
Copy link
Contributor Author

@davidalejandroaguilar for frame responses, Turbo already knows how to extract the frame content from the response, and only include the relevant portion on the page. It doesn’t depend on that having been stripped out by the server.

In the same way, you can use your full layout on your frame responses if you want to. The fact that turbo rails renders with less/no layout is an optimisation to avoid rendering and shipping content that won’t be used. It’s not a necessary part of frames working correctly.

@davidalejandroaguilar
Copy link
Contributor

@kevinmcconnell Thanks, I do understand that. Maybe I'm not understanding this PR. I thought this PR would cause Turbo to start including <head> content when placing the frame content into the DOM.

But I think I see now that the overall "ignore-everything-outside-the-turbo-frame" behavior is done on Turbo Javascript and will still keep this behavior. This PR will just add the head tag so that Turbo Javascript can pick specific information encoded there to alter some behavior, if needed.

Did I get that right?

@packagethief
Copy link

Did I get that right?

Exactly @davidalejandroaguilar 👍

seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Mar 2, 2023
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 added a commit to seanpdoyle/turbo that referenced this pull request Mar 2, 2023
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 added a commit to seanpdoyle/turbo that referenced this pull request Mar 2, 2023
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
@nickjj
Copy link

nickjj commented Mar 2, 2023

Hi,

Does something else need to change on the Turbo side to take advantage of this behavior?

For example with Turbo Rails 1.4.0 and Turbo 7.3.0, the following doesn't update:

layouts/application.html.erb

  <head>
    <title><%= yield :title %></title>
  </head>

views/example/show.html.erb

<%
  content_for(:title) { 'Something cool' }
%>

If you hard reload /example/1 the title gets set since Turbo isn't being used. If you goto /example and then click a link to /example/1 inside of a Turbo Frame using advance, the URL updates but the <title> does not get updated.

There is an open issue from last year on Turbo at hotwired/turbo#600, I was hoping this PR may have fixed it but I tested it in an app I'm building and the title along with other <head> tags do not get updated.

@kevinmcconnell
Copy link
Contributor Author

Hi @nickjj this change doesn't affect how Turbo processes frame responses. Turbo still excludes everything outside of the frame itself, except for specific items like Turbo-specific meta tags that it checks for. So although this change sounds a bit like what you're describing, it's not really the same thing, unfortunately.

@rromanchuk
Copy link

rromanchuk commented Mar 8, 2023

There's something anti-pattern i must be doing, but this release broke every regular GET link I have

For example GET /users to GET /users/:id

The response (200) did not contain the expected <turbo-frame id="users"> and will be ignored. To perform a full page visit instead, set turbo-visit-control to reload.

I guess because of this frame in the layout? Don't really understand how now rendering the layout would still cause that error message

<%#  app/views/layouts/application.html.erb" %>
<body>
<%= yield %>
<%#  render "application/current_user" %>
<%= turbo_frame_tag :current_user do %>

<% end %>
</body>

@packagethief
Copy link

packagethief commented Mar 8, 2023

did not contain the expected <turbo-frame id="users">

This suggests the request was made from within a turbo-frame element with the id "users". Could there be an outer frame wrapping your links that's gone unnoticed? In previous releases this would have appeared to work, and the fact that the request originated from a frame would be ignored.

@turgs
Copy link

turgs commented Apr 30, 2023

Hi @nickjj this change doesn't affect how Turbo processes frame responses. Turbo still excludes everything outside of the frame itself, except for specific items like Turbo-specific meta tags that it checks for. So although this change sounds a bit like what you're describing, it's not really the same thing, unfortunately.

Thanks for your work on this with commits and merging it in @seanpdoyle @kevinmcconnell

I really wish I could update the <title> tag with <head> from a turbo frame what was loaded via data-turbo-action="advance". That updates the URL, but in these "advance" instances I need it to update the titlebar too.

Seems like a few others were expecting this too:

@loust333
Copy link

loust333 commented Jul 14, 2023

Hi everyone,

I want to express my gratitude for the hard work you've put into improving the turbo-rails gem.

I'd like to share some feedback regarding this pull request and @turgs' comment:

We integrated turbo-rails into our SaaS project to enhance page loading speed and ensure a faster interface, leveraging turbo frames.

Example:

In our "companies" controller, specifically in the transition between companies#show and companies#edit, we added a turbo_frame with the "advance" action, as suggested by @turgs.

While implementing this, we encountered the same issue as @turgs. The tag was not being updated, preventing us from modifying certain elements like the <title> tag. While this wasn't a critical problem for us, as our clients would not pay attention to the <title> tag.

Everything worked flawlessly until we deployed the changes to the staging environment. 🦸

In companies#edit, we have a form that includes a "file_field" input for file uploads. Under normal circumstances, there were no issues—except when we utilized the clever solution <%= f.file_field :logo, direct_upload: true %>.

(On a humorous note, I primarily use Chrome, which shielded me from this reality. 😂)

Chrome employs caching to store user sessions, so when we had the CSRF token within the tag using <%= csrf_meta_tags %> for security purposes, and we loaded the turbo frame through turbo frames, along with a file_field utilizing direct_upload, Chrome retained the CSRF token while loading the turbo frame.

However, this led to a problem for users who prefer Safari. When simulating the same scenario with Safari, the file_field would block the file upload process, and the form displayed the error message: "can't verify CSRF token authenticity."

After an entire day of patience and countless cups of coffee, I believe I've grasped the problem and stumbled upon this pull request. Despite testing the minimal layout and failing to achieve the desired outcome, I reached the same conclusion as @turgs and others. It would be fantastic if there was a way to update a section of the tag using the turbo frame and the "advance" action.

I hope my experience helps others avoid similar mistakes.

Currently, there are three solutions to address the Safari issue:

  • Remove the turbo frame tag ✅
  • Utilize the turbo frame tag but load the entire page with target="_top" 🤷‍♂️
  • Remove the direct_upload: true attribute from the file_field 💔

Thank you all once again for your efforts in making turbo-rails better.

Best regards,

seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Jul 21, 2023
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 added a commit to seanpdoyle/turbo that referenced this pull request Sep 10, 2023
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
shiftyp pushed a commit to shiftyp/ts-turbo that referenced this pull request Sep 11, 2023
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
@sc0ttman
Copy link

sc0ttman commented Oct 5, 2023

I was hoping this PR may have fixed it but I tested it in an app I'm building and the title along with other <head> tags do not get updated.

@nickjj Yea it looks like you need to use Turbo Drive for this. We had disabled it by default and were hoping the same <head> merging functionality would exist with Turbo Frames but it appears it does not.
If you have Drive off by default you can replace the Frame tag with a <div data-turbo="true"> and it should behave similar with the exception that you now have to replace the full page.

@nickjj
Copy link

nickjj commented Oct 5, 2023

@sc0ttman I am using drive for most things but I have a very heavy sidebar with unique user content that's not really cache'able that I wanted to avoid re-rendering by loading the main content area in with a frame when sidebar links were clicked.

I hope one day they allow this behavior with frames, it's a really common feature in my opinion to be able to update the page's title or meta tags while still using frames without having to resort to a custom hand rolled Javascript approach.

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

Successfully merging this pull request may close these issues.

10 participants