Last modified: 2013-06-27 18:28:46 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 T49872, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 47872 - [Regression] mwCustomEditButtons from user script should work
[Regression] mwCustomEditButtons from user script should work
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
1.22.0
All All
: Normal minor with 1 vote (vote)
: 1.22.0 release
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-30 13:07 UTC by Krinkle
Modified: 2013-06-27 18:28 UTC (History)
8 users (show)

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


Attachments
Test case for the browser issue with setTimeout and document.write (867 bytes, application/rsd+xml)
2013-04-30 15:14 UTC, Brad Jorsch
Details

Description Krinkle 2013-04-30 13:07:15 UTC
From the fall out of bug 47457's resolution:

(bug 47457 comment #63 by Liangent)
> (bug 47457 comment #54 by Krinkle)
> > Change-Id: Ic3d0c937268d0943d2f770f3ca18bcf4e1eed346
> 
> This fix causes breakage of any user of site scripts which assume that
> they're
> loaded before $(document).ready(). Maybe someone can move jQuery.ready() call
> to a position just before </body> and after all other script references?

(bug 47457 comment #64 by matma.rex)
> And it also breaks the old 'mwCustomEditButtons' interface to add custom
> buttons to the old edit toolbar, which apparently everyone and their dog are
> using across the wikis. (Since the user JS is loaded from the bottom queue,
> but
> the toolbar script either from the top or using inline <script> tags with
> $.ready inside.)
> 
> Reopening. Moving the jQuery.ready() call after the bottom queue seems like a
> good idea.


(bug 47457 comment #66 by Brad Jorsch)
> (bug 47457 comment #63 by Liangent)
> > Maybe someone can move jQuery.ready() call to a position just before 
> > </body> and after all other script references?
> 
> (bug 47457 comment #64 by matma.rex)
> > Moving the jQuery.ready() call after the bottom queue seems like a good idea.
> 
> We tried that, see patchset 5 in https://gerrit.wikimedia.org/r/#/c/61057/5.
> It didn't work.
> 
> But unless the browsers were delaying the DOMContentLoaded event until after
> the async-loaded scripts in the bottom queue were loaded, I don't know how
> this wasn't already a problem. Krinkle, any ideas?

A few important questions I'd like answered:

@Liangent: Can you given an example where it matters for a user script whether the document becomes ready before or after they are loaded? The user scripts are loaded from the bottom of the <body> before </body>. Practically the document is already ready at this point, just $(document).ready() hasn't fired yet. Any user script code that isn't wrapped in $(document).ready() will still execute at exactly the same time. Any user script code that *is* wrapped in $(document).ready() will now execute immediately after loading instead of... immediately after loading. Exactly the same, not?

@Matma.rex: You mean the fact that the deprecated mwCustomEditButtons global (unlike the mw.toolbar interface) is inspected only once by mediawiki.action.edit.js, namely at the domready event. And if it is triggered before the user script, adding to that array will become a no-op.

Though there is a lot of usage of this feature, lets not forget the context:

* The mwCustomEditButtons interface is for the classic toolbar (aka "Monobook editor", though Vector users can also disable WikiEditor to get to it). This is already a large minority of users.
* The mwCustomEditButtons interace has been deprecated in favour of mw.toolbar.addButton, which I created especially for the legacy toolbar users last year to address the many declined bug reports for mwCustomEditButtons which could not be otherwise addressed because mwCustomEditButtons is beyond repair, it is a flawed concept and it keeps breaking for many reasons.

I don't think anything related to mwCustomEditButtons can justify a change of any kind unless it comes at absolutely no overhead and downside.

@Brad Jorsch: It seems that the Firefox bug with document.write being disabled before DOMContentReady is worse than we thought. It is disabled even before the end of the </body> has been reached. We found through trial-and-error that placing it after the main content but before the bottom queue worked.

As Brad pointed out, $.ready being triggered earlier makes no difference for the bottom queue because they are loaded asynchronously, they always execute without blocking the parser or the DOM ready event. In fact (while designing this) Roan and I made very sure to test this across the entire board of browsers we even remotely support (IE6-10, FF2-17, Chrome10-23, Opera9-11.x). And in any browser that we couldn't load it async, we scheduled the addScript to after dom-ready manually (only in Opera).

However note that the user scripts (like the site scripts) are *not* loaded through mw.loader because we still support legacy global scope for them. They are loaded by a hardcoded script tag in the DOM. Which means they are blocking and used to block dom-ready, and now they run after dom-ready (well, they still block dom-ready, but we trigger that callback queue earlier).
Comment 1 Bartosz Dziewoński 2013-04-30 13:24:02 UTC
(In reply to comment #0)
> @Matma.rex: You mean the fact that the deprecated mwCustomEditButtons global
> (unlike the mw.toolbar interface) is inspected only once by
> mediawiki.action.edit.js, namely at the domready event. And if it is
> triggered
> before the user script, adding to that array will become a no-op.

Yes, precisely.


> Though there is a lot of usage of this feature, lets not forget the context:

> * The mwCustomEditButtons interface is for the classic toolbar (aka "Monobook
> editor", though Vector users can also disable WikiEditor to get to it). This
> is
> already a large minority of users.

Yeah, my guesstimate is a few hundred power-users across all of the Wikimedia wikis. (I just searched for 'mwCustomEditButtons' in user pages on a few large projects, and tried to account for inactive ones.)


> * The mwCustomEditButtons interace has been deprecated in favour of
> mw.toolbar.addButton, which I created especially for the legacy toolbar users
> last year to address the many declined bug reports for mwCustomEditButtons
> which could not be otherwise addressed because mwCustomEditButtons is beyond
> repair, it is a flawed concept and it keeps breaking for many reasons.
> 
> I don't think anything related to mwCustomEditButtons can justify a change of
> any kind unless it comes at absolutely no overhead and downside.

I realize that this is deprecated, and agree that it's broken by design. But killing it should be preceded by some release notes and some Wikimedia announcements (or just fixing users' JavaScript for them, since there are comparatively few of them).

I have reopened the previous bug because of the combination of Liangent's comment (which I didn't verify myself) and at least two users complaining about this on #wikimedia-tech today, including one who was kind and persistent enough to help me confirm that this was what broke the functionality ([[nl:User:Romaine]]).

So I'm all for removing this legacy compatibility, but let's do it right instead of as a side-effect of an unrelated fix.
Comment 2 Brad Jorsch 2013-04-30 15:06:31 UTC
(In reply to comment #0)
> @Brad Jorsch: It seems that the Firefox bug with document.write being
> disabled before DOMContentReady is worse than we thought. It is disabled even
> before the end of the </body> has been reached. We found through
> trial-and-error that placing it after the main content but before the bottom
> queue worked.

But it works from synchronously-processed scripts, even just before </body>.

I've done some more testing on this Firefox situation. More specifically, it seems that *any* document.write called from a setTimeout callback will replace the document, even if it fires during <head>. And this even avoids the check (since Gecko 2.0 (Firefox 4.0)) that is supposed to disable document.write when called from async-loaded scripts.[1]

 [1]: https://developer.mozilla.org/en-US/docs/HTML/Element/script#attr-async

The following browsers seem to be affected by this issue: Firefox 20, IE8, IE10, Chrome 24, Chrome 26, Opera 12.14.

The following browsers seem to *not* be affected by this issue: Safari 5.1 (Windows)

So runScript() should probably be forcing async when it calls addScript(), since it is called from a setTimeout in addEmbeddedCss(), as another layer of fix for bug 47457.


> As Brad pointed out, $.ready being triggered earlier makes no difference for
> the bottom queue because they are loaded asynchronously, they always execute
> without blocking the parser or the DOM ready event.

In fact, this should make the behavior for the bottom queue *more* predictable. Previously, depending on whether the async-loaded scripts for the bottom queue loaded extremely quickly (e.g. from cache) and whether the browser then decided to process them before firing DOMContentLoaded, they might sometimes be fired before $.ready and sometimes after. Now they'll always be after, no matter what the browser does.

> However note that the user scripts (like the site scripts) are *not* loaded
> through mw.loader because we still support legacy global scope for them. They
> are loaded by a hardcoded script tag in the DOM. Which means they are
> blocking and used to block dom-ready

I suppose that's where these complaints about mwCustomEditButtons and other such are coming from, if someone coded something assuming pre-ready directly in a user script or site script. OTOH, scripts loaded from the site or user scripts loaded via mw.loader.load() already had the issue (thanks to the fix for bug 34542 defaulting that sort of thing to async), so it only half-worked at best before bug 47457 anyway.
Comment 3 Brad Jorsch 2013-04-30 15:14:07 UTC
Created attachment 12209 [details]
Test case for the browser issue with setTimeout and document.write

I've attached my test case for the browser issue with setTimeout and document.write. Note that it should be served from a webserver that is not behind a proxy server, so the flush calls result in it slowly counting 0-9.
Comment 4 Liangent 2013-04-30 15:20:35 UTC
(In reply to comment #0)
> @Liangent: Can you given an example where it matters for a user script
> whether
> the document becomes ready before or after they are loaded? The user scripts
> are loaded from the bottom of the <body> before </body>. Practically the
> document is already ready at this point, just $(document).ready() hasn't
> fired
> yet. Any user script code that isn't wrapped in $(document).ready() will
> still
> execute at exactly the same time. Any user script code that *is* wrapped in
> $(document).ready() will now execute immediately after loading instead of...
> immediately after loading. Exactly the same, not?

Let me give a (almost) real world example (difference explained below):

We have some gadgets (technically some gadgets, but socially one gadget; not using [ResourceLoader]). The first is the gadget itself, which registers some global states as data/options, then $(document).ready() its execution code. All gadgets below are modifiers for the gadget which change those global states so when the main body of the gadget is executed, the modified global states are visible to it. But now...

Difference from real world: addOnloadHook() is used there instead of $(document).ready() so it finally escaped :)
Comment 5 Krinkle 2013-04-30 15:39:23 UTC
Rephrasing bug to be about mwCustomEditButtons to avoid mixed subjects.

So in summary: mwCustomEditButtons is very ugly and undocumented. But it used to be the only way to add buttons to the toolbar. Though (as pointed out by various comments) it is a coincidence that most existing usages have survived without causing errors, but it is the case.

How mwCustomEditButtons works now:
* Initialise to empty array from top queue
* Hook in document ready
* Iterate through array (e.g. items added before the page load) and call mw.toolbar.addButton for each one.

The state is that most existing usages of mwCustomEditButtons happen from a site or user script at run-time in the global scope. And this works fine because they are blocking script tags.

Now that $.ready() (the jquery callback list, not the event itself) is triggered before site/user script load (the site/user script still load from the bottom) it means the code that iterates through the legacy array will not get any of the items because they're not in the array yet at that time.
Comment 6 Liangent 2013-04-30 15:49:51 UTC
(In reply to comment #5)
> Rephrasing bug to be about mwCustomEditButtons to avoid mixed subjects.

See my comment above. There may be other similar structures existing there.
Comment 7 Bartosz Dziewoński 2013-04-30 16:04:52 UTC
So how hard would it be to get the site&user scripts to run first, then $.ready, then the rest of the bottom queue? Then we could kill off mwCustomEditButtons after announcing it properly and revert the change (unless there is a considerable number of other scripts this would break).

[Disclaimer: I know next to nothing about ResourceLoader internals.]
Comment 8 billinghurst 2013-04-30 16:12:40 UTC
As a comment mwCustomEditButtons functionality is used at English Wikisource. And for numbskulls like myself it is far easier to code snippets to implant code than the alternate means. I repeatedly failed to master mw.toolbar.addButton, gave up walked away, too much to do, too much to customise.

The old toolbar allows for easy nulling out of buttons not required, or not prioritised, and the easy addition of little bits of personal functionality, and the new toolbar makes these things hard (IMO).  Don't blame us that we find the older tool to work better; has the requisite functionality, and meets our simple needs. We are not WP, our requirements are different, our tasks are different, our namespaces are different.

I am happy to try again to be less stupid to see if I can create a core set of tools that could be used, and allow users to plug them in as they see fit, however, it isn't my core skill set. Definitely an area where better guidance would be of use.

PLUS as the 'new' toolbar is still considered a BETA FEATURE (see preferences), why would we be expected to move. Stridency against us simple editors seems a little harsh. Some of us just want to get on with our tasks, and chasing and understanding the backend changes isn't easy.
Comment 9 Bartosz Dziewoński 2013-04-30 16:18:40 UTC
billinghurst, this has nothing to do with switching to the new toolbar; mw.toolbar.addButton works with and is intended for the old one, and the change is just a simple find and replace:

  Find:         mwCustomEditButtons[mwCustomEditButtons.length] = { ... }
  Replace with: mw.toolbar.addButton({ ... })

Just be careful about braces and parentheses.

Here's an example diff: https://nl.wikipedia.org/w/index.php?title=Gebruiker:Romaine/vector.js&diff=37497149&oldid=37497060
Comment 10 Krinkle 2013-04-30 20:52:43 UTC
(In reply to comment #7)
> So how hard would it be to get the site&user scripts to run first, then
> $.ready, then the rest of the bottom queue? Then we could kill off
> mwCustomEditButtons after announcing it properly and revert the change
> (unless
> there is a considerable number of other scripts this would break).
> 
> [Disclaimer: I know next to nothing about ResourceLoader internals.]

site&user script run after the bottom queue by design. Just like the cascading stylesheets, they are intended and designed to allow overriding/customising things after everything else.

Though presence of code will not make a difference (since bottom queue is async), the loading is affected.

Code in site and/or user scripts that are wrapped in mw.loader.using would trigger additional http requests instead of tagging along on the already pending request for the bottom queue (this is ResourceLoader internals).

I'd recommend we add some code to mediawiki.action.edit to periodically check mwCustomEditButtons. Then immediately deprecate it (not removing it, just marking it as deprecated) and phasing it out over the following weeks (I'd like to add it to the js-deprecations train roll-out in May which will introduce mw.log.deprecate and console.warn messages that users can use to easily see if they are using something that is deprecated).

Then we can safely remove mwCustomEditButtons in the 1.22 release.

(In reply to comment #6)
> (In reply to comment #5)
> > Rephrasing bug to be about mwCustomEditButtons to avoid mixed subjects.
> 
> See my comment above. There may be other similar structures existing there.

Possible indeed, though unlikely as most legacy things have been phased out. Examples are welcome :)
Comment 11 billinghurst 2013-05-01 04:35:51 UTC
(In reply to comment #9)
> billinghurst, this has nothing to do with switching to the new toolbar;
> mw.toolbar.addButton works with and is intended for the old one, and the
> change
> is just a simple find and replace:
> 
>   Find:         mwCustomEditButtons[mwCustomEditButtons.length] = { ... }
>   Replace with: mw.toolbar.addButton({ ... })
> 
> Just be careful about braces and parentheses.
> 
> Here's an example diff:
> https://nl.wikipedia.org/w/index.php?title=Gebruiker:Romaine/vector.
> js&diff=37497149&oldid=37497060

Thanks. This innovation had escaped me, and clearly a whole community that was using mwCustomEditButtons. I will relay this to the enWS community, and fix our help pages.

Can you recommend that right page to watch for these changes.  I thought that I had watched all the relevant pages where Krinkle does these components, however, it is clear that this change wasn't evident for active changes.
Comment 12 billinghurst 2013-05-01 04:44:20 UTC
(In reply to comment #11)
> (In reply to comment #9)
> > billinghurst, this has nothing to do with switching to the new toolbar;
> > mw.toolbar.addButton works with and is intended for the old one, and the
> > change
> > is just a simple find and replace:
> > 
> >   Find:         mwCustomEditButtons[mwCustomEditButtons.length] = { ... }
> >   Replace with: mw.toolbar.addButton({ ... })
> > 
> > Just be careful about braces and parentheses.
> > 
> > Here's an example diff:
> > https://nl.wikipedia.org/w/index.php?title=Gebruiker:Romaine/vector.
> > js&diff=37497149&oldid=37497060
> 
> Thanks. This innovation had escaped me, and clearly a whole community that
> was
> using mwCustomEditButtons. I will relay this to the enWS community, and fix
> our
> help pages.
> 
> Can you recommend that right page to watch for these changes.  I thought
> that I
> had watched all the relevant pages where Krinkle does these components,
> however, it is clear that this change wasn't evident for active changes.

Clearly it was a long time ago as I have since found reference that it was broken in 1.18, but somehow it didn't break for all. Weird.
Comment 13 Nemo 2013-06-09 18:10:19 UTC
(In reply to comment #9)
> billinghurst, this has nothing to do with switching to the new toolbar;
> mw.toolbar.addButton works with and is intended for the old one, and the
> change
> is just a simple find and replace:
> 
>   Find:         mwCustomEditButtons[mwCustomEditButtons.length] = { ... }
>   Replace with: mw.toolbar.addButton({ ... })
> 
> Just be careful about braces and parentheses.
> 
> Here's an example diff:
> https://nl.wikipedia.org/w/index.php?title=Gebruiker:Romaine/vector.js&diff=37497149&oldid=37497060

This solution didn't work for long, it just broke all JavaScript for me on Meta (most prominently, [[m:WM:LS]]) with:

Timestamp: 09/06/2013 20:03:03
Error: TypeError: mw.toolbar is undefined
Source File: https://bits.wikimedia.org/meta.wikimedia.org/load.php?debug=false&lang=it&modules=user&only=scripts&skin=monobook&user=Nemo+bis&version=20130601T024617Z&*
Line: 1

[MediaWiki 	1.22wmf5 (bad4d50)]
Comment 14 Bartosz Dziewoński 2013-06-09 19:18:16 UTC
We fixed it with Nemo on IRC. For the record, this was because the script ran on all pages (not just edit ones). The simplest way to fix that was to wrap the button definitions in "if ( mw.toolbar ) { ... }".
Comment 15 Krinkle 2013-06-10 02:32:12 UTC
Closing this as wontfix.

The classic edit toolbar has long been deprecated. The default was replaced with WikiEditor on Wikimedia in a 2010 deployment. And since a few releases it is now bundled with new MediaWiki releases as well.

Since then development has started on [[mw:VisualEditor]] which will be launched soon.

Despite all that, I've spend a fair amount of effort over the last months to come up with a brand new API for the classic edit toolbar: mw.toolbar because mwCustomEditButtons is fundamentally flawed and cannot be repaired and kept up with the latest technologies.

Any and all issues you may have with mwCustomEditButtons are from hereon invalid. Please use mw.toolbar instead. Note that mw.toolbar has nothing to do with WikiEditor or VisualEditor, it is a new API for the classic editor you are so familiar with.

I can't make any promises for how long the classic toolbar and this new mw.toolbar will be supported, but at least it has been renewed to work with all the latest technologies so it should be a relatively cheap burdon for us to maintain.

So, long story short: Change:

> if ( wgAction === 'edit' ) {
>     mwCustomEditButtons[mwCustomEditButtons.length] = {...};
> }

to:

> if ( mw.toolbar ) {
>     mw.toolbar.addButton( { ... } );
> }

If there are any bugs with that, please file a new bug report and be sure to mention "mw.toolbar" in its subject. I'll do my best to address if/when there are any issues.
Comment 16 Helder 2013-06-27 18:28:46 UTC
(In reply to comment #10)
...
> I'd recommend we add some code to mediawiki.action.edit to periodically check
> mwCustomEditButtons. Then immediately deprecate it (not removing it, just
> marking it as deprecated) and phasing it out over the following weeks (I'd
> like
> to add it to the js-deprecations train roll-out in May which will introduce
> mw.log.deprecate and console.warn messages that users can use to easily see
> if
> they are using something that is deprecated).

I requested this on bug 50310 so we don't forget.

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


Navigation
Links