Drive turbo-frame with Turbo.visit(url, { frame:, action: })

[seanpdoyle]

Closes #489
Closes #468

First, add test coverage to exercise the <turbo-frame id="frame"> and
<turbo-frame id="hello" target="frame"> as outlined in a
comment on #489.

Next, add coverage to support driving a <turbo-frame data-turbo-action="advance"> through the Turbo.visit method. To pass
that test coverage, invoke
frameElement.delegate.proposeVisitIfNavigatedWithAction(frameElement)
prior to modifying the element's [src] attribute.

To support the implementation changes necessary to pass the tests, this
commit changes the proposeVisitIfNavigatedWithAction(frame, action)
interface by flattening the variable arguments into a single action
argument, then moving the getVisitAction call to the call sites that
require it. The call sites that know their action pass it in directly
when its available.

[alexspeller]

@seanpdoyle I made this PR into your branch with a failing test that should, per my understanding of the docs, be working.

It's extremely similar except that instead of adding data-turbo-action=advance to the frame, it's passing in {action: 'advance'} option to Turbo.visit - I think that this should work also, both firing a turbo:load event and updating the page url.

Am I correct in thinking that the test I've written should pass?

seanpdoyle#2

[seanpdoyle]

@alexspeller thank you for reviewing the test suite changes. I've incorporated them into this PR.

[alexspeller]

No problem - btw I think this is either the same issue as or very closely related to #468

[seanpdoyle]

@afcapel could you review these changes?

[afcapel]

Looks good! Thanks @seanpdoyle 🙏

[michelson]

@seanpdoyle @afcapel, was this fix included in the 8.0.4 release? I'm trying it, but it does not work. It does not replace the URL when a frame + advance is in the mix.

Turbo.visit("/conversations/1044", { frame: "chat-conversation", action: "advance" })
Also with links,

<%= link_to chat["name"], 
      "/conversations/1044"", 
      data: { turbo_frame: "chat-conversation", turbo_action: "advance" } 
    %>

[adampal]

@seanpdoyle @afcapel, was this fix included in the 8.0.4 release? I'm trying it, but it does not work. It does not replace the URL when a frame + advance is in the mix.

Turbo.visit("/conversations/1044", { frame: "chat-conversation", action: "advance" }) Also with links,

<%= link_to chat["name"], 
      "/conversations/1044"", 
      data: { turbo_frame: "chat-conversation", turbo_action: "advance" } 
    %>

According to the change log, this was introduced in 8.0.0 but I'm having the same issue currently on 8.0.3
I'll try updating to 8.0.4 to see if it fixes the issue.

Update: Updating to 8.0.4 did not fix the issue for me.

[seanpdoyle]

@michelson @adampal I'm unable to reproduce the issues you're describing. The test coverage within Turbo's test suite that exercises this feature is passing.

I tried to reproduce the issues with a single-file Rails application. I'll share the source below. Could you save it locally as bug.rb, then execute it with ruby bug.rb? Once you have it executing and passing, could you modify it to more closely reflect your scenarios?

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
    resources :messages, only: :show
  end
end

Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :messages, force: true do |t|
    t.text :body, null: false
  end
end

class Message < ActiveRecord::Base
end

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

  class_attribute :template, default: DATA.read

  def show
    @message = Message.find(params[:id])

    render inline: template, formats: :html
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {js_errors: 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 "reproduces bug" do
    first, second = Message.create! [{body: "First"}, {body: "Second"}]

    visit message_path(first)

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "First"
    assert_equal message_url(first), current_url

    turbo_visit message_path(second), frame: "message", action: "advance"

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "Second"
    assert_equal message_url(second), current_url
  end

  def turbo_visit(...)
    execute_script(<<~JS, ...)
      Turbo.visit(...arguments)
    JS
  end
end

__END__
<!DOCTYPE html>
<html>
  <head>
    <%= csrf_meta_tags %>

    <script type="importmap">
      {
        "imports": {
          "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
        }
      }
    </script>

    <script type="module">
      import "@hotwired/turbo-rails"
    </script>
  </head>

  <body>
    <h1><%= @message.body %></h1>
    <turbo-frame id="message"><%= @message.body %></turbo-frame>
  </body>
</html>

[michelson]

Hi @seanpdoyle , I've looked into the test. Thanks for providing this; running Rails in a single file is fantastic!
I've found that the test runs too fast that it passes the check for the outer value, if you put some sleep just before this check it will fail because the whole body gets refreshed:

  sleep(2)
   assert_css "h1", text: "First". # <- will fail here
   assert_css "turbo-frame", text: "Second"
   assert_equal message_url(second), current_url

Screen.Cast.2024-04-18.at.8.25.42.PM.mp4

[michelson]

Discard the previous message,

I see that the test is trying to navigate to messages, but we have id="message" That is why it refreshed the whole page,

turbo_visit message_path(second), frame: "message", action: "advance" actually pass the test correctly

[seanpdoyle]

@michelson thank you for following up. I apologize for that [id="message"] typo, I hope you didn't waste too much time troubleshooting it.

Since you've resolved that issue in the reproduction script, were you also able to reproduce the issue you're experiencing in your application?

[michelson]

Hi Sean, I've found my issue. I was rendering with the following:

render turbo_stream: [
         turbo_stream.replace(
           "chat-conversation",
           partial: "chat",
           locals: {
             chat_id: params[:id],
             messages: @messages
           }
         )
] and return

that didn't work,

now, if I do render "show" with the proper <%= turbo_frame_tag "chat-conversation", ` then it works.

I hope you didn't waste too much time troubleshooting it.

On the contrary, TIL how to run Rails in a single file 🥇

[adampal]

@seanpdoyle thanks for providing the one file test script. It was really helpful!

I was able to create a failing test case using it. Basically, my issue is that turbo-frame-action attributes are not respected by forms within a frame.

If you put the attribute on the form itself, everything works as expected. But when the attribute is on the turbo-frame and you have a form within that frame, it doesn't work.

I updated the one file test you created to show the issue. Sorry it's a bit hacky because I wasn't sure how to create different views within the one file to show the pass/failure states.

I also noticed that the data-turbo-frame="message" attribute also doesn't seem to be respected so the test actually fails on the line:
assert_css "turbo-frame", text: "Second"

But if you comment out that line, you can see that the URL hasn't been updated and the data-turbo-action attribute didn't work as expected.

Here's my updated single file test showing the issue:

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
    resources :messages, only: :show
  end
end

Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :messages, force: true do |t|
    t.text :body, null: false
  end
end

class Message < ActiveRecord::Base
end

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

  class_attribute :template, default: DATA.read

  def show
    @message = Message.find(params[:search] || params[:id])
    @other_message = Message.where.not(id: @message.id).first

    render inline: template, formats: :html
  end
end

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

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

require "rails/test_help"

class TurboSystemTest < ApplicationSystemTestCase
  # This test fails
  test "turbo-frame-action on turbo-frame element" do
    first, second = Message.create! [{body: "First"}, {body: "Second"}]

    visit message_path(first, frame: true)

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "First"
    assert_equal message_url(first, frame: true), current_url

    click_on "click me"

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "Second"
    assert_equal message_url(first, search: second.id), current_url
  end

  # this test passes
  test "turbo-frame-action on form element" do
    first, second = Message.create! [{body: "First"}, {body: "Second"}]

    visit message_path(first)

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "First"
    assert_equal message_url(first), current_url

    click_on "click me"

    assert_css "h1", text: "First"
    assert_css "turbo-frame", text: "Second"
    assert_equal message_url(first, search: second.id), current_url
  end

  def turbo_visit(...)
    execute_script(<<~JS, ...)
      Turbo.visit(...arguments)
    JS
  end
end

__END__
<!DOCTYPE html>
<html>
  <head>
    <%= csrf_meta_tags %>

    <script type="importmap">
      {
        "imports": {
          "@hotwired/turbo-rails": "<%= asset_path("turbo.js") %>"
        }
      }
    </script>

    <script type="module">
      import "@hotwired/turbo-rails"
    </script>
  </head>

  <body>
    <h1><%= @message.body %></h1>
    <% if params[:frame].present? %>
      <%# The form within this frame won't trigger the advance action %>
      <turbo-frame id="search-holder" data-turbo-frame="message" data-turbo-action="advance">
        <form action="" method="GET">
          <input type="text" name="search" value="<%= @other_message.id %>">
          <input type="submit" value="click me">
        </form>
      </turbo-frame>
    <% else %>
      <turbo-frame id="search-holder">
        <%# This form will work as expected and trigger the advance action %>
        <form action="" method="GET"  data-turbo-frame="message" data-turbo-action="advance">
          <input type="text" name="search" value="<%= @other_message.id %>">
          <input type="submit" value="click me">
        </form>
      </turbo-frame>
    <% end %>
    <turbo-frame id="message"><%= @message.body %></turbo-frame>
  </body>
</html>