Core js still uses -processed instead of context/.once()

openid.js
color.js
ajax.js

Don't see other incorrect uses beside in those files. Not really a major task either. Much more important, and broken, things to take care of in JS-land.

here it is, i'll have to reroll 2 or 3 patches because of this one so that'll be nice if it doesn't takes forever to gets this in.

stuff changed since last month, will need a reroll.

It was #1419968: Replace $('selector', domelement) with $(domelement).find('selector') that broke it, here's a reroll.

thanks :)

All good, i'd RTBC if I could…

Talked with litwol about this on IRC, the color and openid parts are busted. I think he'll be following up.

and ajax selector part is busted :). artifact from d7 days that was not caught. will follow up shortly. need to write test module :-\

the color thing was backward, but the rest is working, can you describe the issue? looks like it works to me.

See my patch for test module i wrote to illustrate where drupal.behaviors.ajax.attach work as expected and when not.
Be advised that this module is showing pre-patched ajax.js behaviors. Do not apply your patch from above to it when testing.

I've left comments in the module file that explain the problems.

First work in progress patch to fix bad selector/killswitch.

This patch fixes discrepancy between element_settings.selector and base variables. With introduced changes .once() killswitch now works as expected.

Strictly speaking the "base" variable that we keep passing around (such as into Drupal.ajax(base,...)) is rather useless when compared to element_settings.selector. However i left base in for the time being to reduce the scope of the patch, which originally is just making core switch to .once() uniformly. We should open another issue to reexamine use of "base" (and ajax.js) in general for "makes no sense" functionality.

Attached patch is the merge between #8 and #10.

@todo:
fix original logic behind openid.js

diff --git a/core/modules/openid/openid.js b/core/modules/openid/openid.js
index 0e673f3..6b43cd1 100644
--- a/core/modules/openid/openid.js
+++ b/core/modules/openid/openid.js
@@ -8,20 +8,20 @@ Drupal.behaviors.openid = {
     var cookie = $.cookie('Drupal.visitor.openid_identifier');
 
     // This behavior attaches by ID, so is only valid once on a page.
-    if (!$('#edit-openid-identifier.openid-processed').length) {
+    $('#edit-openid-identifier').once('openid', function () {
+      var $that = $(this);
       if (cookie) {
-        $('#edit-openid-identifier').val(cookie);
+        $that.val(cookie);
       }
-      if ($('#edit-openid-identifier').val() || location.hash == '#openid-login') {
-        $('#edit-openid-identifier').addClass('openid-processed');
+      if ($that.val() || location.hash == '#openid-login') {
         loginElements.hide();
         // Use .css('display', 'block') instead of .show() to be Konqueror friendly.
         openidElements.css('display', 'block');
       }
-    }
+    });

Lets tackle Drupal.ajax afterwards and make it more flexible/useful: #1533366: Simplify and optimize Drupal.ajax() instantiation and implementation

Attached patch is continuation of #11, with fix to the openid.js. ("fix" in a sense that original patch broke logic flow with straight convert to .once() killswitch. this fix uses .once() while maintaining old logic flow).

works for me!

I got so confused about this patch and the final working patch looks nothing like mine so I'm taking the liberty to rtbc it. We talked for hours about this on IRC i don't think we missed something.

Works for me as well, great work.

Attached a patch for Drupal 7 so this can get backported.

Any D7 patches should be suffixed with -do-not-test until this is into D8.

Back to RTBC for #12.

sorry about that.

Wondering if #14 will just work now.

Nope, needed to be reuploaded. Just retitled version of @attiks's patch in #14.

+++ b/core/misc/ajax.js
@@ -21,23 +21,18 @@ Drupal.behaviors.AJAX = {
-      if (!$('#' + base + '.ajax-processed').length) {
...
-        $('#' + base).addClass('ajax-processed');
...
+      $(element_settings.selector).once('drupal-ajax', function() {

1) Wrong .once() identifier 'drupal-ajax' instead of 'ajax'. This means that dependent JS and CSS no longer applies.

2) This is a change in behavior, which I do not see being discussed in this issue, so it looks like it was unintentionally overlooked: The Ajax behaviors were previously attached to #base instead of .selector. This means that Ajax behaviors are now bound to the .selector instead of the triggering #base. Which in turn means that whenever an unprocessed .selector appears in the DOM, Ajax behaviors are attached to it. The previous behavior potentially did not re-execute, because the primary condition was on #base.ajax-processed, regardless of whether there is a .selector or not. This needs more discussion and in-depth reviews from Ajax framework authors and maintainers.

3) While touching that selector logic, we could have fixed the missing usage of context as well. However, that also requires testing and discussion.

4) Missing space after "function" in (anonymous) function().

+++ b/core/modules/openid/openid.js
@@ -8,23 +8,24 @@ Drupal.behaviors.openid = {
+      $edit_openid_identifier.once('openid', function() {

Missing space after "function" in (anonymous) function().

1) Wrong .once() identifier 'drupal-ajax' instead of 'ajax'. This means that dependent JS and CSS no longer applies.

1. I've attached patch to revert back to original "ajax"[-processed] lock as it was dev artifact evaluating whether it is better to namespace once() locks with "drupal-" or not. I think simply "ajax" is too short and may conflict with other css/js libraries, but then again.. maybe not.

2) This is a change in behavior, which I do not see being discussed in this issue, so it looks like it was unintentionally overlooked: The Ajax behaviors were previously attached to #base instead of .selector. This means that Ajax behaviors are now bound to the .selector instead of the triggering #base. Which in turn means that whenever an unprocessed .selector appears in the DOM, Ajax behaviors are attached to it. The previous behavior potentially did not re-execute, because the primary condition was on #base.ajax-processed, regardless of whether there is a .selector or not. This needs more discussion and in-depth reviews from Ajax framework authors and maintainers.

2) This was deliberate change, rather than overlooked. See patch in #9 that illustrates the /bug/ in d7 ajax.js attach behaviors. it was explained to me that ajax framework was originally intended for form api (which provides #base, always?), and that was good for the time. Now we provide ajax support for anything (links, divs, map area, images, buttons, ...anything...) that can be interacted with (clicked, for sake of argument) or more advanced have "interaction" event triggered by some other javascript (that is client/project specific). Assuming that a to-be-ajax element always to have #base is simply wrong, that is artifact days. Perhaps core doesn't make full use of this, but that does not mean project/client specific tools are not being built that rely on the more flexible element_setting.selector (i do, hence the test module patch in #9).

3) While touching that selector logic, we could have fixed the missing usage of context as well. However, that also requires testing and discussion.

3) elaborate please. are you referring to something like this: http://drupal.org/node/1509540#comment-5818186 ?

4) Missing space after "function" in (anonymous) function().

4) Fixed in attached patch.

I realize now i'm not sure what sun meant in his 4th point. I've grepped other js files in ./core/misc/*.js and use of anon "function() {" seems to be identical. Elaborate please. in the mean while i'm attaching patch that takes care of first point from http://drupal.org/node/1473314#comment-5887008

+++ b/core/misc/ajax.jsundefined
@@ -25,7 +25,7 @@ Drupal.behaviors.AJAX = {
+      $(element_settings.selector).once('ajax', function() {

I think he means it should be function ()

See file. thoughts? I am all in favor of "standardizing" this. However this is scope creeping on the original issue. please do not high jack. Open new issue for this "cleanup".

1/ patch in #23 is looking good

2/ are there any examples that rely on the old behavior, i skimmed through core and couldn't find anything that can break with the change from #12, but it can just be me

3/ i think this can be done in a follow up issue

4/ i think we better clean this in a separate patch

@litwol, you're catching jquery with your command. I find 5 use for our code in this folder, the rest is jQuery/UI

In all core files (including the 5 from before): 14 times.

for function () with a space: 190. Needs to be fixed in this patch.

re 2) It's suboptimal to squeeze in a fix for a functional bug into a code clean-up patch like this. It also means the fix is not backed by automated tests (if possible). After looking at the example module code in #9, the change looks correct, but I'm not 100% sure. This needs extensive testing (with existing contrib modules) and reviews before it can be backported.

re 3) I meant to change $(selector) into $(selector, context). However, let's do that in a separate issue.

re 4) See JavaScript coding standards section about functions (anonymous functions).

@sun: i created a test for the new patch, so failing for the moment: http://testswarm003.h001.attiks.com/drupal-ajax-1473314?testswarm-test=t... but i do understand your concern.

Do you (or anyone else) knows about contrib modules that might break?

I tried testing this (on drupal 7, patch attached) with views, ctools, panels but everything behaves the same as without the patch.

all editing works the same as without the patch
view with exposed filter + pager and ajax enabled
panel
mini-panel
page
ctools AJAX demo

patch of #30

the open id part of it was fixed separately.

Not anymore. Fixed by jquery once update to 2.0

I guess this is still worth backporting, if it won't break anything.