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

Support multiple submit buttons in Active Storage forms #33413

Merged
merged 1 commit into from Aug 18, 2018

Conversation

Projects
None yet
5 participants
@cseelus
Copy link
Contributor

cseelus commented Jul 22, 2018

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

multiple submits

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

This comment has been minimized.

Copy link

rails-bot commented Jul 22, 2018

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.

activestorage/app/javascript/activestorage/ujs.js Outdated
@@ -49,7 +49,8 @@ function handleFormSubmissionEvent(event) {
}

function submitForm(form) {
let button = findElement(form, "input[type=submit]")
let { value, name } = form._ujsData["ujs:submit-button"]

This comment has been minimized.

@javan

javan Jul 27, 2018

Member

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

This comment has been minimized.

@cseelus

cseelus Aug 4, 2018

Author Contributor

Yes, thats understandable.

@cseelus

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 4, 2018

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?

@cseelus cseelus force-pushed the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch Aug 5, 2018

@cseelus cseelus force-pushed the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch Aug 12, 2018

@javan

This comment has been minimized.

Copy link
Member

javan commented Aug 12, 2018

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 cseelus force-pushed the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch 2 times, most recently Aug 12, 2018

@cseelus

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 12, 2018

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

Added the changes there too.

activestorage/app/javascript/activestorage/ujs.js Outdated
event.target.setAttribute("data-clicked", "true")
}

document.addEventListener("turbolinks:load", function(event) {

This comment has been minimized.

@javan

javan Aug 12, 2018

Member

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

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

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 12, 2018

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.

activestorage/app/javascript/activestorage/ujs.js Outdated
const inputs = findElements(document, "input[type=submit]")
inputs.forEach(function(input) {
input.addEventListener("click", tagClickedButton)
})

This comment has been minimized.

@javan

javan Aug 13, 2018

Member

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

@javan

This comment has been minimized.

Copy link
Member

javan commented Aug 13, 2018

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

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 13, 2018

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

This comment has been minimized.

Copy link
Member

javan commented Aug 17, 2018

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

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 18, 2018

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

@cseelus

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 18, 2018

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 javan assigned javan and unassigned rafaelfranca Aug 18, 2018

@javan javan added the activestorage label Aug 18, 2018

@javan

This comment has been minimized.

Copy link
Member

javan commented Aug 18, 2018

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 cseelus force-pushed the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch Aug 18, 2018

@cseelus

This comment has been minimized.

Copy link
Contributor Author

cseelus commented Aug 18, 2018

Done

@cseelus cseelus force-pushed the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch to 880f977 Aug 18, 2018

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

@javan

javan approved these changes Aug 18, 2018

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

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rails rails deleted a comment Aug 18, 2018

@rails rails deleted a comment Aug 18, 2018

@javan

This comment has been minimized.

Copy link
Member

javan commented Aug 18, 2018

Thank you!

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

@cseelus cseelus deleted the cseelus:active-storage-direct-uploads-multiple-submit-buttons branch Aug 19, 2018

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

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

Support multiple submit buttons in Active Storage forms
@javan

This comment has been minimized.

Copy link
Member

javan commented Aug 19, 2018

Backported to 5-2-stable: 388a1f3

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor

bogdanvlviv commented Aug 19, 2018

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

This comment has been minimized.

Copy link
Member

javan commented Aug 19, 2018

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.

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

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

@kemenaran kemenaran referenced this pull request Dec 4, 2018

Merged

Bump gems #3114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.