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.

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

Jump to bottom

Head merge issue with turbo frame navigation promoted to a page visit #1237

Open
Cervenka opened this issue Apr 3, 2024 · 6 comments
Open

Head merge issue with turbo frame navigation promoted to a page visit #1237

Cervenka opened this issue Apr 3, 2024 · 6 comments

Comments

@Cervenka
Copy link

Cervenka commented Apr 3, 2024 •
edited
Loading

When using the data-turbo-action="advance" attribute the document's head is merged. (Handbook)
During that process meta-tags are removed from the head if the response's head doesn't include them. That includes meta-tags that may be required such as csrf-token or vapid-public-key.

I ran into this issue when using the custom layout provided by turbo-rails.

Thank you

Gems:
turbo-rails (2.0.5)

Packges:
@hotwired/turbo-rails@^8.0.4
@hotwired/turbo@^8.0.4
@Cervenka Cervenka changed the title Header merge issue with turbo frame navigation promoted to a page visit Head merge issue with turbo frame navigation promoted to a page visit Apr 3, 2024
@adampope
Copy link

adampope commented Apr 4, 2024

I have to just run into the exact same issue. I noticed because when navigating within the frame the browser tab title was being changed to the URL and I realised it was because the <title> tag and all <meta> tags were being removed from the <head>.

Initially I thought it was because we were using render partial: 'my_frame' to return just the frame, but swapping to rendering the whole view and using the turbo_rails/frame layout didn't improve the situation. Moving to render :show, layout: 'application' does work, as the <head> is present with all the tags to be replaced, but that's really inefficient to update the contents of one frame.

@adampope
Copy link

adampope commented Apr 9, 2024

I have found the problem in my application. I'm working on an old codebase that had been upgraded to use Turbo. In debugging this error I created a minimal reproduction using rails new and found that the contents of the <head> tag was preserved as you would expect when navigating. So I then started deleting code from the production app to get as close to the repro as possible. After deleting everything I could it still didn't work, so I started diffing the new app and the old app to see if there was anything I was missing. The magic change was

- stylesheet_link_tag "application"
+ stylesheet_link_tag "application",  "data-turbo-track": "reload"

In reverse, if I remove data-turbo-track from the minimal reproduction it exhibits the same behaviour of emptying the <head> of the title and meta tags when clicking a link with the data-turbo-action=advance option in a frame.

From reading the reference on attributes I'm not really sure why this should be the case, so I think this is probably a bug.

@adampope
Copy link

adampope commented Apr 9, 2024

I've now discovered that it isn't specific to the stylesheet, this behaviour happens with you have 0 occurrences of data-turbo-track. In my attempts to simplify my reproduction I had swapped

- <%= javascript_importmap_tags %>
+ <%= javascript_include_tag "turbo", type: "module" %>

The importmap tag appears to include data-turbo-track by default. If I go back to the importmap setup, then I can remove the data-turbo-track attribute from the stylesheet and everything continues to work. So the key seems to be to have at least one script or stylesheet tracked.

@Cervenka
Copy link
Author

Adding the data-turbo-track="reload" to an asset does fix the head merge issue. However it seems to break the back-forward-cache.

@oliverguenther
Copy link

I also see this same problem occur when using a layout: false to return just a frame element. We don't use the turbo_frame template nor it's head content blocks.

I can confirm that data-turbo-track="reload" fixes the issue. From what I can see in the page, this is what happens:

  1. Before visiting, a HeadSnapshot is created that results in the asset element being marked as tracked: https://github.com/hotwired/turbo/blob/main/src/core/drive/head_snapshot.js#L14
  2. This results in the trackedElementSignature to be present: https://github.com/hotwired/turbo/blob/main/src/core/drive/head_snapshot.js#L26-L30
  3. The frame visit happens, and visitProposedToLocation triggers a new visit/snapshot comparison
  4. As there is no new head, the trackedElementsAreIdentical method returns false: https://github.com/hotwired/turbo/blob/main/src/core/drive/page_renderer.js#L92-L94
  5. This results in renderer.shouldRender to result in false: https://github.com/hotwired/turbo/blob/main/src/core/drive/page_view.js#L24-L30
  6. The rest of the rendering still takes place, but it never enters visit.changeHistory which I guess is what is causing the history cache to break

I wonder, is it expected to allow navigation to frame-only pages for performance reasons, and the head comparison should be skipped if there is none? Maybe someone from the core team can comment on this before we try to provide a pull request.

oliverguenther added a commit to opf/openproject that referenced this issue Aug 13, 2024
@oliverguenther
Use data-turbo-track="reload" workaround for head
9a41487 
hotwired/turbo#1237
oliverguenther added a commit to opf/openproject that referenced this issue Sep 5, 2024
@oliverguenther
Use data-turbo-track="reload" workaround for head
bc18faf 
hotwired/turbo#1237
@oliverguenther oliverguenther mentioned this issue Sep 6, 2024
Open
oliverguenther added a commit to opf/openproject that referenced this issue Sep 6, 2024
@oliverguenther
Use data-turbo-track="reload" workaround for head
fdc7417 
hotwired/turbo#1237
@AntonKhorev AntonKhorev mentioned this issue Sep 10, 2024
Draft
@sfnelson
Copy link

@oliverguenther I think you've uncovered the same broken control flow I documented here: hotwired/turbo-rails#580

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sfnelson @oliverguenther @adampope @Cervenka