Turbo from not triggered when using submit button with form attribute #1246

oliverguenther · 2024-04-18T06:38:55Z

Given the following code:

<form id="my-form" action="/foo" method="POST" data-turbo="true">
# Some inputs here
</form>

<button type="submit" form="my-form">
Submit the form
</button>

I would have expected the form to be submitted using turbo, but it falls through (tested in Chrome, Firefox) probably because the submit button triggers form.submit() directly.

A workaround for that is a global event listener:

  document.addEventListener('submit', (event:SubmitEvent) => {
    if (event.submitter?.getAttribute('form')) {
      event.preventDefault();
      navigator.submitForm(event.target as HTMLFormElement, event.submitter);
    }
  });

I know that's a rather strange use case to have a submit button outside of a form, but at least it's part of the HTML5 spec (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button?retiredLocale=de#form), so I wonder if it would be useful for Turbo to have me provide support in a pull request for it.

airblade · 2024-04-18T10:55:10Z

If the button is outside the form, it needs a form attribute identifying the form it submits.

oliverguenther · 2024-04-18T11:29:38Z

If the button is outside the form, it needs a form attribute identifying the form it submits.

Sorry the form attribute was missing in the above example. My point is: Despite the form attribute being set, the form submit is not handled by Turbo.

seanpdoyle · 2024-04-18T14:11:01Z

@oliverguenther thank you for opening this issue.

I've tried to reproduce this in the Turbo test suite with the following changes:

diff --git a/src/tests/fixtures/form.html b/src/tests/fixtures/form.html
index aa46bcd..9026e4e 100644
--- a/src/tests/fixtures/form.html
+++ b/src/tests/fixtures/form.html
@@ -15,11 +15,12 @@
   <body>
     <h1>Form</h1>
     <div id="standard">
-      <form id="standard-form" action="/__turbo/redirect" method="post" class="redirect">
+      <form id="standard-form" action="/__turbo/redirect" method="post" class="redirect" data-turbo="true">
         <input type="hidden" name="path" value="/src/tests/fixtures/form.html">
         <input type="hidden" name="greeting" value="Hello from a redirect">
         <input id="standard-post-form-submit" type="submit" value="form[method=post]">
       </form>
+      <button id="standard-form-external-submit" form="standard-form">Submit #standard-form</button>
       <form action="/__turbo/redirect" method="get" class="redirect">
         <input type="hidden" name="path" value="/src/tests/fixtures/form.html">
         <input type="hidden" name="greeting" value="Hello from a redirect">
diff --git a/src/tests/functional/form_submission_tests.js b/src/tests/functional/form_submission_tests.js
index e744794..2a896be 100644
--- a/src/tests/functional/form_submission_tests.js
+++ b/src/tests/functional/form_submission_tests.js
@@ -66,6 +66,14 @@ test("form submission with confirmation cancelled", async ({ page }) => {
   assert.notOk(await formSubmitStarted(page))
 })
 
+test("form submission with external submitter", async ({ page }) => {
+  await page.click("#standard-form-external-submit")
+
+  await nextEventOnTarget(page, "standard-form", "turbo:before-fetch-request")
+  await nextEventOnTarget(page, "standard-form", "turbo:submit-start")
+  await nextEventNamed(page, "turbo:load")
+})
+
 test("form submission with secondary submitter click - confirmation confirmed", async ({ page }) => {
   page.on("dialog", (alert) => {
     assert.equal(alert.message(), "Are you really sure?")

Unfortunately, I cannot force the test to fail.

I've also tried to reproduce it in a single-file Rails application. I'll share the code below:

Save this file locally to `bug.rb`, then execute it with `ruby bug.rb`

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: [:create, :index]
  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 create
    @message = Message.create!(params.require(:message).permit(:body))

    redirect_to action: :index
  end

  def index
    @messages = Message.all

    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
    visit messages_path

    fill_in "Body", with: "Hello, world"
    click_button "Create Message"

    assert_text "Hello, world"
    assert_text "turbo:before-fetch-request"
    assert_text "turbo:submit-start"
    assert_text "turbo:load"
  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"

      function log(event) {
        document.getElementById("events").insertAdjacentHTML("beforeend", `<p>${event.type}</p>`)
      }

      addEventListener("turbo:before-fetch-request", log)
      addEventListener("turbo:submit-start", log)
      addEventListener("turbo:load", log)
    </script>
  </head>

  <body>
    <%= form_with model: Message.new, id: "new_message" do |form| %>
      <%= form.label :body %>
      <%= form.text_area :body %>
    <% end %>

    <button form="new_message">Create Message</button>
    <section id="events" data-turbo-permanent></section>
    <section>
      <% @messages.each do |message| %>
        <p><%= message.body %></p>
      <% end %>
    </section>
  </body>
</html>

Could you modify that reproduction script so that it more accurately reflects your scenario, then execute it with ruby bug.rb so that it fails the SystemTest assertions?

oliverguenther · 2024-04-22T11:21:37Z

Hi @seanpdoyle, thanks a lot for looking into this and create a repro. I should have done that to begin with - sorry.

The difference is that we're still migrating to Drive, so we have data-turbo="false"on the body. This is the bit that is missing to reproduce the issue. When setting data-turbo=true on the form, the test fails:

with data-turbo=true on form only

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: [:create, :index]
  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 create
    @message = Message.create!(params.require(:message).permit(:body))

    redirect_to action: :index
  end

  def index
    @messages = Message.all

    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
    visit messages_path

    fill_in "Body", with: "Hello, world"
    click_button "Create Message"

    assert_text "Hello, world"
    assert_text "turbo:before-fetch-request"
    assert_text "turbo:submit-start"
    assert_text "turbo:load"
  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"

      function log(event) {
        document.getElementById("events").insertAdjacentHTML("beforeend", `<p>${event.type}</p>`)
      }

      addEventListener("turbo:before-fetch-request", log)
      addEventListener("turbo:submit-start", log)
      addEventListener("turbo:load", log)
    </script>
  </head>

  <body data-turbo="false">
    <%= form_with model: Message.new, data: { turbo: true }, id: "new_message" do |form| %>
      <%= form.label :body %>
      <%= form.text_area :body %>
    <% end %>

    <button form="new_message">Create Message</button>
    <section id="events" data-turbo-permanent></section>
    <section>
      <% @messages.each do |message| %>
        <p><%= message.body %></p>
      <% end %>
    </section>
  </body>
</html>

With this minimal repro, I was able to step through the conditionals and found that the submitter also needs to have data-turbo="true", which makes sense but I simply missed this:

with data-turbo on submitter

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: [:create, :index]
  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 create
    @message = Message.create!(params.require(:message).permit(:body))

    redirect_to action: :index
  end

  def index
    @messages = Message.all

    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
    visit messages_path

    fill_in "Body", with: "Hello, world"
    click_button "Create Message"

    assert_text "Hello, world"
    assert_text "turbo:before-fetch-request"
    assert_text "turbo:submit-start"
    assert_text "turbo:load"
  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"

      function log(event) {
        document.getElementById("events").insertAdjacentHTML("beforeend", `<p>${event.type}</p>`)
      }

      addEventListener("turbo:before-fetch-request", log)
      addEventListener("turbo:submit-start", log)
      addEventListener("turbo:load", log)
    </script>
  </head>

  <body data-turbo="false">
    <%= form_with model: Message.new, data: { turbo: true }, id: "new_message" do |form| %>
      <%= form.label :body %>
      <%= form.text_area :body %>
    <% end %>

    <button data-turbo="true" form="new_message">Create Message</button>
    <section id="events" data-turbo-permanent></section>
    <section>
      <% @messages.each do |message| %>
        <p><%= message.body %></p>
      <% end %>
    </section>
  </body>
</html>

With this, I would assume this issue to be rejected. Would be accepting a PR that clarifies this in the docs for future reference in either the doc for data-turbo or the form parts? https://turbo.hotwired.dev/reference/attributes or https://turbo.hotwired.dev/reference/frames ?

seanpdoyle · 2024-04-22T15:58:44Z

@oliverguenther thank you for sharing more context.

Based on intuition alone, the scenario you've shared feels like it should work without button[data-turbo=true].

In the current implementation, the code is checking that the button submitter has an ancestor with [data-turbo="false"] without having a closer ancestor with [data-turbo="true"]. That's taking precedence over the associated form[data-turbo="true"], because it's conventional for the submitter annotations to have precedence over the form annotations.

I think this scenario should be exceptional, and require that the submitter be Turbo-enabled unless it's explicitly opted out with [data-turbo="false"] on the submitter itself.

If that makes sense, could you re-open this issue?

) When using a submitter with [form=id] attribute, it should respect the form's navigatable state unless the submitter explicitly opts out of turbo. Implements the expected behavior outlined in this comment: hotwired#1246 (comment)

oliverguenther · 2024-04-23T07:07:57Z

Thanks for the clarification. I opened a PR to implement what I understood is your expected behavior in #1251

If the submitter has a form attribute, the form's navigatable state should be used unless the submitter itself explicitly opts out of turbo.

) When using a submitter with [form=id] attribute, it should respect the form's navigatable state unless the submitter explicitly opts out of turbo. Implements the expected behavior outlined in this comment: hotwired#1246 (comment)

oliverguenther closed this as completed Apr 22, 2024

oliverguenther reopened this Apr 23, 2024

oliverguenther mentioned this issue Apr 23, 2024

Use navigatable associated form unless submitter opts out (#1246) #1251

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turbo from not triggered when using submit button with form attribute #1246

Turbo from not triggered when using submit button with form attribute #1246

oliverguenther commented Apr 18, 2024 •

edited

airblade commented Apr 18, 2024

oliverguenther commented Apr 18, 2024

seanpdoyle commented Apr 18, 2024

oliverguenther commented Apr 22, 2024 •

edited

seanpdoyle commented Apr 22, 2024

oliverguenther commented Apr 23, 2024

Turbo from not triggered when using submit button with form attribute #1246

Turbo from not triggered when using submit button with form attribute #1246

Comments

oliverguenther commented Apr 18, 2024 • edited

airblade commented Apr 18, 2024

oliverguenther commented Apr 18, 2024

seanpdoyle commented Apr 18, 2024

oliverguenther commented Apr 22, 2024 • edited

seanpdoyle commented Apr 22, 2024

oliverguenther commented Apr 23, 2024

oliverguenther commented Apr 18, 2024 •

edited

oliverguenther commented Apr 22, 2024 •

edited