I'm proposing using javascript events instead of Drupal.behaviors.

Problems

  • It's a big Drupalism
  • Drupal.behaviors makes monkey-patching a recommended way to change an init function.
  • It's not flexible enough, contrib will build things that way and it ends up a terrible mess (see openlayers init function for an example). Using events would have helped dev to do the right thing.

Details

  • The context variable already exists in event.target. Nothing special to be done here.
  • It's possible to leverage custom events or to namespace events to allow scripts to bind a listener to the specific event it needs to react to.
  • We can pass additional data to the event callback, to add the settings and trigger values we have right now with behaviors.

Benefits

  • Remove a drupalism and help JS dev get into Drupal JS.
  • Much more flexible.
  • Loosely coupling is a better way to deal with this.

Also if we're considering the AMD proposal #1542344: Use AMD for JS architecture, there won't be a need to "weight" the behaviors or to play with the weight option of drupal_add_js to have them run at the proper time. AMD takes care of dependencies and making sure are executed in the right order.

Files: 
CommentFileSizeAuthor
#23 core-js-no-behaviors-1446166-23-nowhitespace.patch50.28 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,355 pass(es).
[ View ]
#21 core-js-no-behaviors-1446166-21-nowhitespace.patch49.48 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]
#19 core-js-no-behaviors-1446166-19-nowhitespace.patch49.48 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 35,378 pass(es).
[ View ]
#13 core-js-no-behaviors-1446166-13.patch155.9 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-no-behaviors-1446166-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 core-js-no-behaviors-1446166-13-nowhitespace.patch50.32 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-no-behaviors-1446166-13-nowhitespace.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

nod_’s picture

Title:Replace Drupal.attachBehaviors by events.» Use JS events instead of Drupal.behaviors
Damien Tournoud’s picture

Not sure I'm following all of this. Yes, Drupal.behaviors is basically a small event system. Yes, it could be replaced by a custom event. But...

  • Custom events are not core Javascript and requires a library (like jQuery)
  • Binding an event to a DOM element that doesn't exist yet requires a library (like jQuery delegated events)

So it seems that this proposal directly conflicts with #1541860: Reduce dependency on jQuery.

nod_’s picture

Custom events are not core JS but it's something any JS dev (or anyone doing JS for a couple of days) would understand right away, Drupal.behaviors isn't.

Events bubbles, what jquery delegate does is pretty much filter the target to see if it matches the selector given when the event was bound.

Well, there are 3 issues. And they are loosely coupled, I'm assuming jQuery is available here because it's still in core and that could help people getting and thinking about what this issue is about. It doesn't directly confilcts with the jQuery issue but yes, it might make it harder. As long as it doesn't make it impossible we're good.

RobLoach’s picture

Drupal's JavaScript behavior system was introduced before .live(), so behaviors were needed at the time. jQuery has grown enough that the behavior system is actually detrimental to site performance when other more appropriate events could be used instead.

cosmicdreams’s picture

I'm confused, can you describe what the fix is here?

Damien Tournoud’s picture

As soon as we have jQuery and can rely on delegated events (.live() and the new .on()), I'm fine with this.

This said, this might make implementing weights more difficult (#1302126: Weights for Drupal.behaviors).

cosmicdreams’s picture

After talking with nod_ on irc the two strategies we can deploy to resolve this issue are:

  • change the code of attachBehaviors and detachBehaviors to $(context).trigger('attach.drupal') / trigger('detach.drupal'); or something
  • instead of having Drupal.behaviors.myBehavior it's $(document).bind('attach.drupal', function (e) { var $context = $(e.target); });

If I have time today I'll give that a go for a small change.

nod_’s picture

Don't need weights if we have dependencies: #1542344: Use AMD for JS architecture.

Also before when I say "change the code" it's "replace the method" really.

jessebeach’s picture

+1000 to this idea. Drupal.behaviors is a huge impediment to JS devs new to Drupal.

ericduran’s picture

Yea, this is probably the 1st and probably most important patch that should get in regarding JS refactoring.

As someone that's constantly helping new Drupal dev it's a pain trying to convinced them to start using Drupal.behaviors in there JS. Especially if the JS developer is an experience/novice JS developer they rather just implement there own event system, or use jquery because it's already forced on every page :-/ (be it I know that's a separate issue).

Anyways just giving my +1 on this issue.

mfer’s picture

@nod_ So, you've put the idea out there. How about a patch?

nod_’s picture

Haha yeah I know! I was busy with the two others, and I do have a job.

It's on the way, I was actually waiting to see if someone was up for it. Might as well help someone to make the patch and I'd get to rtbc a patch for once.

In any case that's in the very near future :)

nod_’s picture

Status:Active» Needs review
StatusFileSize
new50.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-no-behaviors-1446166-13-nowhitespace.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new155.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-no-behaviors-1446166-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here you go.

Status:Needs review» Needs work

The last submitted patch, core-js-no-behaviors-1446166-13-nowhitespace.patch, failed testing.

sun’s picture

I'd suggest to keep going without whitespace changes, as long as we're discussing high-level issues for this patch.

+++ b/core/misc/ajax.js
@@ -17,8 +17,9 @@ Drupal.ajax = Drupal.ajax || {};
-Drupal.behaviors.AJAX = {
-  attach: function (context, settings) {
+$(document).on('ready attach', function (e, settings) {
+  var context = e.target;
+  settings = settings || Drupal.settings;

I'd like to see a replacement helper instead of .on() that performs this target/context and settings handling, and potentially other things once for all behaviors.

+++ b/core/misc/drupal.js
@@ -6,115 +6,13 @@ jQuery.noConflict();
- * @param trigger
- *   A string containing what's causing the behaviors to be detached. The
- *   possible triggers are:
- *   - unload: (default) The context element is being removed from the DOM.
- *   - move: The element is about to be moved within the DOM (for example,
- *     during a tabledrag row swap). After the move is completed,
- *     Drupal.attachBehaviors() is called, so that the behavior can undo
- *     whatever it did in response to the move. Many behaviors won't need to
- *     do anything simply in response to the element being moved, but because
- *     IFRAME elements reload their "src" when being moved within the DOM,
- *     behaviors bound to IFRAME elements (like WYSIWYG editors) may need to
- *     take some action.
- *   - serialize: When an Ajax form is submitted, this is called with the
- *     form as the context. This provides every behavior within the form an
- *     opportunity to ensure that the field elements have correct content
- *     in them before the form is serialized. The canonical use-case is so
- *     that WYSIWYG editors can update the hidden textarea to which they are
- *     bound.
...
-Drupal.detachBehaviors = function (context, settings, trigger) {
...
-        this.detach(context, settings, trigger);

As the JSDoc clarifies, there are different sub-types of detach - the sub-types are absolutely required for certain scripts/modules (WYSIWYG editors in particular).

The concept has to be retained.

+++ b/core/misc/machine-name.js
@@ -84,7 +81,7 @@ Drupal.behaviors.machineName = {
-              machine = self.transliterate($(this).val(), options);
+            machine = transliterate($(this).val(), options);

@@ -123,10 +119,9 @@ Drupal.behaviors.machineName = {
-  transliterate: function (source, settings) {
+function transliterate (source, settings) {

It is not a good idea to destroy the notion of the current (behavior) objects - I want to see attach and detach and any related functions for a behavior grouped/bundled in some sane way.

Closely related to that, the function transliterate cannot be overridden by a module (in particular Transliteration module) anymore.

sun’s picture

Issue summary:View changes

updated after frontendunited

merlinofchaos’s picture

In just reviewing the idea, these are my thoughts:

Pros

Easier to understand for pre-existing javascript developers.
More consistent with practices javascript already uses.

Cons

Does not provide much in the way of actual benefit other than nicer DX, at the expense of requiring everyone to rewrite a bunch of javascript.
Will never be able to use a weighting system.

Unknowns

Which one actually runs faster? I would love to see some benchmarks. It should be very easy to set up a rigged test.
Can events handle bad code in a single event better than behaviors? i.e, if behavior C crashes, will that prevent behavior D from running?

More or less an event triggered system will work the same way that the old behavior system worked. The 'context' variable in behaviors is more or less broken anyway, and has proven to be an unreliable thing to use, so cleaning that up seems valuable. And I prefer the syntax of events.

I'm not 100% sure that this is compelling enough to require contrib developers to rewrite a lot of code, though demonstrating that javascript developers actually have trouble figuring out behaviors could assist in showing the worth.

neclimdul’s picture

Yeah, thanks for the nowhitespace patch. really helpful with the conceptually unrelated whitespace changes. Couple of nits:

+++ b/core/misc/drupal.jsundefined
@@ -374,22 +272,15 @@ Drupal.ajaxError = function (xmlhttp, uri) {
-$('html').addClass('js');
+document.documentElement.className += " js";

Not related?

+++ b/core/misc/drupal.jsundefined
@@ -374,22 +272,15 @@ Drupal.ajaxError = function (xmlhttp, uri) {
-$('html').addClass('js');

+++ b/core/modules/user/user.permissions.jsundefined
@@ -4,7 +4,7 @@
Drupal.behaviors.permissions = {
-  attach: function (context) {
+  attach: function (e) {

Not finished?

nod_’s picture

#15

1. So we'll be replacing behaviors by… events wrapped in some kind of drupal specific function? This is not helping with the drupalism. we should only have to document that this and that event is provided by drupal and that you have access to settings and trigger parameter as well. context is there only because it's quicker. the setting thing can be removed by attaching only to the 'attach' event. I just wanted to avoid having to fire an event in drupal.js.

2. trigger for detach event is still there, still available as the 3rd parameter of the detach handler.

3. Yeah messed up this one, but shouldn't be in there to begin with. Drupal.transliterate might be a better home for it. There is another file with some behaviors and other things.

#16
I did some benchmarks, it's pretty much the same, and that's because in jQuery (i checked) they use the same kind of thing, they loop over an array of callbacks. That answers the next question: if one break, everything break. This is not how real events should behave. it's a jQuery-ism. From what I remember that was for performance reason… so real events must have been slower.

And we're stipping code from drupal.js, and that's always a good thing.

#17
yeah forgot to reverse that from another patch. Yep missed this one.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new49.48 KB
PASSED: [[SimpleTest]]: [MySQL] 35,378 pass(es).
[ View ]
nod_’s picture

Status:Needs review» Needs work

ok trigger() doesn't work like I though. hang on.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new49.48 KB
PASSED: [[SimpleTest]]: [MySQL] 35,352 pass(es).
[ View ]

here it is

sun’s picture

Status:Needs review» Needs work
+++ b/core/modules/file/file.js
@@ -12,9 +12,8 @@
/**
  * Attach behaviors to managed file element upload fields.
  */
-Drupal.behaviors.fileValidateAutoAttach = {
-  attach: function (context, settings) {
-    var $context = $(context);
+$(document).on('attach', function (e, settings) {
+  var $context = $(e.target);
...
-  },
-  detach: function (context, settings) {
-    var $context = $(context);
+});
+
+$(document).on('detach', function (e, settings, trigger) {
+  var $context = $(e.target);
     var validateExtension = Drupal.file.validateExtension;
     var selector, elements;
     if (settings.file && settings.file.elements) {

All of these anonymous event handler functions cannot be overridden or replaced by contributed and custom modules anymore.

Wouldn't it make much more sense to keep the named functions, and simply register them afterwards?

Drupal.behaviors.fileBlah {
  attach: ...
  detach: ...
}
$(document).on('attach', Drupal.behaviors.fileBlah.attach);
$(document).on('detach', Drupal.behaviors.fileBlah.detach);

Or... even more simple - just register all Drupal.behaviors.*.attach for $(document).on('attach') (and likewise for detach) in drupal.js?

In turn, the code change for all module maintainers boils down to injecting an "e, " into their behaviors - done.

nod_’s picture

Status:Needs work» Needs review
StatusFileSize
new50.28 KB
PASSED: [[SimpleTest]]: [MySQL] 35,355 pass(es).
[ View ]

ok that should do it. Now the way it works is simple, if you don't want to have the tableheader behavior run for example:

$(document).off('attach.tableHeader');

And you bind your own event, you can claim the namespace if you want. Or you can add a second behavior with the same namespace if you want to.

(edit) actually instead of using a 3rd parameter in the detach handlers we could trigger a namespaced event, only behaviors bound to .serialize or .move would trigger (and you can have several namespaces on one event: detach.serialize.userForm, so you can still unbind easily).

No need to resort to ugly methods, we're trying to put the PHP out of JS :)

nod_’s picture

I don't think the behavior weight is an important issue. Currently the behavior weight module has 15 install and I haven't been presented with a single use-case for them yet (I'm really waiting for some!).

sun’s picture

Any chance to incorporate #22?

nod_’s picture

#22 can definitely work but we'll be adding more code to make it work. It's possible to achieve pretty much the same things with events while removing code overall. I could get behind doing the binding from drupal.behaviors if we weren't using anonymous functions, both solutions are the same on that front.

Like I showed in #23, it's possible to remove any event handler. It's not possible to replace it exactly like before though. I would argue that the need for replacing is to work around poor code. If we're talking about core, some of our libraries needs to be cleaned up and have a better separation between the initialization step and the rest of the code, it should be possible to take out the initialization code from the lib and stick that in another file. If we have that, if you want to replace the init code, hook_js_alter() is the answer. The problem is also that we have X unrelated behaviors in the same file, system.js is a good example of that.

The way it work for jQuery plugins is that you have to take care of the init yourself, and if there is a generic init, it can be turned off by configuring some variable before the jQuery.ready thing.

Binding for people is kind of the easy way out I'd like to try avoiding.

nod_’s picture

Status:Needs review» Needs work

With more thinking I'm happy to go with a small wrapper that will allow us to do something like .bindBefore/After('myOtherBehavior') and bind all the events in the right order in drupal.js

nod_’s picture

about guarding against broken behaviors: #1639012: Guard against broken behaviors

donquixote’s picture

Re-posting (with minor modifications) from #1302126-18: Weights for Drupal.behaviors, following up on a use case mentioned in #1302126-17.

Ok, so in this case it would better be solved with dependencies, aka "run A before B".

Following #1446166-22: Use JS events instead of Drupal.behaviors and #1446166-27: Use JS events instead of Drupal.behaviors:
We move to events for the internal implementation, and this will change the signature of attach() methods to expect an event argument.
However, the registration of behaviors would still be quite similar: You create an object Drupal.behaviors.my_behavior with an attach() method, and drupal.js will register it automatically. (If I understand correctly).

Instead of the module js explicitly saying ".bindBefore()", I think it would be preferable if you can simply give the behavior object a dependencies attribute.

E.g.
Drupal.behaviors.my_behavior.run_before.another_behavior = true;

As mentioned in #1302126-7: Weights for Drupal.behaviors, we can have both, or only one of: behavior.weight, and behavior.run_before / .run_after.
This said: I wonder how event handler dependencies can be dealt with outside Drupal behaviors attach() stuff. How does it work with jQuery events, for instance?

And how can .bindBefore() recognize string keys, if events are registered with numeric keys? (remember, string key attributes have undefined order)

I think some of this is already answered here, but nod_ asked me to re-post nevertheless. In the end I hope this can help to clarify.

donquixote’s picture

Btw, when talking about dependencies:
If we call .bindBefore('another_event'), or anything else to register a dependency, then we need to expect that 'another_event' might not be known or registered yet. So, any sorting can only happen after the "registration phase", or directly before the event is fired.

Maybe this is already being considered, in this case sorry for the noise.

nod_’s picture

All right so I went around looking at what others are doing. Basically what we need here is publish/subscribe, not custom jQuery events. Behaviors were meant for handling new AJAX content and that's not tied into a DOM event on an element (which are what jquery .on and .off are for) but tied to an object event.

Good news, takes almost no code to implement that, bad news, there are tons of ways of writing this small piece of code.

Across all lib I saw only AmplifyJS had some kind of weight system, all the others had nothing of the sort and assumed you'd do your homework and register everything in the right order. Here are some of my best finds (in no particular order):

http://amplifyjs.com/api/pubsub/ source here: https://github.com/appendto/amplify/blob/master/core/amplify.core.js
https://github.com/addyosmani/pubsubz/blob/master/pubsubz.js
https://github.com/mroderick/PubSubJS
http://backbonejs.org/docs/backbone.html#section-15
http://radio.uxder.com/ code https://github.com/uxder/Radio

and a very long article about pub/sub in Java vs JS: http://peter.michaux.ca/articles/transitioning-from-java-classes-to-java...

What I'm tempted to go with is using the same DX as symfony which is the AmplifyJS implementation with a little twist around parameters. I was actually concerned that the way to define events names in symfony and JS would be very different, if we go with this it'll be more coherent. Not relying on the DOM is a good thing. DOM is slow and has a lot of overhead.
If we make the pub/sub user-proof that would fix #1639012: Guard against broken behaviors by itself.

Seeing our requirements, I haven't found a lib that does everything we want, we might need to fork an existing implementation. It's a small amount of straightforward code, not a big deal.

droplet’s picture

I'd suggest use jQuery.Callbacks() + borrowing few weighting code from amplifyjs.

jessebeach’s picture

So would every callback be a $.Callback object even if it's just one callback? I think it would have to be the case.

droplet’s picture

andypost’s picture

Any progress in this? Maybe move the feature for D9?

nod_’s picture

Version:8.x-dev» 9.x-dev
nod_’s picture

Issue summary:View changes

remove unexact problems

catch’s picture

Version:9.x-dev» 8.1.x-dev

Could be added in a minor version I think.

yched’s picture

nod_’s picture

The other issue is a better solution for the problems with behaviors, I'm not sure we can get the same thing as easily with events now.

So while behaviors are a drupalism, with that new issue, there is a valid reason for them. Tempted to won't fix this one.

catch’s picture

Status:Needs work» Closed (duplicate)

Let's mark this duplicate.

catch’s picture