When using modules that make additional calls to Drupal.attachBehaviors() the simpletest JS behaviors may be attached multiple times leading to unexpected behaviors and/or breakage (e.g. 2 toggles per click which leads to the same state).

This patch adjusts the JS behaviors so that they attach only once against the DOM elements. See http://drupal.org/node/114774#javascript-behaviors. Patched against CVS checkout of DRUPAL-6--2.

#8 drupal-642734-8.patch3.91 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,153 pass(es).
[ View ]
#4 core-js-simpletest-table-642734-4.patch3.93 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 34,218 pass(es).
[ View ]
simpletestBehaviorFix.patch5.06 KByhahn
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletestBehaviorFix.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]


Damien Tournoud’s picture

Title:Ensure JS behaviors attach only once» Simpletest behaviors should process the elements only once
Project:SimpleTest» Drupal core
Version:6.x-2.x-dev» 7.x-dev
Component:Code» simpletest.module
Status:Needs review» Patch (to be ported)

Indeed, but this needs to be fixed in core first, then backported.

TR’s picture

Version:7.x-dev» 8.x-dev
oriol_e9g’s picture

Status:Patch (to be ported)» Needs work

We need a patch cleanup (coding standards, docs, proper comments...)

nod_’s picture

Status:Needs work» Needs review
new3.93 KB
PASSED: [[SimpleTest]]: [MySQL] 34,218 pass(es).
[ View ]

That's surprising that nobody ran into this in a couple of years. It bitted me working on #1423500: Use RequireJS to load all JS.

added: .once, declared a leaking variable, re-scoped part of the code to make it work.

It's just a working patch, not a pretty one. Ideally it should be rewritten, having a variable animation time is really confusing.

nod_’s picture

Issue tags:+JavaScript clean-up


sun’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+needs backport to D7

Coolio, thanks!

Shameless plug: I'm experimenting with some SimpleTest UI improvements in http://drupal.org/project/admin_ux - users + reviews + whatnot + ultimately core patches based on that welcome! ;)

Lastly, this should be backported.

catch’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed/pushed to 8.x, moving to 7.x for backport.

@nod_ I'm surprised you're surprised ;)

tim.plunkett’s picture

Status:Patch (to be ported)» Needs review
new3.91 KB
PASSED: [[SimpleTest]]: [MySQL] 39,153 pass(es).
[ View ]


mgifford’s picture

#8: drupal-642734-8.patch queued for re-testing.

oriol_e9g’s picture

Status:Needs review» Reviewed & tested by the community

Yeah! Same as D8.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/e0a0b94

Not sure if we want to follow up with this in the Drupal 6 SimpleTest module queue also (where this issue started originally).

David_Rothstein’s picture

Also, I noticed that as a result of the patch this line of JavaScript now appears twice:

      var direction = settings.simpleTest[this.id].imageDirection;

Seems harmless, but probably a mistake, so perhaps it's worth a quick followup (Drupal 8 also).

nod_’s picture

Actually no it's required, if we remove the first one we can't get the right image. And if we remove the other one the group can't be toggled back up.

More cleanup would be rewritting it from JS settings to better HTML.

David_Rothstein’s picture

Ah, yes, thanks - I missed that the setting was being modified again 20 lines later in the function :)

Status:Fixed» Closed (fixed)
Issue tags:-needs backport to D7, -JavaScript clean-up

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