Description of bug: A submit form element with a #ajax handler should handle the entire submit action without a page reload. It sometimes/often does not.

What happens: Set a form element to have a #ajax. Press the submit button. Always the #ajax action fires and what happens should happen. SOMETIMES the form then submits after that, and the page reloads.

What I expect: The #ajax handler should handle everything about the form submission, and the form should not manually submit, nor should the page reload.

Example code: The D7 Examples module AJAX Example, Advanced AJAX examples. Click any of the submit buttons. If you don't wait 5-6 seconds between clicks, they will often submit the page after the javascript processing is done.

This bug exists in D6 and D7, using Firefox and several versions of IE.

@aidan suggests the use of e.preventDefault()
@effulgentsia says "the problem is the onSubmit() handler needs to return FALSE to prevent the browser from doing what it normally does on a submit event. but, for some weird reason, it apparently needs to do it fast enough. for example, if you stick in an "alert" in the onSubmit() handler, or even a breakpoint, then you can't stop the form from submitting."

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

The complexity of this one is that we often have forms in which some elements are #ajax, but the submit button is not.

If you turn off all normal form submit with
$("form").submit(function() { return false; });

then the inappropriate form submits are completely removed.

Unfortunately, this disables a *normal* button in any form that has a #ajax element.

The attached patch shows what I've tried so far, and it seems to improve, but not solve the situation. It

  • Explicitly tells the form element not to submit.
  • Moves the disabling of the clicked element a little sooner in the javascript execution.

But it does not completely solve this.

The attached module is a testbed for this. You can use the "submit only" or "submit only, multiple" menu items. Keep on hitting the submit button. Eventually it will submit the form instead of doing the AJAX action.

bleen’s picture

subscribe

effulgentsia’s picture

Title: AJAX submit of a 'submit' element works... but then also does the regular submit » Submit buttons that trigger AJAX submissions sometimes fail to prevent browser-level form submission
Component: forms system » javascript

Subscribing. Changing "component" of the issue to try to get some JavaScript experts to see this and comment. Has aidan's suggestion of e.preventDefault() been tried?

rfay’s picture

My notes from my debugging session were not conclusive, but I definitely tried that. Here are the notes.

I tried a lot of things in my debugging session on this. Here are the notes, in case they spur any ideas. I did not find a definitive answer.

AJAX debugging notes 11/29/2009 and 1/23/2010

"Submit submits the form instead of just calling ajaxSubmit"

1. Add e to signature and call e.preventDefault(). This improves the behavior.

  // Bind the ajaxSubmit function to the element event.
  $(this.element).bind(element_settings.event, function (e) {
    console.log("event, this=%o ajax=%o e=%o e.preventDefault=%o",this, ajax, e, e.preventDefault);
    if (e.preventDefault) {
      console.log("calling e.preventDefault");
      e.preventDefault();
    }

2. The jquery way: $("form").submit( function() { return false; } );

3. Use bind instead of submit on just the item in question.

4. This is likely a timing issue, where the button doesn't get disabled in time. Check where it gets disabled and move it up! Make it happen immediately on the event. Make sure it doesn't get disabled until everything is done.

5. Jquery Form docs clearly state that returning false after ajaxSubmit() should do the job. http://jquery.malsup.com/form/#api

6. Moving the disable into the serialize function makes it happen sooner:

Drupal.ajax.prototype.beforeSerialize = function (element, options) {
  // Disable the element that received the change.
  $(this.element).addClass('progress-disabled').attr('disabled', true);

7. KEY: Disable submit before binding:

+  // Turn off the normal form submit, since we will use this only with AJAX.
+  $(this.element).submit(function() { return false; });
+
   // Bind the ajaxSubmit function to the element event.
casey’s picture

Subscribing.

effulgentsia’s picture

Priority: Normal » Critical
Issue tags: +Ajax

Tagging and bumping to critical. I think it's critical, because even though the same bug exists in D6, D6 AHAH just isn't used all that much. But D7 AJAX is so awesome that it will be used throughout contrib in all sorts of interesting ways, unless a bug like this discourages that. I suspect some attention from people who really know JavaScript will lead to a solution, but if it's unsolvable, we can always de-prioritize it as we get closer to release.

tanoshimi’s picture

Subscribing. I seem to consistently get both #submit and #ajax[callback] handlers firing on a very simple form:

  $form['submit'] = array(
    '#type' => 'submit',
    '#ajax' => array(
      'callback' => 'mymodule_ajax_submit',
      'wrapper' => 'box',
      'name' => 'submit1',
    ),
    '#value' => t('Submit'),
    '#submit' => array('mymodule_noajax_submit'),
  );

I'm not that familiar with drupal AHAH/AJAX but I'm assuming from this issue that this is not expected behaviour!

Furthermore, disabling javascript (FF3.5.8) means that neither function gets called... which just doesn't seem right at all...

rfay’s picture

@tanoshimi, Could you repeat your testing on what happens with javascript disabled? I don't know how that could happen, but if it does, this is a truly critical bug.

effulgentsia’s picture

@tanoshimi: Having both handlers fire is the correct behavior. The #submit is not a "no js", but a "regardless of js or no js". The #ajax['callback'] is what runs after the #submit runs and after $form has been rebuilt with changes as a result of the submission, and #ajax['callback'] should not have any submission logic in it, but only return the part of the form that has changed and needs to be sent back to the browser for it to replace the old content in 'wrapper'. I appreciate that this is confusing, and I created an issue about that: #649628: Make it easier to write AJAX-enabled forms that fully work with JavaScript disabled too, but it's too late to do anything about that in D7, other than document it well, which rfay has done (some links to that documentation is on that issue, perhaps rfay can post other links here).

If #submit is not running when JS is disabled, that's a bug. Please open a new issue for that, and include a complete .module file that demonstrates it. Please add a comment here with a link to that issue when you open it.

This issue isn't about either of those (though I understand why you thought it was). This issue is about a JavaScript bug where clicking the submit button multiple times (not even all that quickly) will sometimes trigger the AJAX submission, and sometimes trigger the non-AJAX submission, based on quirkiness with timing.

tanoshimi’s picture

Thanks very much for the replies.
@effulgentsia - I'm sorry if I accidentally hijacked this thread, and your first post in #649629 pretty much exactly summarises my failure to understand the purpose of the #ajax['callback'] when trying to create AJAX-ified submission forms that degrade nicely when javascript is not available! I'll be sure to read all the associated doc links to try to find a workaround for what I thought #ajax['callback'] did (i.e "if javascript available, call this ajax submit function, otherwise, call this regular submit function") - it seems that this might be what the ahah_helper module did for D6.

@rfay - My testing might have been erroneous because I can't now recreate this (I'm not sure whether it was a caching issue - at what point does drupal determine if I have JS enabled? on every page load? on session start? simply enabling/disabling javascript in the browser and reloading the page didn't seem to always set it correctly)... I'll keep on trying but I don't think you should set alarm bells ringing when it's probably just my mistake!

As a side note, while the existing AJAX examples are great, I think I will raise a request in the "Examples for Developers" module to include an example of [#ajax] that degrades when javascript is not available...

So, sorry for wasting your time, and thankyou both.

rfay’s picture

@tanoshimii: It's not Drupal that determines whether AJAX is enabled so much as your browser actually executing any javascript code. If the form is ajax-enabled, there is javascript code that goes with it. If that code runs (if javascript is enabled on the browser) then the form behaves as an AJAX-enabled form.

We will be doing a form example that's simpler and that degrades to a multistep form, and your issue will be welcome in the Examples queue, as well as an example if you want to contribute one yourself.

Before Drupalcon I'll be updating my document on AJAX/AHAH (http://randyfay.com/ahah) and putting it on Drupal.org. Right now the Examples module is the best set of code, I think.

effulgentsia’s picture

I'm sorry if I accidentally hijacked this thread...So, sorry for wasting your time, and thankyou both.

Not at all. You posted a completely reasonable comment given the title of this issue and the remaining complexities inherent in the AJAX system. It's great to have another person looking through all this, and I hope we all contribute to clearer examples and documentation.

katbailey’s picture

subscribing

rfay’s picture

katbailey and I took a look at this today and found that with current code (and both of our machines) it was recreatable but not a major issue. In other words, you had to click *really* fast and a lot of times to see this behavior. This is *not* what I had seen earlier, when you could recreate it by just clicking at 2-second intervals.

I'll try to bring out the machine I was testing on earlier (a bit slower, less cores) and see if something has magically improved this behavior.

rfay’s picture

Priority: Critical » Minor

After failing to demonstrate this adequately to katbailey today, I spent some time with this tonight. It seems to be tied to either the operating system/webserver/php or the hardware? of the *server*.

First, it's easy to demonstrate this on any server or client (and I think any browser) by just clicking fast. It will eventually do a browser-level submit. But it takes unusual speed and abnormal effort to make it happen. So there's a bug in there somewhere (in browser? Drupal? somewhere?) but it may not be worth fixing.

I was easily able to replicate this on the "old" Toshiba Intel Core 2 Duo machine running Ubuntu 9.04/PHP 5.2.12 (from the dotdeb release, 5.2.12-0.dotdeb.1). With it running Drupal, the browser sometimes submitted the very first time, and often by the third or fourth 2-second click.

I was unable to replicate it without rapid, rapid clicking when the newer Sony I7 machine was the server. (Ubuntu 9.10/php 5.2.10/5.2.10.dfsg.1-2ubuntu6.4).

So I installed it on a Dreamhost shared account (php 5.2.12): http://drupal7.thefays.us/ajax_example/advanced_commands. Just click the first button "AJAX After" and see if you can get it to fail. You *can*, if you stick with it. But it only happens after many clicks.

However, since I can't demonstrate this at the awful level on anything except one machine, I'm lowering the priority to "minor". It is conceivable, though, that this will be a server-software-dependent issue and we'll find it later and find it to be annoying.

At this level it is very difficult to study.

Opinions and suggestions are welcome!

effulgentsia’s picture

Title: Submit buttons that trigger AJAX submissions sometimes fail to prevent browser-level form submission » Race condition causes AJAX-enabled submit buttons to sometimes submit a form as non-AJAX

Since ideally, AJAX-enabled forms should be written to degrade well to users with JavaScript disabled, this condition doesn't seem to result in a serious functional bug: once in a while a user with JavaScript enabled will have the same thing happen as if he or she had JavaScript disabled. As long as this is true, I think it's fine to consider this "minor". However, if this race condition is discovered to cause a serious functional bug, I think we should upgrade the priority to "normal". This issue can also be linked to from within documentation that reminds people to write forms that degrade to no-js: hey, it's not just for the small percentage of users who disable js, it's also for people who click too fast.

sun’s picture

Component: javascript » ajax system
Priority: Minor » Normal
Status: Active » Postponed (maintainer needs more info)
Issue tags: -Ajax

Reclassifying.

The internal ajax.ajaxing property should prevent this from happening. Unless we have clear steps to reproduce, needs more info.

effulgentsia’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

More info in #995854-69: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form. We may be able to solve the problem as part of that issue, so marking as duplicate. If we don't solve it there, I'll reset this to "active".

rfay’s picture

Status: Closed (duplicate) » Active

This is a actually not hard to reproduce. I agree that ajax.ajaxing should prevent it (and so should the "return false" in the submit handler.

I recreated drupal7.thefays.us from current HEAD and the procedure in #15 (using firefox 3.6 or chrome) still makes it fail. It also will fail on examples/ajax_example/submit_driven_ajax if you just keep hitting submit. Sometimes this happens quite often, sometimes less. It may be related to the server side in some way. This doesn't seem to happen with IE8.

Just go to http://drupal7.thefays.us/ajax_example/advanced_commands with firefox and hit submit on the first button lots of times. It will eventually do a page (non-ajax) submit. With a faster server-side experience it will happen sooner.

rfay’s picture

Status: Active » Closed (duplicate)

x-post.

effulgentsia’s picture

aspilicious’s picture

I followed steps from http://drupal.org/node/216059#comment-721125

1) Enable the poll module and create a new poll.
2) Move to the title field and hit the enter button.
3) A new element will be added to the AHAH add more section.

This still gives the same problems, I used the patch webchick made in #1004742: Change #ajax event 'mousedown' into 'click'

I also noticed something else (probably not related)
1) Pushing the enter button made the row weights appear + drag and drop icon => drag and drop broken
==> pressing add more fixes the issue

sun’s picture

Title: Race condition causes AJAX-enabled submit buttons to sometimes submit a form as non-AJAX » AJAX submit buttons sometimes submit a form as non-AJAX
Status: Active » Needs review
FileSize
4.47 KB

It took some time to figure out the actual problem... documented in code.

rfay’s picture

This works for me.

The way I usually test this one is with AJAX Example enabled, go to
examples/ajax_example/submit_driven_ajax

and just click as fast as I can until it submits. Without this patch, it does it before long. With the patch it doesn't do it any more.

rfay’s picture

Capturing sun's comments on this issue from IRC:

<sun>rfay: http://drupal.org/node/634616 revealed a quite poor problem in the AJAX framework currently... did you read the code comments?  That workaround should work in FF, Opera, IE... but support/behavior in Konqueror, Safari, and perhaps even Chrome is unknown
<sun> And the problem space is actually easy to understand:  Pressing enter in a input type="text" submits a form.  In case you press enter in a text input of a multiple value field, the form is submitted.  Hence, the AJAX event on the submit button is triggered.
<sun>However, that event is triggered by the *browser*, so the event actually states that the form button has been pressed -- which is not the case.
<sun> ...because enter has been pressed in a text input.  The button was not pressed at all.
sun’s picture

@DamZ had the idea of purposively hi-jacking pressing ENTER in AJAX-enabled forms, and forcing all browsers to conform to standards by manually resembling the html5 behavior described in:
http://www.w3.org/TR/html5/association-of-controls-and-forms.html#implic...

Given that I already know that the approach of the last patch is or may not be 100% x-browser compatible, that sounds worth to explore.

effulgentsia’s picture

See also #927176: "Enter" key removes file instead of saving node. While annoying, I'm not sure it's within the scope of this issue (or of D7) to fix. Maybe worth exploring in http://drupal.org/project/html5? We now know the answer to why HEAD binds to mousedown instead of click. Would it make sense to bring over the 'secondary_event' code from #995854-70: #ajax doesn't work at all if a file element (or enctype => "multipart/form-data") is included in the form (dropped in subsequent patches) as a solution to the race condition problem?

effulgentsia’s picture

A simplified version of what was started in that other issue. Code comments here also explain how to consistently reproduce the bug.

sun’s picture

While adding a general purpose #ajax[prevent] setting would definitely be useful, I think that we have to continue the direction of changing mousedown to click. It is also true that #927176: "Enter" key removes file instead of saving node is very closely related. But to me it looks like all of these issues are just scratching the surface, and each one is adding code to work around consequences that are caused by the workaround of binding to mousedown in the first place. And to underline that, it's highly uncommon and unusual to bind to mousedown.

Status: Needs review » Needs work

The last submitted patch, 634616-ajax-race-condition.28.patch, failed testing.

rfay’s picture

For D7, let's take whatever approach that we *can* to get this problem solved. It can be solved elegantly in D8 (or in D7, that's fine) but let's just make sure this gets solved.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

Well, I'm all for changing to 'click' if we can. Here's a patch to do it.

From #216059-12: AHAH triggered by text input enter key press, breaks e.g. autocomplete:

I realized that this is probably not the AHAH event firing 5 times, and indeed, it is basically not firing, but the usual form submission happens, which adds 5 new options.

And from #216059-52: AHAH triggered by text input enter key press, breaks e.g. autocomplete:

Everything works as expected in FF -- pressing enter on a poll choice field generates a full form submission with five new fields, pressing enter on autocompletes in either the author or CCK add more forms selects the value without triggering the AHAH.

I'm not clear from reading that issue how this was deemed an improvement. Fixing autocomplete breakage is indeed wonderful, but why is triggering the non-AJAX submit of the button considered better than triggering the AJAX submit of the button? In both cases, you're triggering the button that is logically unrelated to the textfield you're on.

In other words, in HEAD, if you click ENTER on the title field, it triggers the non-AJAX submit of the first button. If that happens to be poll module's "More choices" button, it adds 5 new choices. With this patch, we revert that original issue, and then clicking ENTER on the title field triggers the submit of the first button. If that button happens to be AJAX-enabled, then this means it triggers the AJAX submit. If that happens to be poll module's "More choices" button, it adds 1 new choice. Both versions suck. I'm not sure which sucks more. I guess one problem with allowing the AJAX submit is that it can be happening below the viewport, so the user doesn't know it's happening. At least with a non-AJAX submit, you can see a page refresh to know that something unexpected happened.

BTW: I re-checked autocomplete, and that seems fixed regardless. I'm guessing autocomplete.js has been improved at some point since that issue to trap the ENTER key.

Also, this patch includes a change to field_ui.js. But what about contrib code that assumes mousedown? Also, there's some code in file.js that works with mousedown and might need to be changed.

If we can solve the UI issues above (e.g., by making #26 work) and address the BC concerns for contrib modules already relying on mousedown, great. Otherwise, given we already have mousedown in D6 and D7, is it really so bad to leave it, fix all these problems properly in D8, and just do something like #28 for the race condition?

effulgentsia’s picture

Just as a proof of concept, this patch builds on #32, and adds some code to disable textfield ENTER presses from triggering a button click. This code is not fit for committing, but maybe can be improved to be something decent.

effulgentsia’s picture

This, I think, is acceptable for committing. I thought about whether we want to implement something like #26. In particular, this part of the html5 recommendation:

User agents may establish a button in each form as being the form's default button. This should be the first submit button in tree order whose form owner is that form element, but user agents may pick another button if another would be more appropriate for the platform.

In other words, trying to pick a better default button to trigger a click on, such as the first non-AJAX button, or even adding a #default_button property to FAPI, but decided this would lead to integration side effects, such as how would other code intervene to stop that (preventDefault() would no longer work). Ultimately, I think that is the work for contrib or for D8, but not for D7 core, at this late stage. But just what's in this patch, I think is acceptable.

I also updated file.js as needed, but as per #32, given that this patch requires changing field_ui.js and file.js, are we concerned about BC breaks for contrib? I agree that we *should* change from mousedown to click. But will contrib authors who are already assuming mousedown be understanding of this late change? That said, #216059-58: AHAH triggered by text input enter key press, breaks e.g. autocomplete changed from click to mousedown for D6 2 days before 6.0 was released. So maybe changing back to the proper event a day or two before 7.0 release is ok too?

effulgentsia’s picture

If we're gonna go with #34, let's do it before 7.0 is released, since it is, technically, a BC break.

Issue Summary
Drupal 6.0 was released on 2/13/08 with a shiny new AHAH framework. The framework allowed FAPI elements to have a #ahah property for attaching AHAH behavior to that element. When setting the #ahah property, the developer may set #ahah['event'] to customize which JavaScript event should trigger the AHAH behavior, but this is rarely used. Usually developers leave this unspecified, and allow the AHAH system to set a reasonable default based on element types. Up until 2 days before 6.0 was released, the default for submit buttons (including image buttons), was 'click'. Then, just before 6.0 was released, this was changed to 'mousedown' to solve #216059-58: AHAH triggered by text input enter key press, breaks e.g. autocomplete.

For Drupal 7, the AHAH system was revamped into a more powerful AJAX system. Support for links (not just form elements) was added, but other than that, default event bindings were not revisited. Buttons still bind to 'mousedown', text inputs still bind to 'blur', and select/radio/checkbox still bind to 'change'.

However, binding to 'mousedown' is really a hack. From http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#e...

Note: For maximum accessibility, content authors should use the click event type when defining activation behavior for custom controls, rather than other pointing-device event types such as mousedown or mouseup, which are more device-specific. Though the click event type has its origins in pointer devices (e.g., a mouse), subsequent implementation enhancements have extended it beyond that association, and it should be considered a device-independent event type for element activation.

As sun alludes to in #29, and as per the above message from W3C, 'click' is the de-facto standard event for activating a button. There are side-effects when you deviate from standards:

  • Right now, in D6 and D7, AHAH/AJAX buttons are triggered on mousedown with the right, middle, etc. mouse buttons, as well as the left button. Maybe no big deal, but probably not what the work on #216059: AHAH triggered by text input enter key press, breaks e.g. autocomplete intended to do.
  • This issue came about, because if we bind to mousedown, then under some circumstances, it's possible for both a mousedown and a click to register, and therefore, for both the AJAX behavior, and the non-AJAX form submission behavior of the button to occur.
  • #216059: AHAH triggered by text input enter key press, breaks e.g. autocomplete wasn't fully solved. It just replaced the undesirable triggering of an AHAH submission from a textfield's ENTER press with an undesirable triggering of a non-AHAH form submission. Which was a small improvement, but as per #927176: "Enter" key removes file instead of saving node, I don't think anyone considers it ideal.
  • As can be seen in the field_ui.js and file.js hunks of #34, if another jQuery plugin/behavior wants to activate the button, or bind additional behavior to the button's activation, it needs to know that the Drupal AJAX system is non-standard and uses mousedown rather than click, and adjust accordingly. Suddenly, 1 hack propagates into many hacks. This is fine for Drupal code, but makes it harder to integrate 3rd party plugins that have no reason to know about Drupal's deviation from standards.
  • It's not very forward thinking. As part of the issue that initially changed from click to mousedown, we also had to add a keypress binding in order to support keyboard access. But what about other input devices that trigger neither mouse nor keyboard events? Or trigger a keypress, but not ENTER or SPACE? I don't know if such devices exist currently, but will ones be invented in D7's lifetime? The benefit of a standardized activation event like 'click' is that we can be confident that new devices will trigger it.

So, #34 goes back to using click. But solves the original issue by capturing an ENTER press on textfields.

I tested this on Firefox 3.6, Chrome 8, Safari 5, and Opera 11, all on Mac. And on Windows 7 IE 8. #24 and #927176: "Enter" key removes file instead of saving node are fixed by this patch, and #216059: AHAH triggered by text input enter key press, breaks e.g. autocomplete remains fixed. Additional x-browser testing (e.g., IE6/7/9) would be helpful.

OTOH, if we decide that it's too late to be making this change in D7, then we can fix each of the issues in the above bulleted list separately, as needed. #28 contains a fix for this specific issue, and I'm sure we can get those tests to pass.

effulgentsia’s picture

Title: AJAX submit buttons sometimes submit a form as non-AJAX » Various problems due to AJAX binding to mousedown instead of click
Priority: Normal » Major
Issue tags: +API change

Retitling, tagging, and upping priority to reflect #34/#35. If we don't get that in, we can revert to the more limited scope of #28.

threewestwinds’s picture

I've been working with ajax a bit, and keep butting heads with Drupal's 'mousedown' behavior. I don't have time to test the patch right now, but I support #34 based on a quick look over the code and the history of the issue.

As one of the contrib developers who you'll be breaking backwards compatibility with, I'd love to see this change happen. It'll make my job much easier.

ufku’s picture

For the record, enter key triggers submit-click on any form element not just on text inputs.

edit:
exceptions: textarea and select.

ufku’s picture

The issue is how to differentiate between enter-triggered click and regular click. Variable dump of the two events are very similar and there seems to be no cross-browser way to distinguish between the two.

Just some thoughts:
The actual event that triggers submit-click is keydown. A dirty hack could involve recording the time(T) of enter-keydown event of form elements and then checking it in ajax-button's click callback. If the difference between current time and the last recorded time(T) is very small(under 1 sec) then it is an enter-triggered click(do nothing), otherwise a regular click(do the ajax job).

threewestwinds’s picture

#28: 634616-ajax-race-condition.28.patch queued for re-testing.

As per talk with webchick/sun in IRC, it's too late for #35. Thus, I'm going to see if I can get #28 to work. I'll make a new patch once updated test results come in.

webchick’s picture

effulgentsia, thanks once again for the excellent issue summary at #35.

sun, threewestwinds, merlinofchaos, and I kicked this around in #drupal-contribute for a bit. I understand all the arguments for click being better than mousedown, and this would be great to get into 7.0, which is a huge boost for us in the accessibility department. However, there is simply absolutely no way this is getting in before 7.0. The BC break we're talking about needs cross-browser testing all over the place, and we need to ensure we don't break major contrib modules like Views, Page Manager, etc. With < 36 hours to go, we don't have time (and even if we do, I'm not willing to take the risk).

I am, however, willing to entertain arguments for including in a later point release of D7, if there's enough buy-in that we can do this from the javascript/ajax maintainers (sun, merlin, eff, quicksketch, rfay, ksenzee, etc.) without breaking stuff everywhere. It does seem like it could prompt weird behaviours though, so we might actually be safer pushing to D8. :\

HylkeVDS’s picture

subscribe

sime’s picture

Am I right that #28 current passes testing?

sime’s picture

Reroll of #28. Note that all references to "AJAX" have changed to "Ajax" since then, otherwise the patch is the same.

I tested this patch against the Examples module, specifically Submit driven ajax. It works very well.

sime’s picture

Status: Needs review » Reviewed & tested by the community

Marking this RTBC. This solution has had a number of reviews. Based on discussions, this solution doesn't break backwards compatibility and there are no compromises, it simply minimises errors of a less-than-ideal implementation.

I think this is quite an important fix. As more and more Ajax is used people are going to be seeing unexpected page refreshes. I had been working on an example with Ajax, and for hours I thought I was doing it wrong. I thought I was not attaching the event properly because the page was refreshing.

In other cases the bug will appear to the end user where it doesn't appear to the developer. This happened to me when I was triaging my problem against rfay's Examples demo website. On my localhost, the Examples module (identical setup) was much more buggy than on rfays website. I don't know why, but this patch certainly fixed it.

So the continuation of this bug will make Drupal look buggy and it will make it hard for module developers to really leverage the Ajax system that is otherwise pretty straight-forward.

aspilicious’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review

1) read the issue again, we need to crossbrowser test #34
2) I would like to have some input from (sun, merlin, eff, quicksketch, rfay, ksenzee, etc.)
3) I would try to push this for D8 and backport it when we are sure it doesn't brake stuff (I can build a D8 site with this patch included)

sime’s picture

Version: 8.x-dev » 7.x-dev

I don't agree with moving a major bug to D8 (unless there is some protocol I've missed). If #34 is problematic, then it should be #44 which isn't a dramatic change.

bleen’s picture

Version: 7.x-dev » 8.x-dev

sime: all bugs are fixed in the newest dev version of Drupal and then backported (when appropriate). This is done to ensure that we dont fix the bug in d7 and then forget to fix it in d8 and thus reintroducing the bug.

sime’s picture

I disagree because #44 is not appropriate for D7 and #34 is not appropriate for D8.

Not changing the version back to D7. #44 remains RTBC for D7 IMO.

mikey_p’s picture

I'm a surprised that this wasn't left at critical. I'm getting this on upwards of 95% of all ajax submissions when testing http://drupal.org/sandbox/mikey_p/1116474. I'm using a Mid 2010 quad core Macbook Pro with FF 4 and Chrome 10.0.648 so I don't think this bug has much to do with the browsers, or server performance.

As it stands, this bug effectively makes D7 ajax unusable. I'm marking the Field Inspector module as abandoned until this is resolved.

rfay’s picture

Priority: Major » Critical

Well then... Now it's critical. Only 15 bugs are tolerated in D8 at a time. So this will have to go in SOON.

mikey_p’s picture

Issue tags: +Needs backport to D7

Tagging

mikey_p’s picture

Its the same patch for 7.x and 8.x at this point, but here's the -p1 version of the patch from #44 marked against D8.

mikey_p’s picture

Note, I've since promoted Field inspector to a full project, but it is a great test case for this issue.

andypost’s picture

Suppose it's better to catch an event of form submit and analyze a triggering element (button) so we got a global handler for form and it's much easy to process all ajax-related events.

pillarsdotnet’s picture

corbacho’s picture

This is important.

pillarsdotnet’s picture

pillarsdotnet’s picture

In includes/ajax.inc, you should remove the @see http://drupal.org/node/216059 line as we don't link to project issues in core.

In misc/ajax.js, please explain why you changed element_settings to ajax on line 185.

sun’s picture

@pillarsdotnet:

1) @see to d.o issues is acceptable in cases like this, when the code comments already try to capture and explain most details of a very complex problem space and when that's not sufficient.

2) The change from element_settings to ajax is correct, the element property is on the ajax object. Not sure how the old code can currently work.

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

@sun -- Then I see no reason not to mark this RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

1. Who tested this, and in what browsers, and what did you test?
2. I need a summary of what the API change is for existing contrib/sites out there to evaluate its viability as a D7 patch.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Things tested:

1. Code review

I read the code from bottom to top:

This patch fixes two issues that could be split in two issues or patches:

* The first one is the issue at hand that forms are submitted and pages reloaded even though ajax should have been used.

The code fixes the issue as additional to binding to mousedown, the click event is prevented by binding it to "false". Mousedown always runs before "click", so there is no problem here.

* The second one is a left over from D6 porting:

-    $(element_settings.element).keypress(function (event) {
+    $(ajax.element).keypress(function (event) {

This is a valid change as this is clearly D6 code and element_settings.element is no longer used or defined in D7/D8 code.

2. Testing and easy reproduction (on Chrome, Firefox, IE 6, 7 and 8)

This can be easily reproduced on Chrome, Firefox, IE 6, 7 and 8:

* Goto: http://d7.drupalexamples.info/examples/ajax_example/submit_driven_ajax

* Press down on the button, wait 4-5 secs (until AJAX call is complete), mouse up
=> The form is submitted

3. Local Testing

* Same test as 2), but with the patch applied the bug no longer is triggered.

It works then reliably for:

Chrome, Firefox, IE 6, 7 and 8

=> Patch does indeed solve the issue.

4. D7 compatibility

* The patch is fully compatible with D7, applies cleanly and solves the problem there, too.

5. API Change

As far as I can see, there is no API change involved (despite the tag saying so). The only thing the patch does is to bind() itself to the submit button via click and evaluates this to FALSE.

When binding a click event to the submit element, there are no changes in behavior with or without the patch.

However: I found out that the click event is not always fired, but that is completely unrelated to the patch discussed here and another completely different issue.

EDIT: Ah, found the possible API change:

Ajax elements do now accept the settings:

   $element['#ajax']['prevent'] = 'click';
   $element['#ajax']['keypress'] = TRUE;

which will be used if $element['#ajax']['event'] is set. If not these will be the defaults.

On JS Level Drupal.ajax also supports:

element_settings.keypress = TRUE;
element_settings.prevent = 'click';

Note however that keypress was already used internally, but undocumented and not in API since D6. And prevent is also used more for internal purposes as it does not neccessarily apply if someone uses its own "event" handler.

So I'd still say this is RTBC with no API change - despite internally using new functionality, which is not officially exposed.

=> RTBC of #53.

Best Wishes,

Fabian

PS: Edit: Performed tests for IE 6,7, and 8 with and without patch. Without patch: Bug is triggered. With Patch: Bug is resolved.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Awesome, thanks for the testing Fabianx! Glad to hear that this fixes the problem in all the major browsers.

Could someone please also test this patch with an AJAX-heavy interface, such as Views/Panels in contrib? It'd also be nice to have sign-off on this from someone like merlinofchaos, actually, who's notably absent in this thread.

7.2 broke CTools with fatal errors everywhere, and I'm not keen to have 7.3 introduce more problems.

pillarsdotnet’s picture

merlinofchaos’s picture

Question: What happens if, in #ajax, I set the event to 'click'. Is click both bound and prevented? That seems like it could be potentially silly, at least.

Views is using 'click' as the event on previews, so testing this with the Views preview and exposed filters during preview with the latest Views release is probably a good idea. Should be an easy test, I bet someone has Views handy...

Fabianx’s picture

> Question: What happens if, in #ajax, I set the event to 'click'. Is click both bound and prevented? That seems like it could be potentially silly, at least.

As spoken in IRC: This part of the code is only reached if $element['#ajax']['event'] is unset.

> Views is using 'click' as the event on previews, so testing this with the Views preview and exposed filters during preview with the latest Views release is probably a good idea. Should be an easy test, I bet someone has Views handy...

I did a quick test, but views is not using the default event (mousedown + prevent), but click, so it is not affected by this patch.

The above has been discussed with merlinofchaos in IRC:

Fabianx: merlinofchaos: Okay, I am then gonna state that in the issue and do one more test with poll module, which was linked to in other issues. Do you else see anymore problems with this patch (besides that it is not just using click() in the first place ...)?
merlinofchaos: Fabianx: Nope.

So there should be one more test with poll module (which seemed to have given the most problems with using click as default event) and we should be good to go.

@aspilicious: You think you could do another review with poll module?

Best Wishes,

Fabian

aspilicious’s picture

I followed steps from http://drupal.org/node/216059#comment-721125

1) Enable the poll module and create a new poll.
2) Move to the title field and hit the enter button.
3) A new element will be added to the AHAH add more section.

This still gives the same problems, I used the patch webchick made in #1004742: Change #ajax event 'mousedown' into 'click'

I also noticed something else (probably not related)
1) Pushing the enter button made the row weights appear + drag and drop icon => drag and drop broken
==> pressing add more fixes the issue

These are all fixed.

So I'm going to rtbc this.

PS:
Poll module sux big time, I just added three choices and after that I wanted to delete one
1) there is no option te remove one
2) if you leave on blank and click on save, it just ignores the blank and republishes the old option. (not so good)
3) in preview the blank is gone... O_o (expected result)
Probably not related so I'm going to open a new issue.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

K, I'm a little confused by #68 and whether that's caused by this patch or not, but seems to not be.

Sorry, folks but I'm going to hold this patch for 7.3, and aim to get it into the next point release instead, just so we have a bit more lead time to test things. Thanks a lot for the testing though; hopefully my paranoia is for nothing. ;)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, let's try this, now that we have a month to figure out if it breaks something.

Committed to 8.x and 7.x. Thanks!

jhodgdon’s picture

This issue is marked "API change". Is there an API change that needs to be documented in update docs -- 6.x to 7.x?

jhodgdon’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation updates

Status change, until the question of whether an API change needs to be documented is decided (I don't want it to vanish).

aspilicious’s picture

jhodgdon’s picture

That looks reasonable. So this API change was introduced in 7.3/7.4?

aspilicious’s picture

This patch is for 7.5 or 7.6

But #63 ==> point 5 says there isn't a real api change (external).
So I think we are done.

mikey_p’s picture

No, no. This patch doesn't touch the change from 'clicked button' to 'triggering element' and it is *not* in 7.4.

This was a problem because in some cases, a regular click could be registered after the mousedown, and the form would submit twice, once with ajax, and another without ajax, causing a redirect, page load, etc. Here we prevent the second click event from doing anything since we can't prevent the event from firing.

The change that this patch makes is adding a new property to AJAX settings, called 'prevent' and any event that is set in this property will be bound to 'false' when the JS for ajax run on a given form. This property defaults to 'click' meaning that while the ajax event is bound to mousedown to start with, if a click event should happen, even by accident, it will be found to false, and not trigger an inadvertent form submission.

pillarsdotnet’s picture

Does this new "prevent" property need to be documented on one or more of the following?

sun’s picture

Not Form API.

We need a follow-up patch that adds docs for 'prevent' to http://api.drupal.org/api/drupal/includes--ajax.inc/group/ajax/7

I've no idea whether we have D7->D7 update docs, but if we have, we can add something to that.

We don't need D6->D7 update docs, since the AJAX framework did not exist in D6.

pillarsdotnet’s picture

Here is a patch to the Forms API Reference for consideration.

EDIT: This patch applies to the 8.x-1.x branch of the Documentation project.

pillarsdotnet’s picture

Sorry about the crosspost, but other #ajax properties are documented in the Forms API, so I used the same format to document the #ajax['prevent'] one as well.

I don't see a similar example to reference in the ajax.inc file, so I'll leave that to someone smarter than me.

webchick’s picture

Title: Various problems due to AJAX binding to mousedown instead of click » Document the new "prevent" property of #ajax settings.
Project: Drupal core » Documentation
Version: 8.x-dev »
Component: ajax system » API documentation files
Status: Needs work » Needs review

We do have D7 -> D7 upgrade docs. See http://drupal.org/node/1204572. So this should be created as a sub-page of http://drupal.org/update/modules/6/7 for now.

But hopefully VERY SOON we can be done with janky handbook crap for this though and move to API change nodes.

pillarsdotnet’s picture

So the Forms API Reference will disappear, or be no longer maintained? What will replace it?

webchick’s picture

Not related to FAPI reference. This is just in relation to documenting API changes between versions.

pillarsdotnet’s picture

Posted http://drupal.org/node/1206676

Please make corrections.

pillarsdotnet’s picture

@#84 -- So we document some of the #ajax settings but not others? Why?

EDIT: After discussion on IRC, I realize that webchick's janky handbook crap response in #82 and Not related to FAPI reference response in #84 had nothing to do with the patch in #80. Since other #ajax settings are documented in the FAPI, this one should be also, notwithstanding sun's Not Form API comment in #79.

rfay’s picture

jn2 is pretty much maintaining the Form API reference, which is part of the Documentation project. We're hoping for a better future for that information. The FAPI doc *is* maintained.

rfay’s picture

Title: Document the new "prevent" property of #ajax settings. » Various problems due to AJAX binding to mousedown instead of click
Project: Documentation » Drupal core
Version: » 8.x-dev
Component: API documentation files » ajax system
Status: Needs review » Needs work

Changing the name and the project back.

Please, oh please, do not take things that have a long history and suddenly repurpose them and change their title. That's called hijacking. Create a new issue when you need to do some work on a doc.

We need to preserve our history!

pillarsdotnet’s picture

Summary of Documentation Changes

jhodgdon’s picture

Title: Document the new "prevent" property of #ajax settings. » Various problems due to AJAX binding to mousedown instead of click
Project: Documentation » Drupal core
Version: » 8.x-dev
Component: API documentation files » ajax system
Status: Needs review » Needs work

Thanks for the summary... So what about this, referenced above:
http://drupal.org/update/modules/6/7#clicked_button
Is that legit? That was a 6/7 change I guess?

pillarsdotnet’s picture

The change referenced in http://drupal.org/update/modules/6/7#clicked_button did not happen as a result of any patch submitted within this issue.

pillarsdotnet’s picture

Also, $form_state['clicked_button']is documented as deprecated in the Form generation page:

  • 'triggering_element': (read-only) The form element that triggered
    submission. This is the same as the deprecated
    $form_state['clicked_button']. It is the element that caused submission,
    which may or may not be a button (in the case of Ajax forms.) This is
    often used to distinguish between various buttons in a submit handler,
    and is also used in Ajax handlers.

This documentation was added by #802746: Document $form_state and form builder function in form_api group

sun’s picture

Status: Needs work » Needs review
FileSize
1.79 KB

http://drupal.org/update/modules/6/7#clicked_button refers to #1049462: Usage of deprecated $form_state['clicked_button'] causes bugs during AJAX submissions by non-buttons, not related to this issue. I've added a link, although it wasn't the original issue that introduced the change.

Anyway, the docs for ajax.inc are most important, since that is where developers look up the available #ajax properties and their meaning.

Attached patch documents the mess.

pillarsdotnet’s picture

Hmm... Somebody deleted #93 which I spent an hour on researching and composing. Wonder why?

rfay’s picture

#93 was unpublished... I see no reason why, so published it.

jhodgdon’s picture

RE #94 - someone needs to give that a technical review -- out of my area of expertise.

I do have one style comment. After the first sentence of the #ajax['prevent'] list item, you launch into what seems to be an example of why this would be useful (which is excellent and fairly easy to understand).

But I found it a bit confusing the first time I read it -- it would have been easier for me to follow if that second sentence started with "For example, ", so I knew I was launching into an example instead of a straight explanation.

pillarsdotnet’s picture

Status: Needs review » Needs work

@#94:

For this case, 'prevent' can be set to 'click',

It should be noted that #ajax['prevent'] is set to 'click' by default, and thus the form developer need not set it explicitly in the form generation function.

sun’s picture

Status: Needs work » Needs review
FileSize
2.44 KB

Reworded, and fixed the code to actually be a default only, not overriding a possibly existing value.

Lastly, also noted that multiple events can be prevented by separating them with spaces. See http://api.jquery.com/bind/

pillarsdotnet’s picture

jhodgdon’s picture

+1 for the documentation changes. I have no comment on the code changes in this patch, not having been involved in this issue.

emackn’s picture

is there a consolidated patch for Drupal 7.4? I'm applying the patch from #53 and #99 with latest examples module code and submits are not being handled by ajax. Did i miss something else?

pillarsdotnet’s picture

is there a consolidated patch for Drupal 7.4?

No, but feel free to contribute one. ;-)

I'm applying the patch from #53 and #99 with latest examples module code and submits are not being handled by ajax. Did i miss something else?

Most likely, yes. The patches in #53 and #99 do not have anything to do with submits not being handled by ajax. If you can describe your problem with a repeatable test case, please do so in a separate issue. If it turns out that I am mistaken, then someone will respond by closing that issue as a duplicate of this one.

emackn’s picture

Right, I created #1212420: Submit driven ajax example not working, which was closed as a duplicate. And was pointed to here, so maybe it needs to be reopened.

rfaytest’s picture

@pillarsdotnet, if you read the OP, you'll see that it's probably the issue being pointed out in #1212420: Submit driven ajax example not working. However, I believe emackn just has to use the current version of drupal. Case closed.

pillarsdotnet’s picture

@rfaytest: The OP says, in part:

Always the #ajax action fires

But emackn reports in #1212420 that his #ajax action does not fire. Unfortunately, there is not enough information in that issue to troubleshoot his problem, so I encouraged him to provide more information.

webchick’s picture

This patch just needs sign-off on the docs at #99 and then we can close a critical.

rfay? Fabianx? effulgentsia? Any of you available? :)

jhodgdon’s picture

Actually, I already signed off on the docs. We are waiting for a sign-off on the code (see #101).

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.9 KB

The entirety of the code change is this chunk:

diff -u b/includes/ajax.inc b/includes/ajax.inc
--- b/includes/ajax.inc
+++ b/includes/ajax.inc
@@ -650,7 +652,9 @@
         // an additional handler to prevent such a click from triggering a
         // non-Ajax form submission. This also prevents a textfield's ENTER
         // press triggering this button's non-Ajax form submission behavior.
-        $element['#ajax']['prevent'] = 'click';
+        if (!isset($element['#ajax']['prevent'])) {
+          $element['#ajax']['prevent'] = 'click';
+        }
         break;

       case 'password':

Not that my opinion counts for anything, but the code change is obviously correct.

sun’s picture

@webchick: #99 already clarified: "fixed the code to actually be a default only, not overriding a possibly existing value."

In other words, the code change fixes the (possibly not yet reported) bug that you cannot set/use a custom #ajax['prevent'] on buttons, because it is always overridden with the "default".

catch’s picture

Priority: Critical » Major

That means it's definitely not a critical bug then, given the original bug was fixed June 30th.

pillarsdotnet’s picture

Priority: Major » Critical

When a bugfix is committed without documentation, the issue priority does not change until the documentation is also committed.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. Committed and pushed #99 to 8.x and 7.x.

jhodgdon’s picture

Priority: Critical » Normal
Status: Fixed » Needs work

Back to Needs Work for documentation. Do we need update docs for this, and if so, are they complete, and which version to which version do they apply to?

pillarsdotnet’s picture

Status: Needs work » Fixed

(updated) Summary of Documentation Changes

Unless someone has specific problems with any of the above, I'd say that the documentation is done.

jhodgdon’s picture

Status: Fixed » Needs work

2nd bullet item is marked "stub" and "incomplete". This doesn't seem like it is "fixed"?

pillarsdotnet’s picture

2nd bullet item is marked "stub" and "incomplete". This doesn't seem like it is "fixed"?

Jennifer, all the necessary technical information is there. I left it "incomplete" because it's just a cut-and-paste from the docs sun wrote elsewhere. If you have specific suggestions for how that page could be better formatted, please make them. Otherwise, I fear this issue will stay in a "needs work" status forever.

jhodgdon’s picture

Thanks for finally explaining that. Not being an expert in this issue (all 90+ comments on it) or on AJAX, I had no idea why it was marked "incomplete" or why it wasn't added to the correct module doc page.

Now all we need to do is decide whether the 7.x-7.x changes belong in the 6/7 module update guide, or as Change nodes, and do the appropriate thing.

pillarsdotnet’s picture

Now all we need to do is decide whether the 7.x-7.x changes belong in the 6/7 module update guide, or as Change nodes, and do the appropriate thing.

According to your response here, we are not creating change nodes for 7.x-7.x changes. Have you changed your mind, or did I misunderstand your response in that issue?

webchick’s picture

We handled the node access change in 7.3 as sub-page to the gargantuan one: http://drupal.org/node/1204572 We could do the same here, though the reason we did that there was that API change nodes didn't exist at the time.

I don't think there's any value at all in back-porting all 200 or whatever 6.x => 7.x changes. But making 7.x => 7.x changes API change nodes seems to make sense to me, since they'd be far more visible than a sub-page of a gargantuan page.

jhodgdon’s picture

There's also one 7.x-7.x change on the gargantuan page, I think -- a 7.4 change or something.

I'm fine with the 7.x-7.x changes being change nodes -- it's not just my decision, it's a community here. :) We would only need to take about 3 existing changes now and make them nodes, and I think the change nodes system is far superior.

So, I vote with webchick, and I would prefer it if someone who understands this change can make the change node please:
http://drupal.org/node/add/changenotice
(it should hopefully be fairly self-explanatory, and after making it there will be a link to it in the right sidebar).

Thanks!

jhodgdon’s picture

We still need a change node for this (see comment above). Tagging properly.

pillarsdotnet’s picture

Status: Needs work » Fixed
Issue tags: -Needs change record

Status: Fixed » Closed (fixed)
Issue tags: -API change, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.

cmonnow’s picture

Issue summary: View changes

This is a very old issue but does anyone else think it's unusual/unexpected that the right mouse button would trigger AJAX requests every time due to binding of the mousedown event instead of clicks? I don't know if checking if (eventData.which === 1) using jQuery should be implemented to improve UI? Maybe it's just my developer's console/right-clicker's obsession...

For a quick example, right click the Upload button on this page.

I can't think of any popular non-Drupal websites that react the same way (and Facebook, my go-to example, uses a lot of AJAX).

cmonnow’s picture

Status: Closed (fixed) » Needs work
andypost’s picture

Status: Needs work » Closed (fixed)
Issue tags: +JavaScript
studiotwelve’s picture