If we have a form with ajax buttons (i.e. multi-field) when we will press enter (i.e. in simple textfield) form will be reloaded without being submitted.
I propose do not replace ajax-ed input submit with button html tag.

CommentFileSizeAuthor
#2 7.x-2.x-hide_submit-fix-ajax-buttons-enter-press-2763391-2.patch566 bytesIRuslan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

IRuslan created an issue. See original summary.

IRuslan’s picture

IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented
Status: Needs work » Needs review
FileSize
7.x-2.x-hide_submit-fix-ajax-buttons-enter-press-2763391-2.patch566 bytes

Patch attached.

sumthief’s picture

sumthief CreditAttribution: sumthief as a volunteer and at DrupalJedi commented
Status: Needs review » Reviewed & tested by the community

Hello,

I've made a review of the patch on clean Drupal install.
I've reproduced described bug:
1. Go to settings via admin/config/content/hide-submit.
2. Set blocking method to "Built-in loading indicator".
3. Describe custom form with elements like this:

//...
 $form['test_text'] = array(
    '#type' => 'textfield',
    '#title' => t('Test textfield'),
  );

  $form['test_submit'] = array(
    // Also checked on 'button' type.
    '#type' => 'submit',
    '#value' => t('Test submit'),
    '#ajax' => array(
      'callback' => 'do_something',
    ),
  );
//...

4. Set cursor to the text input and press enter.
Expected result: Nothing happens.
Achieved result: Button submitted.

Attached patch looks good and solves this problem.

P.S.: Also tried to reproduce this bug on other blocking methods and have no success. So patch looks good. I think we can set RTBC to the patch and include to the next release.

rymcveigh’s picture

rymcveigh
he/him/his
CreditAttribution: rymcveigh at CARD.com commented 
+++ b/js/hide_submit.js
@@ -24,7 +24,7 @@
+          $('input.form-submit:not(.ajax-processed)', $form).each(function(index, el) {

@IRuslan I'm reviewing your patch and I recommend using .not() instead of :not. Even though :not has a slight improvment on performance it tends to be less reliable especially on older browsers. The jQuery docs state:

The .not() method will end up providing you with more readable selections than pushing complex selectors or variables into a :not() selector filter. In most cases, it is a better choice.

.

IRuslan’s picture

IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented

@rymcveigh
Thanks for attention on this.

Actually, I agree it's a bit easy to read.
Two points stop me from replace it with '.not()'
* Drupal core use the same ':not()' approach (see misc/ajax.js)
* $('something:not(something-else)', context) could be not the same with $('something', context).not('something-else') in terms of implementation (but the result for sure will be the same).

In this particular case, the selector is pretty short and simple.
But if you could provide me some links about reliability (not readability) of ':not()', I would be glad to replace it.

rymcveigh’s picture

rymcveigh
he/him/his
CreditAttribution: rymcveigh at CARD.com commented

@IRuslan Here are some places in major modules and libraries where you'll find .not in use: views(module), jquery.ui(library), features(module), extlink(module), context (module). Take that for what it's worth since some of those modules use :not as well.

Personally, I've seen issues with :not on some browsers like versions of IE less than 11 and I believe readability is worth considering here.

Anther thing to consider is that developers may choose to use libraries like jQuery (Sizzle) that extend the :not selector with non-standard selectors, so it's easy to break qSA. Personally I like the :not selector and would love to use it but choose not to due to issues I've had to deal with in the past. .not() is just more reliable in the long run.

IRuslan’s picture

IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented
Issue summary: View changes