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

Read FormSubmission.{method,location} from submitter #1

Merged
merged 1 commit into from
Dec 19, 2020

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Dec 14, 2020

Read FormSubmission.{method,location} from submitter

When a SubmitEvent is fired, it encodes the
element responsible for the submission. This element can be an <input type="submit">, a <button type="submit"> element, or the <form>
element itself (in the case of submissions initiated by
HTMLFormElement.requestSubmit() that omit the submitter argument).

According to the MDN documentation for the event.submitter
property
:

An element, indicating the element that sent the submit event to the
form. While this is often an <input> element whose type or a
<button> whose type is submit, it could be some other element which
has initiated a submission process.

If the submission was not triggered by a button of some kind, the
value of submitter is null.

To support submissions from elements other than the <form> that can
declare their own formmethod and
formaction, extend the FormSubmission object to
encode a reference to the submitter, and add an HTMLElement argument
to the FormSubmitObserver and FormSubmissionDelegate methods.

Invokes HTMLFormElement.method instead of
getAttribute("method") to defer gracefully handling missing value
fallbacks to the HTMLFormElement.method implementation.

Include Submitter in FormData

While constructing the FormData during a submission, attempt to read
the submitting <button> element's [name] and
[value] attributes, and encode them as part of the
submission.

While an <input type="submit"> element can have a
[name] and [value] attribute, the value is rendered as the
"button"'s text content.

Form Submitter polyfill

Extend the FormSubmitObserver event listening to track <button type="submit"> and <input type="submit"> clicks in Browsers that
have spotty support
.

The implementation is largely ported from both basecamp/turbolinks#4
and rails/rails#33413.

The FormSubmitter type definition is deliberately scoped to the
FormSubmitObserver module, since the Browser-native
SubmitEvent.submitter is only as specific as
HTMLElement
, so it's least disruptive to scope
limitations to the polyfilling logic.

@seanpdoyle seanpdoyle requested review from sstephenson and javan and removed request for sstephenson and javan December 14, 2020 17:10
@seanpdoyle seanpdoyle force-pushed the form-submitter branch 3 times, most recently from 76aa42e to 4af85e7 Compare December 18, 2020 17:14
Copy link
Contributor

@sstephenson sstephenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate all the thought that's gone into this change!

src/core/drive/form_submission.ts Outdated Show resolved Hide resolved
@@ -20,13 +20,13 @@ export class FormInterceptor {
this.element.removeEventListener("submit", this.submitBubbled)
}

submitBubbled = (event: Event) => {
submitBubbled = (event: Event & { submitter?: HTMLElement }) => {
Copy link
Contributor

@sstephenson sstephenson Dec 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When TypeScript eventually gets a SubmitEvent definition, I think it'll look more like this:

interface SubmitEvent extends Event {
  submitter: HTMLElement | null
}

since the spec explicitly says submitter might be null. But I agree that HTMLElement | null is a lot less nice to use in method signatures than optional argument syntax.

I think what I'd suggest here is adding the SubmitEvent definition above to globals.d.ts, and then defining the event handler like this:

-  submitBubbled = (event: Event & { submitter?: HTMLElement }) => {
+  submitBubbled = <EventListener>((event: SubmitEvent) => {
     if (event.target instanceof HTMLFormElement) {
       const form = event.target
-      if (this.delegate.shouldInterceptFormSubmission(form, event.submitter)) {
+      const submitter = event.submitter || undefined
+      if (this.delegate.shouldInterceptFormSubmission(form, submitter)) {
         event.preventDefault()
         event.stopImmediatePropagation()
-        this.delegate.formSubmissionIntercepted(form, event.submitter)
+        this.delegate.formSubmissionIntercepted(form, submitter)
       }
     }
-  }
+  })

The explicit cast to EventListener allows it to be passed to addEventListener and removeEventListener without any other declarations.

src/polyfills/submit-event.ts Outdated Show resolved Hide resolved
When a [`SubmitEvent` is fired][mdn-submit-event], it encodes the
element responsible for the submission. This element can be an `<input
type="submit">`, a `<button type="submit">` element, or the `<form>`
element itself (in the case of submissions initiated by
`HTMLFormElement.requestSubmit()` that omit the `submitter` argument).

According to the [MDN documentation for the `event.submitter`
property][mdn-submitter]:

> An element, indicating the element that sent the submit event to the
> form. While this is often an `<input>` element whose type or a
> `<button>` whose type is `submit`, it could be some other element which
> has initiated a submission process.

> If the submission was not triggered by a button of some kind, the
> value of `submitter` is `null`.

To support submissions from elements other than the `<form>` that can
declare their own [`formmethod`][mdn-formmethod] and
[`formaction`][mdn-formaction], extend the `FormSubmission` object to
encode a reference to the submitter, and add an `HTMLElement` argument
to the `FormSubmitObserver` and `FormSubmissionDelegate` methods.

Invokes [HTMLFormElement.method][mdn-method] instead of
`getAttribute("method")` to defer gracefully handling missing value
fallbacks to the [HTMLFormElement.method][mdn-method] implementation.

[mdn-request-submit]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/requestSubmit#Parameters
[mdn-submit-event]: https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent
[mdn-submitter]: https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent/submitter
[mdn-formmethod]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-formmethod
[mdn-formaction]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-formaction
[mdn-method]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/method

Include Submitter in FormData
===

While constructing the `FormData` during a submission, attempt to read
the submitting `<button>` element's [`[name]`][button-name] and
[`[value]`][button-value] attributes, and encode them as part of the
submission.

While an [`<input type="submit">` element][input-submit] can have a
`[name]` and `[value]` attribute, the `value` is rendered as the
"button"'s text content.

[form-data]: https://developer.mozilla.org/en-US/docs/Web/API/FormData
[button-name]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-name
[button-value]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#attr-value
[input-submit]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/submit

Form Submitter polyfill
===

Extend the `FormSubmitObserver` event listening to track `<button
type="submit">` and `<input type="submit">` clicks [in Browsers that
have spotty support][support].

The implementation is largely ported from both [basecamp/turbolinks#4][]
and [rails/rails#33413][].

The `FormSubmitter` type definition is deliberately scoped to the
`FormSubmitObserver` module, since the [Browser-native
`SubmitEvent.submitter` is only as specific as
`HTMLElement`][SubmitEvent], so it's least disruptive to scope
limitations to the polyfilling logic.

[support]: https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent/submitter#Browser_compatibility
[basecamp/turbolinks#4]: https://github.com/basecamp/turbolinks/pull/4
[rails/rails#33413]: rails/rails#33413
[SubmitEvent]: https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent#Properties
@sstephenson sstephenson merged commit 1dd7a80 into main Dec 19, 2020
@sstephenson sstephenson deleted the form-submitter branch December 19, 2020 13:58
function findSubmitterFromClickTarget(target: EventTarget | null): FormSubmitter | null {
const element = target instanceof Element ? target : target instanceof Node ? target.parentElement : null
const candidate = element ? element.closest("input, button") as FormSubmitter | null : null
return candidate?.getAttribute("type") == "submit" ? candidate : null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use the type property here to handle implicit submit <button>s:

<form><button></button></form>
> button.getAttribute("type")
null

> button.type
"submit"

Copy link
Contributor Author

@seanpdoyle seanpdoyle Dec 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javan this is a great call.

From what I remember, TypeScript prevented type property access:

src/polyfills/submit-event.ts|8 col 21 error| 2339[QF available]: Property 'type' does not exist on type 'FormSubmitter'

FormSubmitter is defined within the module:

type FormSubmitter = HTMLElement & { form?: HTMLFormElement }

Would adding type?: string be correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sstephenson's probably best equipped to answer that

Copy link

@jalada jalada Dec 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got tripped up by this today. In Safari, where this polyfill applies, my form was submitting to its action instead of the formaction defined on the button being clicked because I was missing type='submit' on the button. In Chrome, I didn't need to define that type because the polyfill didn't apply. Once I added type='submit', Safari was happy 🎉 .

This feels like a bug? Happy to open a separate issue if this needs to be tracked separately, or if there's something else I can do to help, please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened #49 attempt to resolve this.

seanpdoyle added a commit that referenced this pull request Dec 28, 2020
Follow-up to #1 (comment)

Replaces the `SubmitEvent` polyfill's `type` attribute access with
[`type property access][].

[type]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLButtonElement#Properties
domchristie pushed a commit to domchristie/turbo that referenced this pull request Jan 3, 2021
Follow-up to hotwired#1 (comment)

Replaces the `SubmitEvent` polyfill's `type` attribute access with
[`type property access][].

[type]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLButtonElement#Properties
@AndKenneth AndKenneth mentioned this pull request Sep 6, 2023
brunoprietog pushed a commit that referenced this pull request Jun 21, 2024
The scenario
---

Imagine a List-Details style page layout, with navigation links on the
left and the contents of the page on the right:

```html
<turbo-frame id="list">
  <a href="/articles/1" data-turbo-frame="details">
    Article #1

    <turbo-frame id="preview_article_1" src="/articles/1/preview">
      Some preview text
    </turbo-frame>
  </a>
  <!-- ... -->

  <a href="/?page=2">Next Page</a>
</turbo-frame>

<turbo-frame id="details">
  <h1>Details</h1>
</turbo-frame>
```

The `#list` element is a `<turbo-frame>` to handle pagination, and the
`#details` element is a `<turbo-frame>` as well. The `<a>` elements
within the `#list` frame drive the `#details` frame through their
`[data-turbo-frame="details"]`. The `<a>` element nests a third kind of
`<turbo-frame>` that is specific to the resource being linked to. It
asynchronously loads in preview content with a `[src]` attribute.

The problem
---

Clicking the `Article #1` text within the `<a>` element drives the
`#details` frame as expected.

However, clicking the `Some preview text` within the `<a>` element's
nested `<turbo-frame>` element navigates the entire page.

This is demonstrated in the following reproduction script:

```ruby
require "bundler/inline"

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

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  gem "propshaft"
  gem "puma"
  gem "sqlite3"
  gem "turbo-rails"

  gem "capybara"
  gem "cuprite", "~> 0.9", require: "capybara/cuprite"
end

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

require "active_record/railtie"
require "action_controller/railtie"
require "action_view/railtie"
require "action_cable/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"
  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 ApplicationController < ActionController::Base
  include Rails.application.routes.url_helpers

  class_attribute :template, default: DATA.read

  def index
    render inline: template, formats: :html
  end
end

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: {
    js_errors: true,
    headless: false
  }
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 root_path

    binding.irb
    click_link "Drive #details to ?key=1"
  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>

    <style>
      body {
        display: grid;
        grid-template-areas:
          "list details"
          "list details";
        grid-template-columns: 150px 1fr;
        grid-template-rows: 50px 1fr;
        height: 100vh;
      }

      #list {
        display: flex;
        flex-direction: column;
        grid-area: list;
        overflow-y: scroll;
      }

      #details {
        grid-area: details;
      }
    </style>
    <meta name="turbo-prefetch" content="false">
  </head>

  <body>
    <turbo-frame id="list">
      <% 1.upto(5).each do |key| %>
        <%= link_to({key:}, data: {turbo_frame: "details"}) do %>
          <turbo-frame id="frame_<%= key %>">
            Drive #details to <%= {key:}.to_query %>
          </turbo-frame>
        <% end %>
      <% end %>
    </turbo-frame>

    <turbo-frame id="details">
      <%= params.fetch(:key, "0") %>
    </turbo-frame>
  </body>
</html>
```

The solution
---

When observing `click` events, utilize the `findLinkFromClickTarget` to
find the nearest `<a>` element to the `click` target so that **that**
element's ancestors are used to determine which `<turbo-frame>` to
target instead of risking the possibility of using one of its
**descendants**.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants