Support multiple submit buttons in Active Storage forms by cseelus · Pull Request #33413 · rails/rails

cseelus · 2018-07-22T00:01:45Z

Often times forms have more than one submit button enabling different actions, there are various ways to use this with Rails.

Currently activestorage.js takes a somewhat naive approach in just using the first input[type=submit] inside a form to click() and submit the form after direct uploads.

This PR utilizes the name and value from form._ujsData to use the input the User has really clicked, when submitting the form after an upload.

I'm not too happy with the code myself, so this is more of a question if Rails should even support multiple submit buttons per form or not. If so, it might be possible for me to find a cleaner approach.

Edit: When this fix is finshed, it might be better if I backport it to 5-2-stable before.

rails-bot · 2018-07-22T00:01:48Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

javan · 2018-07-27T20:16:52Z

activestorage.js doesn't depend on rails-ujs so we can't rely on help from it. Even if we could, those "private" expando properties aren't meant to be accessed directly.

Maybe there's an easier way to track which submit button was clicked? Something like:

let button = findElement(form, "input[type=submit]:active, input[type=submit]")

(untested!)

Yes, thats understandable.

cseelus · 2018-08-04T19:02:20Z

Not being able to get the button that actually submitted a form, is a known shortcoming of the form being the event.target. With the exception of Firefox, which implements Event.originalTarget, its not really straight forward to get the button that was actually clicked when a form was sumitted.

Unfortunately its also not as simple as using

let button = findElement(form, "input[type=submit]:active, input[type=submit]")

I made it work another way (see latest commit). Guess that turbolinks:load event listener will have to go before this can be merged, but is the general approach mergeable?

javan · 2018-08-12T15:54:46Z

I made it work another way (see latest commit).

Can you commit your source changes to activestorage/app/javascript/* too, please? They're out of sync with the compiled activestorage.js file currently.

cseelus · 2018-08-12T16:10:41Z

Can you commit your source changes to activestorage/app/javascript/* too, please?

Added the changes there too.

javan · 2018-08-12T16:25:21Z

We can't rely Turbolinks events since not all apps use it. I'd add a new click listener here:

rails/activestorage/app/javascript/activestorage/ujs.js

Lines 10 to 11 in ba1dab1

document.addEventListener("submit", didSubmitForm)

document.addEventListener("ajax:before", didSubmitRemoteElement)

The handler can check if the event.target is a submit button and record the last clicked button in a WeakMap using the button's form as the key.

cseelus · 2018-08-12T19:56:13Z

Guess that turbolinks:load event listener will have to go before this can be merged, but is the general approach mergeable?

My initial approach was to add the event listener into the start() method (see latest commit) as you requested in your latest review. But this won't work with Turbolinks enabled. Maybe you have a hint how such things are usually handled in the Rails source, so I can adapt the code.

javan · 2018-08-13T12:44:36Z

This won't handle clicks on dynamically inserted elements (ajax loaded content, Turbolinks page change, etc).

javan · 2018-08-13T12:46:18Z

I think this will work, and avoids the need to handle click events / annotating the submit buttons:

--- a/activestorage/app/javascript/activestorage/ujs.js
+++ b/activestorage/app/javascript/activestorage/ujs.js
@@ -30,6 +30,7 @@ function handleFormSubmissionEvent(event) {
     return
   }
 
+  const button = findActiveSubmitButton(form)
   const controller = new DirectUploadsController(form)
   const { inputs } = controller
 
@@ -42,14 +43,14 @@ function handleFormSubmissionEvent(event) {
       if (error) {
         inputs.forEach(enable)
       } else {
-        submitForm(form)
+        submitForm(form, button)
       }
     })
   }
 }
 
-function submitForm(form) {
-  let button = findElement(form, "input[type=submit]")
+function submitForm(form, button) {
+  button = button || findElement(form, "input[type=submit]")
   if (button) {
     const { disabled } = button
     button.disabled = false
@@ -73,3 +74,10 @@ function disable(input) {
 function enable(input) {
   input.disabled = false
 }
+
+function findActiveSubmitButton(form) {
+  const element = document.activeElement
+  if (element && element.type == "submit" && element.form == form) {
+    return element
+  }
+}

Want to give that a try?

cseelus · 2018-08-13T23:13:59Z

Want to give that a try?

Of course :-)

This was one of the first things I tried. Problem is, document.activeElement is the form, not the submit button in some browsers.

document.activeElement returns the element, which currently has the focus. And in iOS Safari button never gets the focus, neither in Chrome for iOS. FireFox/Mac and Safari/Mac follow the same pattern.

-- https://sarbbottam.github.io/blog/2015/08/21/multiple-submit-buttons-and-javascript

Edit: Again, unfortunately this is not as straight forward as it could be. Should I make the "annotating the clicked submit button and adding click event listeners" version compatible with Turbolinks enabled+disabled?

javan · 2018-08-17T15:02:11Z

Problem is, document.activeElement is the form, not the submit button in some browsers.

Bummer! Here's more-or-less what I was thinking for handling clicks and recording submit buttons:

--- a/activestorage/app/javascript/activestorage/ujs.js
+++ b/activestorage/app/javascript/activestorage/ujs.js
@@ -2,16 +2,25 @@ import { DirectUploadsController } from "./direct_uploads_controller"
 import { findElement } from "./helpers"
 
 const processingAttribute = "data-direct-uploads-processing"
+const submitButtonsByForm = new WeakMap
 let started = false
 
 export function start() {
   if (!started) {
     started = true
+    document.addEventListener("click", didClick, true)
     document.addEventListener("submit", didSubmitForm)
     document.addEventListener("ajax:before", didSubmitRemoteElement)
   }
 }
 
+function didClick(event) {
+  const { target } = event
+  if (target.tagName == "INPUT" && target.type == "submit" && target.form) {
+    submitButtonsByForm.set(target.form, target)
+  }
+}
+
 function didSubmitForm(event) {
   handleFormSubmissionEvent(event)
 }
@@ -49,7 +58,7 @@ function handleFormSubmissionEvent(event) {
 }
 
 function submitForm(form) {
-  let button = findElement(form, "input[type=submit]")
+  let button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]")
   if (button) {
     const { disabled } = button
     button.disabled = false
@@ -64,6 +73,7 @@ function submitForm(form) {
     button.click()
     form.removeChild(button)
   }
+  submitButtonsByForm.delete(form)
 }

cseelus · 2018-08-18T01:34:44Z

Also good utilization of useCapture. I see no reason why this shouldn't work, will try out.

cseelus · 2018-08-18T01:45:46Z

Works as expected. Thanks for taking the time showing me some new JS stuff ✌️

Should I base this off 5-2-stable and rename the branch?

javan · 2018-08-18T16:12:08Z

Should I base this off 5-2-stable and rename the branch?

No, PRs should always be opened against master. I'll backport to 5-2 after merging.

Mind squashing your commits?

cseelus · 2018-08-18T16:26:58Z

Done

javan · 2018-08-18T20:04:15Z

Thank you!

I'll backport this to 5-2-stable as soon as I get baf3d98#commitcomment-30164614 sorted out.

…multiple-submit-buttons Support multiple submit buttons in Active Storage forms

javan · 2018-08-19T15:20:42Z

Backported to 5-2-stable: 388a1f3

bogdanvlviv · 2018-08-19T16:05:45Z

rails/activestorage$ yarn build

produces a diff:

diff --git a/activestorage/app/assets/javascripts/activestorage.js b/activestorage/app/assets/javascripts/activestorage.js
index 8a51805960..375eb6b533 100644
--- a/activestorage/app/assets/javascripts/activestorage.js
+++ b/activestorage/app/assets/javascripts/activestorage.js
@@ -855,7 +855,7 @@
     return DirectUploadsController;
   }();
   var processingAttribute = "data-direct-uploads-processing";
-  var submitButtonsByForm = new WeakMap;
+  var submitButtonsByForm = new WeakMap();
   var started = false;
   function start() {
     if (!started) {
@@ -866,8 +866,9 @@
     }
   }
   function didClick(event) {
-    if (event.target.tagName == "INPUT" && event.target.type == "submit" && event.target.form) {
-      submitButtonsByForm.set(event.target.form, event.target);
+    var target = event.target;
+    if (target.tagName == "INPUT" && target.type == "submit" && target.form) {
+      submitButtonsByForm.set(target.form, target);
     }
   }
   function didSubmitForm(event) {
@@ -902,7 +903,6 @@
   }
   function submitForm(form) {
     var button = submitButtonsByForm.get(form) || findElement(form, "input[type=submit]");
-
     if (button) {
       var _button = button, disabled = _button.disabled;
       button.disabled = false;

Not sure whether it is problem in my local environment. If no, I think we should apply this diff to master and 5-2-stable in order to omit these changes during the next rebuild?

javan · 2018-08-19T16:28:34Z

Good catch, @bogdanvlviv. I see the same so your local environment is correct. Please do commit that change.

I'm guessing that @cseelus updated activestorage.js manually instead of running yarn build to generate it.

Context rails#33413 (comment)

Context rails#33413 (comment) Related to rails#33656

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

rails-bot assigned rafaelfranca Jul 22, 2018

javan suggested changes Jul 27, 2018

View reviewed changes

cseelus force-pushed the active-storage-direct-uploads-multiple-submit-buttons branch from 1c88478 to af1dd2e Compare August 5, 2018 00:07

cseelus force-pushed the active-storage-direct-uploads-multiple-submit-buttons branch from af1dd2e to c0d949d Compare August 12, 2018 15:02

cseelus force-pushed the active-storage-direct-uploads-multiple-submit-buttons branch 2 times, most recently from 3971eca to c9ee0b8 Compare August 12, 2018 16:10

javan reviewed Aug 12, 2018

View reviewed changes

javan reviewed Aug 13, 2018

View reviewed changes

javan assigned javan and unassigned rafaelfranca Aug 18, 2018

javan added the activestorage label Aug 18, 2018

cseelus force-pushed the active-storage-direct-uploads-multiple-submit-buttons branch from 6272904 to 6278103 Compare August 18, 2018 16:26

Enable multiple submit buttons for Active Storage forms

880f977

cseelus force-pushed the active-storage-direct-uploads-multiple-submit-buttons branch from 6278103 to 880f977 Compare August 18, 2018 17:32

javan changed the title ~~Use ujs:submit-button as submitForm click target~~ Support multiple submit buttons in Active Storage forms Aug 18, 2018

javan approved these changes Aug 18, 2018

View reviewed changes

javan merged commit 9270c6d into rails:master Aug 18, 2018

rails deleted a comment Aug 18, 2018

cseelus deleted the active-storage-direct-uploads-multiple-submit-buttons branch August 19, 2018 03:16

javan added a commit that referenced this pull request Aug 19, 2018

Merge pull request #33413 from cseelus/active-storage-direct-uploads-…

388a1f3

…multiple-submit-buttons Support multiple submit buttons in Active Storage forms

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 19, 2018

activestorage: yarn build

0e77706

Context rails#33413 (comment)

bogdanvlviv mentioned this pull request Aug 19, 2018

activestorage: yarn build #33656

Merged

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Aug 19, 2018

[5-2-stable] activestorage: yarn build

4f4027c

Context rails#33413 (comment) Related to rails#33656

bogdanvlviv mentioned this pull request Aug 19, 2018

[5-2-stable] activestorage: yarn build #33658

Merged

kemenaran mentioned this pull request Dec 4, 2018

Bump gems demarche-numerique/demarche.numerique.gouv.fr#3114

Merged

rmacklin mentioned this pull request Oct 19, 2019

Enforce that activestorage compiled JS is in sync with source code #37508

Merged

seanpdoyle mentioned this pull request Dec 14, 2020

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

Merged

	document.addEventListener("submit", didSubmitForm)
	document.addEventListener("ajax:before", didSubmitRemoteElement)

Conversation

cseelus commented Jul 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rails-bot commented Jul 22, 2018

Uh oh!

javan Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

cseelus Aug 4, 2018

Choose a reason for hiding this comment

Uh oh!

cseelus commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javan commented Aug 12, 2018

Uh oh!

cseelus commented Aug 12, 2018

Uh oh!

javan Aug 12, 2018

Choose a reason for hiding this comment

Uh oh!

cseelus commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javan Aug 13, 2018

Choose a reason for hiding this comment

Uh oh!

javan commented Aug 13, 2018

Uh oh!

cseelus commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javan commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cseelus commented Aug 18, 2018

Uh oh!

cseelus commented Aug 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javan commented Aug 18, 2018

Uh oh!

cseelus commented Aug 18, 2018

Uh oh!

javan commented Aug 18, 2018

Uh oh!

javan commented Aug 19, 2018

Uh oh!

bogdanvlviv commented Aug 19, 2018

Uh oh!

javan commented Aug 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cseelus commented Jul 22, 2018 •

edited

Loading

cseelus commented Aug 4, 2018 •

edited

Loading

cseelus commented Aug 12, 2018 •

edited

Loading

cseelus commented Aug 13, 2018 •

edited

Loading

javan commented Aug 17, 2018 •

edited

Loading

cseelus commented Aug 18, 2018 •

edited

Loading