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

Promoted Frame Visits cause tracked_element_mismatch reloads #1047

Closed
domchristie opened this issue Oct 26, 2023 · 23 comments · Fixed by #1144
Closed

Promoted Frame Visits cause tracked_element_mismatch reloads #1047

domchristie opened this issue Oct 26, 2023 · 23 comments · Fixed by #1144

Comments

@domchristie
Copy link
Contributor

This only impacts main branch builds since #887.

When promoting Frame Visits using data-turbo-action="…", Turbo will reload the page. The turbo:reload event reason is: tracked_element_mismatch. This only appears to be an issue since: #887

/cc @seanpdoyle

@domchristie
Copy link
Contributor Author

@afcapel I think this is quite a major issue, since it breaks all Turbo Frames visits with a data-turbo-action. Would be good to get fixed before v8 is released

@afcapel
Copy link
Collaborator

afcapel commented Nov 24, 2023

Agreed, let's get it fixed before v8 👍

@qinmingyuan
Copy link

qinmingyuan commented Nov 26, 2023

Any progress at this issue?

@seanpdoyle
Copy link
Contributor

@domchristie I'm unable to reproduce this locally in the following test:

test("navigating turbo-frame[data-turbo-action=advance] from within pushes URL state", async ({ page }) => {                                                                                                        
  await page.click("#add-turbo-action-to-frame")                                                                                                                                                                    
  await page.click("#link-frame")                                                                                                                                                                                   
  await nextEventNamed(page, "turbo:frame-load")                                                                                                                                                                    
  await noNextEventNamed(page, "turbo:reload")                                                                                                                                                                      
  await nextEventNamed(page, "turbo:load")                                                                                                                                                                          
  await noNextEventNamed(page, "turbo:reload")                                                                                                                                                                      
                                                                                                                                                                                                                    
  const title = await page.textContent("h1")                                                                                                                                                                        
  const frameTitle = await page.textContent("#frame h2")                                                  
                                                                                                                                                                                                                    
  assert.equal(title, "Frames")                                                                           
  assert.equal(frameTitle, "Frame: Loaded")                                                                                                                                                                         
  assert.equal(pathname(page.url()), "/src/tests/fixtures/frames/frame.html")                                                                                                                                       
})   

I've added some assertions related to turbo:reload:

diff --git a/src/tests/functional/frame_tests.js b/src/tests/functional/frame_tests.js
index b090aff..a796e44 100644
--- a/src/tests/functional/frame_tests.js
+++ b/src/tests/functional/frame_tests.js
@@ -642,7 +642,10 @@ test("navigating a frame with a form[method=get] that does not redirect still up
 test("navigating turbo-frame[data-turbo-action=advance] from within pushes URL state", async ({ page }) => {
   await page.click("#add-turbo-action-to-frame")
   await page.click("#link-frame")
+  await nextEventNamed(page, "turbo:frame-load")
+  await noNextEventNamed(page, "turbo:reload")
   await nextEventNamed(page, "turbo:load")
+  await noNextEventNamed(page, "turbo:reload")
 
   const title = await page.textContent("h1")
   const frameTitle = await page.textContent("#frame h2")

Does this reflect the type of scenario you're encountering?

If you're able to cut a branch or open a pull request that reproduced the issue, I'm happy to take it over and explore solutions.

@qinmingyuan
Copy link

@domchristie I'm unable to reproduce this locally in the following test:

test("navigating turbo-frame[data-turbo-action=advance] from within pushes URL state", async ({ page }) => {                                                                                                        
  await page.click("#add-turbo-action-to-frame")                                                                                                                                                                    
  await page.click("#link-frame")                                                                                                                                                                                   
  await nextEventNamed(page, "turbo:frame-load")                                                                                                                                                                    
  await noNextEventNamed(page, "turbo:reload")                                                                                                                                                                      
  await nextEventNamed(page, "turbo:load")                                                                                                                                                                          
  await noNextEventNamed(page, "turbo:reload")                                                                                                                                                                      
                                                                                                                                                                                                                    
  const title = await page.textContent("h1")                                                                                                                                                                        
  const frameTitle = await page.textContent("#frame h2")                                                  
                                                                                                                                                                                                                    
  assert.equal(title, "Frames")                                                                           
  assert.equal(frameTitle, "Frame: Loaded")                                                                                                                                                                         
  assert.equal(pathname(page.url()), "/src/tests/fixtures/frames/frame.html")                                                                                                                                       
})   

I've added some assertions related to turbo:reload:

diff --git a/src/tests/functional/frame_tests.js b/src/tests/functional/frame_tests.js
index b090aff..a796e44 100644
--- a/src/tests/functional/frame_tests.js
+++ b/src/tests/functional/frame_tests.js
@@ -642,7 +642,10 @@ test("navigating a frame with a form[method=get] that does not redirect still up
 test("navigating turbo-frame[data-turbo-action=advance] from within pushes URL state", async ({ page }) => {
   await page.click("#add-turbo-action-to-frame")
   await page.click("#link-frame")
+  await nextEventNamed(page, "turbo:frame-load")
+  await noNextEventNamed(page, "turbo:reload")
   await nextEventNamed(page, "turbo:load")
+  await noNextEventNamed(page, "turbo:reload")
 
   const title = await page.textContent("h1")
   const frameTitle = await page.textContent("#frame h2")

Does this reflect the type of scenario you're encountering?

If you're able to cut a branch or open a pull request that reproduced the issue, I'm happy to take it over and explore solutions.

I can give a Rails Demo App to reproduce this issue. Are you need one ?

@afcapel
Copy link
Collaborator

afcapel commented Nov 30, 2023

@qinmingyuan thanks for the failing test, that's very useful. No need for a rails app, we should fix this within Turbo.

@seanpdoyle
Copy link
Contributor

@afcapel I believe the failing test mentioned in the comment is a quote from one of the tests that I attempted to write. Unfortunately, it passes and does not reproduce the issue.

@qinmingyuan if you're able to share a Rails application that reproduces the issue, I'd be happy to troubleshoot further. Thank you!

@domchristie
Copy link
Contributor Author

@seanpdoyle https://github.com/domchristie/turbo_promoted_frame_visit_test

It appears to only impact turbo-rails setups. I tried a minimal demo, serving plain html files (where the frame visit response included a minimal layout to mimic the turbo-rails behaviour), and it worked.

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Dec 1, 2023

@domchristie I wonder if it's related to a mostly (or completely) empty <head> element coming from turbo-rails:

https://github.com/hotwired/turbo-rails/blob/4eb4e928e30be8cd537af8073f98b80ddea4a578/app/views/layouts/turbo_rails/frame.html.erb#L2-L4

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

If I copy the application layout, it behaves as expected:

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ mkdir -p app/views/layouts/turbo_rails 

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ cp app/views/layouts/application.html.erb app/views/layouts/turbo_rails/frame.html.erb

where the frame visit response included a minimal layout to mimic the turbo-rails behaviour

Could you share that code? I wonder what about the Rails setup is different.

@qinmingyuan
Copy link

qinmingyuan commented Dec 1, 2023

@domchristie I wonder if it's related to a mostly (or completely) empty <head> element coming from turbo-rails:

https://github.com/hotwired/turbo-rails/blob/4eb4e928e30be8cd537af8073f98b80ddea4a578/app/views/layouts/turbo_rails/frame.html.erb#L2-L4

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

If I copy the application layout, it behaves as expected:

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ mkdir -p app/views/layouts/turbo_rails 

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ cp app/views/layouts/application.html.erb app/views/layouts/turbo_rails/frame.html.erb

where the frame visit response included a minimal layout to mimic the turbo-rails behaviour

Could you share that code? I wonder what about the Rails setup is different.

My code there is nothings in header, but also the same issue:

<html>
  <head></head>
  <body>
    <turbo-frame id="body">
      <%= yield %>
    </turbo-frame>
  </body>
</html>

seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Dec 1, 2023
Re-submission of [hotwired#232][]
Related to [hotwired/turbo#1047][]

Render full documents, including default layout rendering behavior.

Rendering a minimal layout forces `turbo:reload` events because of the
severe difference in the contents of the minimal layout's `<head>` and
the requesting document's fully populated `<head>`.

[hotwired#232]: hotwired#232
[hotwired/turbo#1047]: hotwired/turbo#1047
@seanpdoyle
Copy link
Contributor

I've opened hotwired/turbo-rails#534 to re-propose responding to Turbo Frame requests with full documents.

@qinmingyuan
Copy link

Understood, thanks. Maybe have a better resolution.

@domchristie
Copy link
Contributor Author

Could you share that code? I wonder what about the Rails setup is different.

@seanpdoyle demo gist: https://gist.github.com/domchristie/d143a6e7ae1807bd1f8323ecc8a4978b

@seanpdoyle
Copy link
Contributor

@domchristie thank you! Are you saying that despite the fact that the <head> element is empty, that gist works as expected, whereas https://github.com/domchristie/turbo_promoted_frame_visit_test reloads as a mismatch?

@domchristie
Copy link
Contributor Author

@seanpdoyle yes, exactly

@seanpdoyle
Copy link
Contributor

@domchristie is there unexpected behavior if you make this change?

-  <script src="https://unpkg.com/@hotwired/turbo@8.0.0-beta.1/dist/turbo.es2017-umd.js"></script>
+  <script src="https://unpkg.com/@hotwired/turbo-rails@8.0.0-beta.1/app/assets/javascripts/turbo.js"></script>

I wonder if the issue stems from the Rails side, or the turbo-rails side.

@domchristie
Copy link
Contributor Author

is there unexpected behavior if you make this change?

@seanpdoyle as that's the ESM version, I changed it to:

  <script type="module">
    import { Turbo } from 'https://unpkg.com/@hotwired/turbo-rails@8.0.0-beta.1/app/assets/javascripts/turbo.js'
    Turbo.start()
  </script>

And it works as expected.

@weizheheng
Copy link

@domchristie I wonder if it's related to a mostly (or completely) empty <head> element coming from turbo-rails:

https://github.com/hotwired/turbo-rails/blob/4eb4e928e30be8cd537af8073f98b80ddea4a578/app/views/layouts/turbo_rails/frame.html.erb#L2-L4

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

If I copy the application layout, it behaves as expected:

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ mkdir -p app/views/layouts/turbo_rails 

~/Work/domchristie/turbo_promoted_frame_visit_test main*
❯ cp app/views/layouts/application.html.erb app/views/layouts/turbo_rails/frame.html.erb

where the frame visit response included a minimal layout to mimic the turbo-rails behaviour

Could you share that code? I wonder what about the Rails setup is different.

Chipping in here, I am having the same issues where Turbo reloads when promoting Frame Visits using data-turbo-action="…".

Copying the <head></head> from the layout to turbo_rails/frame.html.erb does stops Turbo from unexpected reloading.

@domchristie
Copy link
Contributor Author

It appears to only impact turbo-rails setups. I tried a minimal demo, serving plain html files (where the frame visit response included a minimal layout to mimic the turbo-rails behaviour), and it worked.
#1047 (comment)

This is not correct. The minimal demo did not have data-turbo-track="reload". Adding that, and we see the same behaviour.

@domchristie
Copy link
Contributor Author

@seanpdoyle the minimal repro gist has been updated to demonstrate the issue when data-turbo-track="reload" is present

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Jan 24, 2024

@afcapel @jorgemanrubia I've encountered this today, and like @domchristie mentioned in #1047 (comment), it's a major impediment to upgrading.

I'm not sure about how to resolve it. Like I mentioned in #1047 (comment), I've opened hotwired/turbo-rails#534. I've also opened #694 to send the Turbo-Action: header when it's present in the requesting context. Turbo Rails could render the full document when both Turbo-Frame: and Turbo-Action: are present. Another alternative is to special-case responses with a completely empty <head>, then intervene to populate them with document.head.innerHTML.

I think #694 (even without the Turbo-Action: header) fits into the Morph-centric mental model: the server renders full HTML documents, then the client decides how to negotiate that new content into the document.

@domchristie
Copy link
Contributor Author

Part of me wonders whether we revise our approach to how we update the history in frame navigations?

We currently start a non-rendering Visit if the frame navigation has an action (added here: https://github.com/hotwired/turbo/pull/398/files#diff-8f44fafd854f6f716d51a2b05b5689266a9b66e0b1ad9d68570bd4e8cefd0a47R269)

This triggers a Visit lifecycle (including checking for tracked assets). To me a Visit is more of a "PageVisit", so I'm wondering if there's another way we could compose this behaviour?

afcapel added a commit that referenced this issue Jan 29, 2024
This commit changes the behavior of frame navigations promoted with the
`[data-turbo-action]` attribute so that they don't include the
`Turbo-Frame` header in their requests. This mimics the existing
behavior of frame navigations promoted with `[data-turbo-target=_top]`.

If we don't do this turbo-rails will see the `Turbo-Frame` header and
render the response as a frame update, with a minimal layout that doesn't
include any assets. When Turbo receives the response it believes it's a
full page response in which the assets have gone stale and it issues a full
page reload with `tracked_element_mismatch` as the reason.

Fixes #1047

This is similar to #1138
@afcapel
Copy link
Collaborator

afcapel commented Jan 29, 2024

This is caused because we're sending the Turbo-Frame header in promoted Turbo frame requests, that are intended for full page replacements. Frame navigations that are promoted with [data-turbo-target=_top] already don't include this header. I think navigations promoted with [data-turbo-action] should do the same.

PR in #1143

afcapel added a commit that referenced this issue Jan 29, 2024
Fixes #1047

Rendering a frame with a data-turbo-action was rendering the response
twice.

First, the `FrameController` in charge of the frame renders the response using a
FrameRenderer.

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L314

But when we update a frame with a data-turbo-action, we also issue a visit
to update the browser URL & history:

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L373

This session visit was also triggering a loadResponse callback, which
in turn was instantiating a `PageSnapshot` and rendering it.

This caused problems when the response for the frame didn't include all
the tracked assets in the page. The page snapshot considered the assets
stale and triggered a reload of the page with reason `tracked_element_mismatch`.

This commit fixes the issue by not rendering the page snapshot when
navigating within a frame.

The commit includes a new test that reproduces the issue returning a
turbo frame in an HTML document with an empty `<head>`. Before this fix,
the response would trigger a full page reload.
afcapel added a commit that referenced this issue Jan 29, 2024
Fixes #1047

Rendering a frame with a data-turbo-action was rendering the response
twice.

First, the `FrameController` in charge of the frame renders the response using a
FrameRenderer.

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L314

But when we update a frame with a data-turbo-action, we also issue a visit
to update the browser URL & history:

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L373

This session visit was also triggering a loadResponse callback, which
in turn was instantiating a `PageSnapshot` and rendering it.

This caused problems when the response for the frame didn't include all
the tracked assets in the page. The page snapshot considered the assets
stale and triggered a reload of the page with reason `tracked_element_mismatch`.

This commit fixes the issue by not rendering the page snapshot when
navigating within a frame.

The commit includes a new test that reproduces the issue returning a
turbo frame in an HTML document with an empty `<head>`. Before this fix,
the response would trigger a full page reload.
afcapel pushed a commit that referenced this issue Jan 30, 2024
* Do not render page snapshot when navigating within a frame

Fixes #1047

Rendering a frame with a data-turbo-action was rendering the response
twice.

First, the `FrameController` in charge of the frame renders the response using a
FrameRenderer.

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L314

But when we update a frame with a data-turbo-action, we also issue a visit
to update the browser URL & history:

https://github.com/hotwired/turbo/blob/b7187f13b216cee8c855ef1fb6ac790b40dde361/src/core/frames/frame_controller.js#L373

This session visit was also triggering a loadResponse callback, which
in turn was instantiating a `PageSnapshot` and rendering it.

This caused problems when the response for the frame didn't include all
the tracked assets in the page. The page snapshot considered the assets
stale and triggered a reload of the page with reason `tracked_element_mismatch`.

This commit fixes the issue by not rendering the page snapshot when
navigating within a frame.

The commit includes a new test that reproduces the issue returning a
turbo frame in an HTML document with an empty `<head>`. Before this fix,
the response would trigger a full page reload.

* Do not invalidate if the visit is not rendering

Skipping the rendering when the visit comes from a frame navigation
breaks a few tests related to caching.

Instead, let's make sure we don't invalidate the view if the visit is
not rendering.

* Add clarifying comment
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Feb 21, 2024
Re-submission of [hotwired#232][]
Related to [hotwired/turbo#1047][]

Render full documents, including default layout rendering behavior.

Rendering a minimal layout forces `turbo:reload` events because of the
severe difference in the contents of the minimal layout's `<head>` and
the requesting document's fully populated `<head>`.

[hotwired#232]: hotwired#232
[hotwired/turbo#1047]: hotwired/turbo#1047
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Feb 21, 2024
Re-submission of [hotwired#232][]
Related to [hotwired/turbo#1047][]

Render full documents, including default layout rendering behavior.

Rendering a minimal layout forces `turbo:reload` events because of the
severe difference in the contents of the minimal layout's `<head>` and
the requesting document's fully populated `<head>`.

[hotwired#232]: hotwired#232
[hotwired/turbo#1047]: hotwired/turbo#1047
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants