Support multiple submit buttons in Active Storage forms

[cseelus]

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]

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]

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!)

[cseelus]

Yes, thats understandable.

[cseelus]

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]

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]

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

Added the changes there too.

[javan]

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]

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]

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

[javan]

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]

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]

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]

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

[cseelus]

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]

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]

Done

[javan]

Thank you!

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

[javan]

Backported to 5-2-stable: 388a1f3

[bogdanvlviv]

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]

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.