AJAX behaviors cannot be bound once on links

+++ misc/ajax.js	12 Apr 2010 17:51:26 -0000
@@ -44,6 +44,10 @@ Drupal.behaviors.AJAX = {
+      // should we use bind or one to attach the behavior?

@@ -144,9 +148,9 @@ Drupal.ajax = function (base, element, e
+  // function to bind to the element event

These should be proper sentences, starting with a uppercase letter and ending in a period.

+++ misc/ajax.js	12 Apr 2010 17:51:26 -0000
@@ -44,6 +44,10 @@ Drupal.behaviors.AJAX = {
+        element_settings.bindOnce = true;

@@ -165,7 +169,14 @@ Drupal.ajax = function (base, element, e
+  if (element_settings.bindOnce) {
+    $(this.element).one(element_settings.event, func);
+  }
+  else {
+    $(this.element).bind(element_settings.event, func);

Hm. Isn't there some nifty dynamic method callback construct we could use here? I.e. define "bind" as default bind method, override it via the CSS class, and call something along the lines of:

$(this.element).call(element_settings.bindMethod, function () {
  ...
});

+++ misc/ajax.js	12 Apr 2010 17:51:26 -0000
@@ -144,9 +148,9 @@ Drupal.ajax = function (base, element, e
+  ¶

Trailing white-space here.

107 critical left. Go review some!

subscribing

@katbailey, could you please post a boiled-down module that demonstrates this use-case and allows easy testing of the patch? Since we don't have testing for javascript, we need easy ways to experiment with things like this.

Awesomeness! We need more jQuery.one in Drupal.

subscribe

I apologize for not rolling a patch, but I'm not set up for that on this machine. Nonetheless, here's how to implement a dynamic callback function as discussed in #1 and #2. As Sun suggests, create element_settings.bindMethod which can equal 'bind', 'one' or any other jquery method name as appropriate. Then use js's syntax for tokenizing a string, which is to put it in brackets. So these two method calls:

  $(this.element).one(element_settings.event, func);
  $(this.element).bind(element_settings.event, func);

get replaced by this one

  $(this.element)[element_settings.bindMethod](element_settings.event, func);

Note that the same approach can be used to eliminate the eval statement in #10.

In case the above isn't clear, here's a bit more. Anywhere there's a dot, it can be replaced with brackets, and the brackets can contain any expression (including function calls) which evaluates to a string which is the name of a property or method contained by the object which precedes the opening bracket.

In cases where a variable or function is global, precede the brackets with window. For example:

var foo = 'bar';
var foo1 = window[foo];

+++ misc/ajax.js	21 Apr 2010 18:07:24 -0000
@@ -38,12 +40,18 @@ Drupal.behaviors.AJAX = {
     $('.use-ajax:not(.ajax-processed)').addClass('ajax-processed').each(function () {
...
+      // Check whether the behavior should be bound using .one() instead of .bind().
+      if ($(this).hasClass('ajax-once')) {
+        element_settings.bindMethod = 'one';
+      }

Shouldn't we support .ajax-once for other integration methods than .use-ajax, too?

+++ misc/ajax.js	21 Apr 2010 18:07:24 -0000
@@ -145,8 +156,14 @@ Drupal.ajax = function (base, element, e
-  // Bind the ajaxSubmit function to the element event.
-  $(this.element).bind(element_settings.event, function () {
+  // We must persistently bind the 'return false' behavior to the link
+  // regardless of whether the ajax behavior should persist after one click.
+  $(this.element).bind(element_settings.event, function(){
+    return false;  ¶
+  });
+
+  // This is the ajax event handler that will be bound to the element event.
+  var func = function () {

@@ -163,9 +180,9 @@ Drupal.ajax = function (base, element, e
     }
-
-    return false;
-  });
+  };
+  // Bind the handler to the element event using whichever bind method was set.
+  $(this.element)[element_settings.bindMethod](element_settings.event, func);

1) We do not have to split the function into 'func'; just replace the original line containing bind().

2) To make any sense of the "return false", the second event handler has to be bound after the AJAX event handler, not before. I'd additionally suggest to only conditionally add it, if bindMethod != 'bind'.

3) Anonymous functions should have a space after "function", and there should also be a space before the opening curly brace {.

4) Trailing white-space.

Powered by Dreditor.

+++ misc/ajax.js	26 Apr 2010 04:56:36 -0000
@@ -44,6 +44,10 @@ Drupal.behaviors.AJAX = {
+      // Check whether the behavior should be bound using .one() instead of .bind().

Exceeds 80 chars. (can be shortened)

+++ misc/ajax.js	26 Apr 2010 04:56:36 -0000
@@ -145,8 +149,13 @@ Drupal.ajax = function (base, element, e
+  if (!element_settings.bindMethod) {
+    // The default method for binding behaviors is .bind().

@@ -163,10 +172,16 @@ Drupal.ajax = function (base, element, e
+  if (element_settings.bindMethod == 'one') {
+    // We must persistently bind the 'return false' behavior to the link.

Can we move the inline comment above the if condition? It's easier to read the code flow that way.

Additionally, the second comment just repeats what can be easily read from the following code lines. It should explain the usual "why?" and "WTF?" questions instead. ;)

96 critical left. Go review some!

+++ misc/ajax.js	26 Apr 2010 22:02:47 -0000
@@ -145,8 +149,13 @@ Drupal.ajax = function (base, element, e
+  // The default method for binding behaviors is .bind().
+  if (!element_settings.bindMethod) {

"By default, behaviors are bound with .bind()."

+++ misc/ajax.js	26 Apr 2010 22:02:47 -0000
@@ -163,10 +172,19 @@ Drupal.ajax = function (base, element, e
 
+  if (element_settings.bindMethod == 'one') {
+    // If we don't persistently bind the 'return false' behavior, then elements
+    // whose AJAX behavior was bound using .one() will revert to their default
+    // behavior. For example, clicking on a link that had an ajax behavior will,
+    // after the first click, result in the browser following that link.

Can we move the comment above the condition?

"If the behavior was only bound once, the browser's default behavior needs to be prevented on subsequent events."

("prevented" is not JS jargon in this context, but I just have a blackout, so there's probably a better term.)

Powered by Dreditor.

Better title.

Looks ready to fly for me.

+++ misc/ajax.js	29 Apr 2010 18:04:51 -0000
@@ -27,6 +27,11 @@ Drupal.behaviors.AJAX = {
+
+          // Check whether to bind behaviors using .one() instead of .bind().
+          if ($(this).hasClass('ajax-once')) {
+            element_settings.bindMethod = 'one';
+          }

Why do we need to check for the class here? This is the code block that has #ajax bindings. Shouldn't the PHP code be responsible for adding bindMethod to it?

+++ misc/ajax.js	29 Apr 2010 18:04:51 -0000
@@ -163,10 +177,17 @@ Drupal.ajax = function (base, element, e
-
     return false;
   });
 
+  // If the behavior was only bound once, the browser's default behavior needs
+  // to be prevented on subsequent events, so as to not follow links.
+  if (element_settings.bindMethod == 'one') {
+    $(this.element).bind(element_settings.event, function () {
+      return false;
+    });
+  }
+

Any reason not to remove the first "return false" and always bind the function that just returns false?

Powered by Dreditor.

Also, for the code that must check for class (because links don't have #ajax bindings), what about using a class of ajax-bind-method-METHOD (i.e., ajax-bind-method-one)?

[EDIT: or even crazier, ajax-setting-SETTING-VALUE, so something like ajax-setting-bindMethod-one. Seems like there might be other use-cases for needing settings on links.]

[EDIT: sorry, ignore this comment. Advanced use-cases can use #attached to add settings. Here, we want something easy for a common use-case. Given that, "ajax-once" makes sense.]

Maybe it's just bike shedding, but how about this?

omg - let's not make this even more complicated than it is already, please! :)

(also, I actually hoped for a RTBC here finally as this is just one of the issues that fix ajax with links... ;)

Discussed with sun. The differences between #22 and #25 only start mattering if and when other AJAX patches land. #22 solves this issue completely adequately given current HEAD. Tweaks to the logic can be rolled into the other patches. So, re-uploading #22 and RTBC'ing it.

#27: drupal.ajax-bind-once.22.patch queued for re-testing.

*Retesting old patches*

#27: drupal.ajax-bind-once.22.patch queued for re-testing.

#27: drupal.ajax-bind-once.22.patch queued for re-testing.

I tried rerolling this but the patch can not be applied anymore since #939568: Enable finer control for ajax-using modules of the trigger response in the ajax object got in.

Has to go into D8 first. Shucks. Attaching #35's patch to get it tested on D8.

+1

Herro, patch bot?

Just tested this patch on drupal 7 for some use case we have. Looks to work fine to me.

edit : changed core version, category.

This is a bug and we need to fix bugs first in D8. When it's committed to D8 it will be committed soon to D7.

this is the patch from comment #30 with the path corrected ( core/misc )

+++ b/core/misc/ajax.jsundefined
@@ -173,11 +182,25 @@ Drupal.ajax = function (base, element, element_settings) {
+ ¶

trailing whitespace, in 2 places and there is an unneeded newline.

0 days to next Drupal core point release.

removed newline and whitespace

This need to use e.preventDefault() instead of return false.

Also I'm not really getting the use of .one if you need to do a .bind on it later on. Can't you just do a .bind and have your if inside that?

Better yet have a .bind canceling events for every element and add either a .one or a .bind depending on the class? You can have several listeners for one event.

I think this needs some significant attention. Marking as postponed since we need to confirm this is still an issue as a lot of time has passed in the past 5 years.

@katbailey, Thank you for reporting this problem. We rely on issue reports like this one to resolve bugs and improve Drupal core.

Is this issue still a problem?

There has been no activity on the patch for 10 years and more information was asked for 5 years ago.

Since we need more information to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks

Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more seven year ago and there has been no activity here since that time other than @quietone asking for more information.

Per #61, I'm marking the issue "Closed (cannot reproduce)". If anyone can provide complete steps to reproduce the issue (starting from "Install Drupal core"), document those steps in the issue summary and set the issue status back to "Active" [or "Needs Work" if it has a patch, etc.].

Thanks!

Whoops. Forgot to close but... @klonos noticed that this could be moved to D7 queue.