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

Adding data-turbo-action="advance" on top of a data-turbo-frame="target" causes a full page refresh #1156

Closed
krschacht opened this issue Feb 1, 2024 · 10 comments

Comments

@krschacht
Copy link
Contributor

I have a link which includes data-turbo-frame="target". This successfully causes only the turbo-frame to update. However, I notice the browser's URL does not update. In the documentation I found the solution to this: add data-turbo-action="advance" to the link to promote it to a visit:
https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit

This does indeed update the browser's URL, but it triggers a full page refresh instead of simply updating the turbo-frame. Is this the intended behavior? I don't think so. It doesn't make sense to me. There is a separate official way to get this behavior which is to change data-turbo-frame="_top", but I'm not doing that.

In fact, when I watch in chrome inspector I can see the get updated first and right after it does the full page gets updated.

Please tell me if I'm misunderstanding something or if I've discovered a bug. I love the promise of Turbo, but it's been driving me crazy trying to get my page layout to work as I intend.

@krschacht
Copy link
Contributor Author

I read through the discussion on the PR which added support for this and it seems like it's intended behavior is what I'm hoping for: #398

I think I've found a bug. Notably, I'm on Turbo 8 beta.4

@krschacht
Copy link
Contributor Author

@jeffdlange I was reading through an old discussion in which it seems like you are trying to solve this same issue, and you also used a combination of frame & action in order to do it: #792 (comment)

If you read this issue I just reported, am I misunderstanding something or should it work as I am intending?

@jeffdlange
Copy link

Hm, no, you're correct, using data-turbo-action="advance" should do exactly what you're describing. Here's the example setup:

<div>
  <h1>
    Page: <%= params[:page] %>
  </h1>

  <%= link_to "Outside link that targets frame", some_path, data: {turbo_frame: :some_content} %>

  <%= turbo_frame_tag :some_content, data: {turbo_action: :advance} do %>
    <%= pagination links here ... %>

   <div>Our page content, which updates as pagination links are clicked</div>
  <% end %>
</div>

We can click either:
a) The link outside the frame which targets the frame, or
b) The pagination links within the frame

In both cases, ALL of the following will take place:

  • the content of the turbo-frame will update
  • the content of the page outside the page will NOT update (i.e. the part of the page with <%= params[:page] %> will NOT change)
  • the URL will advance (e.g. "/some/content?page=2" -> "/some/content?page=3")
  • Clicking the back btn will react the way you would expect--the turbo-frame's content displays the prev page of content, while the rest of the page stays the same. The URL goes backwards from page 3 to 2.

That sounds like what you want, yes? But this isn't what you're experiencing? I can confirm all of this, having tested it just now.

@krschacht
Copy link
Contributor Author

krschacht commented Feb 4, 2024

@jeffdlange It's definitely not working as intended, unless the behavior you describe only works for query string params?

Meaning, rather than advancing from /some/content?page=2 to /some/content?page=3, shouldn't this also work to advance from /post/1 to /post/2?

I'm copying the exact page structure you outlined in your comment and that's the only thing that looks to be different. Yet the behavior I'm experiencing is:

  • The content of just the turbo-frame updates, but immediately after the full page refreshes
  • I do see the url change from /post/1 to /post/2 but because the full page refreshed then the scroll position and other page state outside the frame gets reset

Here is a video of the bug I'm describing: https://share.zight.com/2Nu90oO5

I implemented this in Jorge's Turbo 8 demo and pushed this branch in case you want to attempt it: https://github.com/krschacht/turbo-8-morphing-demo/tree/page-refresh-action-bug

@jeffdlange
Copy link

@krschacht Thanks for the thoroughness. I tried out your branch repo and I'm seeing the same results, including when I try slightly different approach (putting data-turbo-frame="project" on the sidebar links instead).

However, I've used this approach in other repos and it's worked as expected...suggesting we're missing something else in your setup here. I will post again if I find anything later

@sevos
Copy link

sevos commented Feb 6, 2024

I can confirm this behavior on turbo-rails 2.0.0.pre.beta.4.
First, I've checked if it persists on turbo-rails 1.5, and... it worked as expected (after disabling broadcasts & morphing).

Then I thought it might be related to <%= turbo_refreshes_with method: :morph, scroll: :preserve %> in the layout file, but commenting this out on 2.0.0.pre.beta.4 did not fix the issue.

To me, it looks like we have a regression in Turbo Frames between 1.5.0 and 2.0.0 (sorry, I am using turbo-rails versions here instead of the turbo version).

It feels like the logic enforcing the full page reload when URL changes runs when the link's target is a turbo frame, but this logic should be triggered only when the target is _top. I will look at the code later today.

@marckohlbrugge
Copy link

Same issue here on @hotwired/[email protected] with the turbo-rails (2.0.0) gem

@krschacht
Copy link
Contributor Author

Oh hey, this bug is fixed as of 8.0.2! It must have been this PR fro Sean that fixed it: #1135

When I upgraded to turbo-rails-2.0.2, my own project started working properly. I went ahead and updated the gem in this sample repo I linked to above in case anyone else needs to see a working implementation of it:
https://github.com/krschacht/turbo-8-morphing-demo/tree/page-refresh-action-bug

If you pull this repo, fire up the server, click on "Project 1", then scroll the left sidebar down a bit so it's scroll position is offset, as you click "Project 2", etc on the left side the scroll position is preserved since only the right turbo-frame is updating, but the URL updates and the back button history works!

@jeffdlange
Copy link

@krschacht Excellent! Thanks for the update

@sevos
Copy link

sevos commented Feb 16, 2024

How would we go about regression testing for this bug? Shall we try to implement something, or is it not important?

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

No branches or pull requests

4 participants