Hooks for data-remote forms don't work with the HTML5 form attribute #303

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

Contributor
asilano commented Jan 29, 2013

HTML5 provides the form attribute on input, button etc. elements. This associates the element with the form whose id matches the attribute, rather than the closest ancestor form. See the MDN documentation on input.

jquery-ujs does not interact properly with the form attribute; clicking a submit button associated with a remote form via the form attribute should invoke normal UJS remote hooks and submit by AJAX. Instead, the form is submitted as a standard HTTP transaction.

Member

This is definitely an issue we should address, but I see a few problems with the submitted pull request. We'd need to add some tests to make sure this all works as expected.

@JangoSteve JangoSteve commented on the diff May 29, 2013
src/rails.js
@@ -35,7 +35,7 @@
formSubmitSelector: 'form',
// Form input elements bound by jquery-ujs
- formInputClickSelector: 'form input[type=submit], form input[type=image], form button[type=submit], form button:not([type])',
+ formInputClickSelector: 'input[type=submit], input[type=image], button[type=submit], button:not(button[type])',
JangoSteve
JangoSteve May 29, 2013 Member

I'd also want to make sure that changing the formInputClickSelector as in this PR doesn't cause functions to be unintentionally run, e.g. for random buttons on the page that don't belong to any form.

@JangoSteve JangoSteve commented on the diff May 29, 2013
src/rails.js
@@ -191,7 +193,9 @@
- Sets disabled property to false
*/
enableFormElements: function(form) {
- form.find(rails.enableSelector).each(function() {
+ form.find(rails.enableSelector)
+ .add($(rails.enableSelector).filter('[form=' + form.attr('id') + ']'))
JangoSteve
JangoSteve May 29, 2013 Member

I'm not a big fan of all the additional run-time jQuery selectors that must scan the entire DOM and instantiate a lot of additional objects. I'd like to look through and see if there's some way we could be accomplishing this a bit more efficiently.

@JangoSteve JangoSteve commented on the diff May 29, 2013
src/rails.js
@@ -378,7 +383,17 @@
var name = button.attr('name'),
data = name ? {name:name, value:button.val()} : null;
- button.closest('form').data('ujs:submit-button', data);
+ var attrForm = $('#' + button.attr('form'));
+
+ if (attrForm == undefined || !attrForm.is('form') || attrForm == button.closest('form')) {
JangoSteve
JangoSteve May 29, 2013 Member

We can get rid of the last condition here, as it causes extra jQuery object scanning and instantiation when it's not needed.

Contributor
asilano commented May 29, 2013

Thanks for taking a look at this. I'm happy to start making the changes you suggest; but as you've probably realised I'm not really experienced in JavaScript, jQuery or UJS; the pull request Works For Me but I'm not sure how easy I'd find it to improve the code. Plus, I'm on Windows and I therefore couldn't bundle install the jquery-ujs test framework last time I tried.

In short - I'm happy to work on this, with guidance; equally, if you (or someone else better qualified than me) want to take my changes as a starting point and run with them, that's fine by me.

@asilano asilano closed this Oct 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment