Back button is not working for turbo advance targeting a frame when rendered inside rails

[krschacht]

I have a really hard bug that I've finally stripped down to an essential repro of it. I can't figure out what's going on.

What the user experiences on my app is:

1. Click a link which targets a turbo-frame on the page with action="advance"
2. The contents of the turbo-frame updates and the URL successfully changes
3. Clicking the back button changes the URL back to what it was before, but it does not revert the contents

It's a bad bug because it feels like the back button is broken, which, of course, we want turbo to respect back behavior.

In order to file a bug report, I tried to create a failing test case within this hotwired/turbo.js project. I opened src/tests/fixtures/frames.html and set up the test situation did the usual yarn build; yarn start. Oddly, the back button worked properly. I could not initially repro it.

Puzzled, I copied the exact same contents of frames.html over to my rails project and the bug appears! I could not figure out what the difference is since I stripped everything down. The only difference is that one is being served by yarn server and one is being served by rails server and rendering view templates. Then I copied these same files into the rails public/ directory and the bug goes away! For some reason this bug is only present when html & javascript code is served up through rails views. I'm baffled.

I know this is confusing so try watching this video where I show it very simply: https://share.zight.com/o0uAplQJ

And here is a repository with of a fresh rails app with just the 4 files needed to reproduce it, everything else has been stripped out. Pull this repo and you can try for yourself what I just demonstrated in the video: https://github.com/krschacht/hotwire-demo

[4lllex]

When you render through a rails controller, turbo frame requests are rendered within a "turbo_rails/frame" layout. The problem is that this layout has an empty head tag which creates an incorrect snapshot. I've noticed turbo:before-render and turbo:render events do not fire during this frame navigation and they should. If you trace it a little bit back it comes down to this comparison:

https://github.com/hotwired/turbo/blob/f88bfe45ed44abb6ea9140a8ba138dfe68aa2730/src/core/drive/page_renderer.js#L92-L94

This also seems to only happen on initial page load, if you navigate to the test page with turbo first and then click the link everything works fine.

If this is something you need to fix now, you could force to always render layout layout "applicaiton" in a controller.

[4lllex]

I've made a rails test script. I've also noticed that if you remove data-turbo-track="reload" there is no bug:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  gem "propshaft"
  gem "puma"
  gem "sqlite3", "~> 1.4"
  gem "turbo-rails"

  gem "capybara"
  gem "cuprite", require: "capybara/cuprite"
end

ENV["DATABASE_URL"] = "sqlite3::memory:"
ENV["RAILS_ENV"] = "test"

require "active_record/railtie"
# require "active_storage/engine"
require "action_controller/railtie"
require "action_view/railtie"
# require "action_mailer/railtie"
# require "active_job/railtie"
require "action_cable/engine"
# require "action_mailbox/engine"
# require "action_text/engine"
require "rails/test_unit/railtie"

class App < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f

  config.root = __dir__
  config.hosts << "example.org"
  config.eager_load = false
  config.session_store :cookie_store, key: "cookie_store_key"
  config.secret_key_base = "secret_key_base"
  config.consider_all_requests_local = true
  config.action_cable.cable = {"adapter" => "async"}
  config.turbo.draw_routes = false

  Rails.logger = config.logger = Logger.new($stdout)

  routes.append do
    root to: "application#index"
    get "frame", to: "application#frame"
  end
end

Rails.application.initialize!

class ApplicationController < ActionController::Base
  include Rails.application.routes.url_helpers

  def index
    render inline: <<~TEMPLATE, formats: :html
      <html>
        <head>
          <%= csrf_meta_tags %>
          <script type="importmap" data-turbo-track="reload">
            { "imports": { "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>" } }
          </script>
          <script type="module">
            import "@hotwired/turbo-rails"
          </script>
        </head>
        <body>
          <turbo-frame id="my-frame"><div>initial frame</div></turbo-frame>
          <a id="frame-link" href="/frame" data-turbo-frame="my-frame" data-turbo-action="advance">click</a>
        </body>
      </html>
    TEMPLATE
  end

  def frame
    # not sure how to stuff layouts into this bug template
    # simulate how a turbo_rails/frame layout is rendered on a turbo frame request
    if turbo_frame_request?
      render inline: <<~TEMPLATE, formats: :html
        <html>
          <head></head>
          <body>
            <turbo-frame id="my-frame"><div>frame loaded</div></turbo-frame>
            <a id="home-link" href="/">home</a>
          </body>
        </html>
      TEMPLATE
    else
      render inline: <<~TEMPLATE, formats: :html
        <html>
          <head>
            <%= csrf_meta_tags %>
            <script type="importmap" data-turbo-track="reload">
              { "imports": { "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>" } }
            </script>
            <script type="module">
              import "@hotwired/turbo-rails"
            </script>
          </head>
          <body>
            <turbo-frame id="my-frame"><div>frame loaded</div></turbo-frame>
            <a id="home-link" href="/">home</a>
          </body>
        </html>
      TEMPLATE
    end
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :cuprite,
    using: :chrome,
    screen_size: [1400, 1400],
    options: {
      js_errors: true,
      headless: true
    }
end

Capybara.configure do |config|
  config.server = :puma, {Silent: true}
  config.default_normalize_ws = true
end

require "rails/test_help"

class TurboSystemTest < ApplicationSystemTestCase
  test "not a bug" do
    visit "/frame"
    click_link "home-link"
    assert_text "initial frame"
    click_link "frame-link"
    sleep 1
    assert_text "frame loaded"
    go_back
    sleep 1
    assert_text "initial frame"
  end

  test "a bug" do
    visit "/"
    click_link "frame-link"
    sleep 1
    assert_text "frame loaded"
    go_back
    sleep 1
    assert_text "initial frame" # bug
  end
end

[krschacht]

@4lllex That's great that you have a repro and even a hunch about the core comparison that is failing. How hard do you think it would be to tee up a PR for this one? It'd be nice to get it in for Rails 8

[4lllex]

1241 looks like the same issue, they have a PR attached but I'm not sure how close it is to an accepted solution.

[4lllex]

The fix seems to be known and it is to revert this line:

https://github.com/hotwired/turbo/blob/7ae954621ddfb36ecac96fda758a3f1d9cae065d/src/core/frames/frame_controller.js#L352

Here is a patch in the meantime:

Turbo.FrameElement.delegateConstructor.prototype.proposeVisitIfNavigatedWithAction = function(frame, action = null) {
  this.action = action

  if (this.action) {
    // const pageSnapshot = PageSnapshot.fromElement(frame).clone()
    const pageSnapshot = Turbo.PageSnapshot.fromElement(frame).clone()
    const { visitCachedSnapshot } = frame.delegate

    // frame.delegate.fetchResponseLoaded = async (fetchResponse) => {
    frame.delegate.fetchResponseLoaded = (fetchResponse) => {
      if (frame.src) {
        const { statusCode, redirected } = fetchResponse
        // const responseHTML = await fetchResponse.responseHTML
        const responseHTML = frame.ownerDocument.documentElement.outerHTML
        const response = { statusCode, redirected, responseHTML }
        const options = {
          response,
          visitCachedSnapshot,
          willRender: false,
          updateHistory: false,
          restorationIdentifier: this.restorationIdentifier,
          snapshot: pageSnapshot
        }

        if (this.action) options.action = this.action

        // session.visit(frame.src, options)
        Turbo.session.visit(frame.src, options)
      }
    }
  }
}

[oliverguenther]

Can confirm that your patch is working, @4lllex . Thanks a lot! We were trying the data-turbo-track=reload head workaround, but with this patch an empty head (or rather, no head at all as mentioned in #1237)