Last modified: 2014-11-17 10:34:47 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T25580, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 23580 - Implement JavaScript mw.hooks framework
Implement JavaScript mw.hooks framework
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
JavaScript (Other open bugs)
unspecified
All All
: Normal enhancement with 1 vote (vote)
: 1.21.0 release
Assigned To: Krinkle
:
Depends on:
Blocks: 52071 30713
  Show dependency treegraph
 
Reported: 2010-05-18 09:56 UTC by Amalthea
Modified: 2014-11-17 10:34 UTC (History)
15 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch. (285 bytes, patch)
2010-05-18 09:56 UTC, Amalthea
Details
Updated patch using jQuery custom events (528 bytes, patch)
2010-05-20 11:44 UTC, Amalthea
Details

Description Amalthea 2010-05-18 09:56:28 UTC
Created attachment 7381 [details]
Patch.

Currently, a previewed edit does not display [[WP:NAVPOP]] popups since NAVPOP can't register its mouse event handlers on the dynamically created links. There is a workaround that checks in intervals for the spinner to disappear, and makes navpop reparse the preview. That's rather ugly, and I presume other scripts wanting to react on the preview have the same problem.

I propose adding a simple hook which is called once display of live preview is finished (pretty much exactly like [[User:Js/ajaxPreview]]). Proposed patch attached.
Comment 1 Derk-Jan Hartman 2010-05-18 20:24:21 UTC
Perhaps it is better to create something like an addOnLivePreviewHook()
Comment 2 Kevin Israel (PleaseStand) 2010-05-18 21:47:13 UTC
Yes, specifically allowing a hook function to be called either before the contents of wpTextbox1 are grabbed or after the preview is updated. A call before would be helpful for adding live preview support to such scripts as [[User:PleaseStand/References segregator]], which temporarily reformats the wikitext when editing starts and changes it back to the correct format when the page is saved.
Comment 3 Amalthea 2010-05-18 22:23:58 UTC
(In reply to comment #1)
> Perhaps it is better to create something like an addOnLivePreviewHook()

Absolutely right. Would taking this one step further and build a more generic hook system in wikibits make sense:

function addNamedHook( eventName, hookFunct );
function runNamedHook( eventName, autoexecuteFutureHooks = false );

Second argument of runNamedEventHook would determine if any future added hooks for that event are executed immediately (e.g. for onload) or delayed till the next run (e.g. for livepreview). Could possibly pass additional arguments through to the hook functions.
Otherwise I see multiple such hook systems being built into MediaWiki itself in the future, it's easy to predict that it will get more and more javascript functionality down the road. Additional hooks could already be useful in e.g. mwsuggest and ajaxwatch, and I could see use for such generic hooks to facilitate inter-script communication of specialized project scripts as well.
Comment 4 Michael Dale 2010-05-18 22:51:20 UTC
It would probably be more ideal to use jQuery "bind" event system somthing like: 

for the "add hook" you would use something like; 
$j( mw ).bind( eventName , function( event, param1, param2 ){
   // stuff to do at eventName time
});

Then the run hook use : 
$j( mw ).trigger( eventName, [ array, of, params ] );

A bind system is more general and can be applied to interface objects .. rather than just all in a root name space. 

This is already used in the usability extension and mwEmbed makes extensive use of this binding type system in conjunction with the scriptLoader it enables modules to conditionally extend each other while preserving a single server request for all script and css resources. 

For example the timedText module conditionally extends the embedPlayer request with its timedText stuff, the set of dependent libraries is dynamically built out binded to object specific events. 
i.e: http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/TimedMediaHandler/TimedText/loader.js?view=markup
Comment 5 Amalthea 2010-05-20 11:44:33 UTC
Created attachment 7389 [details]
Updated patch using jQuery custom events

Sure, why reinvent the wheel.
Comment 6 Derk-Jan Hartman 2010-05-21 14:02:34 UTC
Fixed in r66721
Comment 7 vlakoff 2011-08-29 00:21:56 UTC
I don't think it's a good idea to use jQuery with the object mw as the selector. It currently works, .bind/.trigger match, but I think it's more by luck and it might break in the future.

According to the documentation, jQuery() is designed to be given a selector string, a DOM element, an array of DOM elements, another jQuery object, or even nothing; but certainly not pure JS objects.
Comment 8 Michael Dale 2011-08-29 07:28:18 UTC
The bind trigger system is widely used for pure JS objects, so I don't think jQuery will ever disallow it. For example if you google the concept you see lots of folks recommending it.

But I do agree it can result in some strange gotchas and edge cases that may not be ideal and we should not do it "just because it works and other say to do it". 

A more traditional hook system may be more correct.

While jQuery does not officially "accept pure JS" objects its inherent in the model of loose typing that jQuery is designed around. i.e you can illustrate this with much more evil things like: 

var o = {};
$(o).css('width', 10);
console.log(o.width);

// Output:
10px

If we were to write a hook system it should share the nifty features of the jQuery one. Like namespaced bind add and removal, being able to pass in an array of objects to be binded, some concept of inheritance of same named local methods as the trigger, a convenience function to setup the hook system on-demand so that binds and triggers have no negative consequences if either does not exist etc.

In essence everything the jQuery bind / trigger system does. You can think of it as a plugin. We can always just override jQuery.fn.bind to guarantee pure JS object paths behavior predictably into the future. 

It comes down to semantics and clarity of intent of any given developer. Does it add syntactical clarity to be able to treat the bind trigger concept the same across objects or does it muddle things that it works the same? 

If it can save time and code to share the concept across objects then that’s good, if it will confuse people and take away flexibility then that’s bad. 



Some examples of gotchas for jQuery base bind / trigger

var o = {length: 3};
$(o).bind( 'custom', function(){ console.log('hi') } );
$(o).trigger('custom');

// Output
hi
hi
hi 

Of course this can be a "feature" if you have an array of objects that you want to bind events to. 

Also a "feature" of the binding mechanism includes calling the child method i.e:

var o = {change: function(){ console.log('hi method')} };
$(o).bind( 'change', function(){ console.log('hi bind') } );
$(o).trigger('change');

// Output
hi bind
hi method

Which can be tricky / non-ideal if you don't know about that.
Comment 9 vlakoff 2011-09-08 04:01:42 UTC
Thank you for this detailed and very interesting explanation.

I took a look at the source of jQuery and ran through some debug sessions, their work using makeArray for loose-typing, and unique identifiers for robust references is really impressive.

I agree it is very, very unlikely that jQuery would disallow non-DOM objects in the future, and I think we can be confident it will continue to work with an object such as mw.
Comment 10 Krinkle 2011-09-17 18:33:09 UTC
(In reply to comment #8)
> Some examples of gotchas for jQuery base bind / trigger
> 
> var o = {length: 3};
> $(o).bind( 'custom', function(){ console.log('hi') } );
> $(o).trigger('custom');
> 
> // Output
> hi
> hi
> hi 

Throws TypeError as of jQuery 1.6+, presumably because it's looping through 'o' and finds [0], [1] and [2] to be undefined.

Suggest we revert r66721 and possibly other similar implementations and instead create a mediawiki.hook module. ie.

mw.hooks.run( 'key', .., .. );
mw.hooks.add( 'key', fn );

That way different parts of the pages build up can tab into different events and hooks (instead or relying on the over-used "document ready" event. For example:

* 'outputPage.bodyContentReady' - (args: $bodyContent ): Run after <div id="bodyContent"> has been populated. Called once on a normal page, and re-called in special circumstances (ie. after live preview is injected). Scripts like tablesorters and collapsiblemakers would then hook into this, instead of document-ready to do their stuff.
Comment 11 Michael Dale 2011-09-17 19:35:34 UTC
That was an example of what people should not do. Presumably people would know better than to use "length" as an object property, bind trigger won't be the only thing that acts funny. 

I make pretty heavy use of bind trigger model. It certainly has its trade offs but so does a monolithic global hook system. 

Some points for bind / trigger: 

Its kind of standard for building reusable hooks into components, its used a lot in jQuery ui for extending ui stuff for example: http://jqueryui.com/demos/accordion/#event-create  

You mention "different parts of the pages build up can tab into different events
and hooks" thats not exclusive to the mediaWiki object, complex components need extendability as well. 

The bind trigger model integrates nicely when your creating virtual interfaces for components that inter-mix with emerging apis for dom elements. 

For example you create a virtual iframe to handle uploading you can use a fallback to flash on older browser that triggers the same events, as the modern html5 versions. As the native event becomes available you seamlessly pass the event along the same code path. 

The bind trigger model also is good about encapsulating bindings in the object representation. So when you copy or duplicate your element / object representation it keeps all its bindings.  

jQuery based Bind trigger of course plays nice with jQuery so you can bind / trigger on selectors.  

You can namescape your extension bindings so that its easy to unsubscribe a given extending component. For example you have "player" element. A few modules adding bindings to 'onplay', 'onpause', and 'onend'. Each module namespaces its event binding, i.e: onplay.fooModule so when a controller or sequencer decides it needs to unbind a given module it just runs $(player).unbind(.fooModule) then all fooModule bindings will be removed, with the flexibility to manage this in the controller or in the extending module.  


You certainly ~could~ build a hook system that had a lot of these qualities but it would not be as simple as .run and .add 

Maybe the point is simply the master mw object use hook while anything that touches a selector uses bind / trigger. I would be cautions that we don't end up with other components adding events to the core hook system when it would be a better path for the events to be locally bind / triggered.

If the core promotes this bind trigger model it may help people think about doing things that way for their module rather then each module setting up a local hook system or mapping and calling all points of extendability with a globally named hook.
Comment 12 Sumana Harihareswara 2011-11-09 21:27:26 UTC
Adding "patch" and "reviewed" keywords here for consistency, since Amalthea's attached patch was reviewed. Thanks for the patch, Amalthea.
Comment 13 Krinkle 2011-11-09 21:57:51 UTC
Using the jQuery Event binding/triggering model doesn't seem good on performance (kinda slow for something this simple). Also it's not very recommended by jQuery to use it for this.

I'd suggest we create a simple module ourselves, the functionality we want here really is simple and we should be able to rely on this.

A very rough example:

  var register = {};

  mw.hooks = {
    register: function( name, callback ) {

      if(!register[name]) register[name] = [];

      push.call( register[name], callback );

    },
    run: function( name [, data ] ) {

      if(!register[name]) return;

      for ( var i = 0, len = register[name].length; i < len; i++ ) {
        register[name][i].apply( data || [] );
      }

    }
  };

If we update our copy of jQuery to 1.7 we could even use jQuery.Callbacks [1] to make this code a little more robust and shorter. 

--
Krinkle

[1] http://api.jquery.com/jQuery.Callbacks/#pubsub
Comment 14 Krinkle 2011-11-09 22:08:43 UTC
Rephrasing bug to be more generic.

Example events (most of these currently abuse dom-ready or window-load, by making these mediawiki-custom events many things such as jquery-sortable would then work again for ajax preview).

* 'articleReady'
-- Runs whenever the article section of the page is rendered in html. Normally only once during a page load. Other times it woud run would be during ajax preview. Plugins like jquery.sortable or jquery.makeCollapsible would bind to this instead of $(document).ready

-- data: [ article wrapper element (#bodyContent for Vector) ]

* 'diffHeader' 
-- Runs whenever the diff meta data is shown. Useful for gadgets that want to insert links in here. Normally only once on a page, but gadgets like RTRC that dynamically load in diffs on-demand on Special:RecentChanges would fire these events more often so that stuff like Twinkle can actually work with other gadgets.

-- data: [ table.diff element containing the meta data and diff itself ]
Comment 15 Daniel Friesen 2012-08-13 09:33:08 UTC
This has some overlap with bug 30713.

This bug should probably focus on the introduction of a mw.hooks for anything we need it for. While the other bug focuses on the hook/event that happens on domready/preview and whether it should be a hook or a dom event.
Comment 16 Krinkle 2012-08-16 17:01:27 UTC
Next week.
Comment 17 Mark A. Hershberger 2012-09-28 21:30:13 UTC
(In reply to comment #16)
> Next week.
....
Comment 18 Bartosz Dziewoński 2012-11-09 12:31:16 UTC
Krinkle, will you be implementing this or can somebody else step in? Do you already have something that could be used as a start?

I think the API we need has been already pretty well described here and in the discussion under bug 30713.
Comment 19 Krinkle 2012-11-09 13:28:23 UTC
Other than the code I have proposed here and on other bugs, I don't have any secret local branch that would be wasted if someone else works on it.

However I do have very specific ideas for how it should be implemented and would like to stay involved with it.

Having said that, I have a lot of things I work on and would very much welcome it if someone else steps up and gives this a shot. I'll do my best to be helpful in code review and give you anything you need.
Comment 20 Siebrand Mazeland 2013-01-30 21:56:04 UTC
Unassigned per comment 19
Comment 21 Krinkle 2013-02-03 07:45:51 UTC
(In reply to comment #20)
> Unassigned per comment 19

You changed the assignee field instead of the bug status (which is still ASSIGNED).

Anyway, re-assigning, I'm working on this now and will push (at least a draft version) today.
Comment 22 Andre Klapper 2013-03-20 15:36:40 UTC
(In reply to comment #21 by Krinkle)
> Anyway, re-assigning, I'm working on this now and will push (at least a draft
> version) today.

Krinkle: Did that happen? Is there some (public) gerrit change ID to review?
Comment 23 Krinkle 2013-03-30 19:25:08 UTC
An updated concept, using $.Callbacks:


@@ -290,6 +290,112 @@ var mw = ( function ( $, undefined ) {
 
		hook: ( function () {
			var lists = {};
			return {
				/**
				 * Register a hook handler
				 */
				on: function ( name, handler ) {
					var list = lists[name] || ( lists[name] = $.Callback( 'memory') );
					list.add( handler );
				},
				/**
				 * Unregister a hook handler
				 */
				off: function ( name, handler ) {
					var list = lists[name];
					if ( list ) {
						list.remove( handler );
					}
				},
				/**
				 * Run a hook.
				 * @param {string} name Name of hook.
				 * @param {Mixed...} data
				 */
				emit: function ( name ) {
					var list = lists[name] || ( lists[name] = $.Callback( 'memory') );
					list.fireWith( null, slice.call( arguments, 1 ) );
				}
			};
		}() ),
Comment 24 Krinkle 2013-03-31 00:48:34 UTC
Change-Id: Ic73a3efe53d6fb731e7f1e531d5f51530cd7e4fe
Comment 25 Brad Jorsch 2013-05-07 14:25:14 UTC
Merged

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links