Last modified: 2012-05-11 08:43:43 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 T35952, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 33952 - Refactor mw.toolbar to allow dynamic additions at any time
Refactor mw.toolbar to allow dynamic additions at any time
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
JavaScript (Other open bugs)
1.18.x
All All
: High enhancement with 1 vote (vote)
: 1.19.0 release
Assigned To: Tim Starling
:
: 33922 36753 (view as bug list)
Depends on:
Blocks: 31511
  Show dependency treegraph
 
Reported: 2012-01-26 00:42 UTC by Bergi
Modified: 2012-05-11 08:43 UTC (History)
9 users (show)

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


Attachments

Description Bergi 2012-01-26 00:42:17 UTC
Continuing Bug Bug 31511#6

> http://en.wikipedia.org/w/index.php?diff=454405866
> http://en.wikipedia.org/w/index.php?diff=454588601&oldid=454587788

Two reports about a not well initialised toolbar. I think I found a reason:

The current implementation puts out a inline script tag with the RessourceLoader-call for certain modules, amongst them mw.action.edit. Right after that comes another inline script, which checks the existance of window.mediawiki and then addsButtons() to the toolbar. On-DOM-Ready then the mw.toolbar.inits().

One concern I have is that the second inline script shouldn't rely on the RL module, which might be loaded and executed asynchronously in newer browsers in special cases.
Also, it is very hard for scripts like the XEB-gadget to execute a function between the addButton-calls, which register the default buttons, and the init() function that creates the buttons.

So I'd propose to create a own ressource loader module for the default buttons. I can't see any reason why this is still an inline script; we might even put it together with the mw.toolbar code.
Comment 1 Krinkle 2012-01-28 15:53:32 UTC
Yes, I've been wanting this for a long while.

* Remove inline code
* Move into regular module or into a dynamically generated module (i.e. a new Module class)
* Change JS logic to allow multiple building stages (i.e. addButton should work at any time not just before initialization)
* Make .init() a once-function, and enqueue into document-ready from the 1-line init-module

Note that recently Brion has made some changes in the 1.19-dev version that solved a few problems as well.
Comment 2 Krinkle 2012-01-28 15:54:44 UTC
*** Bug 33922 has been marked as a duplicate of this bug. ***
Comment 3 Bergi 2012-01-29 18:57:20 UTC
(In reply to comment #1)
> Yes, I've been wanting this for a long while.
:-)

> * Change JS logic to allow multiple building stages (i.e. addButton should work
> at any time not just before initialization)
> * Make .init() a once-function, and enqueue into document-ready from the 1-line
> init-module

I would like to see some more functions, which could make it a part of a MVC architecture. I currently think of
* removeButton()
* reload() or, just as well, a public bindable init() function (of course by default on-document-ready, too)
Comment 4 Krinkle 2012-01-29 18:58:41 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > Yes, I've been wanting this for a long while.
> :-)
> 
> > * Change JS logic to allow multiple building stages (i.e. addButton should work
> > at any time not just before initialization)
> > * Make .init() a once-function, and enqueue into document-ready from the 1-line
> > init-module
> 
> I would like to see some more functions, which could make it a part of a MVC
> architecture. I currently think of
> * removeButton()

Makes sense to have remove, yeah.

> * reload() or, just as well, a public bindable init() function (of course by
> default on-document-ready, too)

I was thinking of automatically updateing/building the html when calling addButton, removing the need for a public init().
Comment 5 Rob Lanphier 2012-01-30 22:42:37 UTC
Krinkle, is this really something that needs to block a 1.19 release?
Comment 6 Krinkle 2012-01-30 23:29:09 UTC
(In reply to comment #5)
> Krinkle, is this really something that needs to block a 1.19 release?

Well, I'd say it's your call.

Any code written in 2011 or later should probably be providing tools for users with Vector/WikiEditor, not Monobook/Classic toolbar. WikiEditor has very good javascript hooks whereas the the classic toolbar's are crap (and always have been kind of hacky).

Scripts manipulating the classic toolbar will fail if they load themselves as a resource module because the legacy code for the toolbar bypasses most of resource loader.

-> Either we force users to keep their classic-toolbar enhancement scripts un-resourceloaderified, or the classic toolbar needs to be resource loaderified.

It's not a regression in 1.19, but race conditions are more likely in 1.19 due to speed. And unfortunately, contrary to the module story, there is no "right" way to edit the classic toolbar without having a race condition, simply because the classic toolbar isn't in the normal flow of execution. It exposes a global array where scripts can add things to, and at some point around document-ready event it builds the toolbar with the buttons in the array at that moment, and then the array is further ignored.
Comment 7 Rob Lanphier 2012-02-07 18:32:24 UTC
Removing as 1.19 blocker.
Comment 8 Krinkle 2012-02-26 23:36:33 UTC
Fixed by r112451 which build on r111983, r108191, and others.
Comment 9 Krinkle 2012-05-11 08:43:43 UTC
*** Bug 36753 has been marked as a duplicate of this bug. ***

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


Navigation
Links