Page MenuHomePhabricator

The CSS of each gadget is included two times
Closed, ResolvedPublic

Description

I noticed that when I enable this gadget
https://test.wikipedia.org/wiki/Special:Gadgets/export/EditTools
and go to
https://test.wikipedia.org/w/index.php?title=TestPage&action=edit&debug=1
the HTML source code of the page contains two copies of
https://test.wikipedia.org/w/index.php?title=MediaWiki:Gadget-EditTools.css&oldid=144099

Indeed, searching for elements containing "ext.gadget.EditTools&only=styles" on Google Chrome 21.0.1180.89 I find:

<link rel="stylesheet" href="//test.wikipedia.org/w/load.php?debug=true&amp;lang=en&amp;modules=ext.gadget.EditTools&amp;only=styles&amp;skin=vector&amp;*">

and

<link type="text/css" rel="stylesheet" href="//test.wikipedia.org/w/load.php?debug=true&amp;lang=en&amp;modules=ext.gadget.EditTools&amp;only=styles&amp;skin=vector&amp;version=20120916T155758Z&amp;*">

The same happens for "ext.gadget.hlist-test&only=styles":

<link rel="stylesheet" href="//test.wikipedia.org/w/load.php?debug=true&amp;lang=en&amp;modules=ext.gadget.hlist-test&amp;only=styles&amp;skin=vector&amp;*">

<link type="text/css" rel="stylesheet" href="//test.wikipedia.org/w/load.php?debug=true&amp;lang=en&amp;modules=ext.gadget.hlist-test&amp;only=styles&amp;skin=vector&amp;version=20120916T155758Z&amp;*">

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Just ran into this again as a cause of a major reflow on english wikipedia. Really hoping that T92459 gets off the ground this time round.

Change 308096 had a related patch set uploaded (by Krinkle):
Implement support for specifying type=styles

https://gerrit.wikimedia.org/r/308096

Change 308096 merged by jenkins-bot:
Implement support for specifying type=styles

https://gerrit.wikimedia.org/r/308096

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

I'm reading and trying to understand https://gerrit.wikimedia.org/r/308096

Under "Effective difference", the first case is "Gadgets with only styles", the second case is "Gadgets with only scripts" and the third case says "Gadgets with scripts (with or without styles)". Surely, this "or without" should be removed? I mean, the third case has to be gadgets with scripts and styles, right?

There is also "by calling not calling".

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

Is this really correct? This is not how I understand it. The way I understand it, gadgets with styles only, or scripts only, won't need to have their definitions changed. Gadgets that have both styles and scripts can be changed to "type=general", assuming that the styles only affect HTML that is added by the script.

Hmm, it seems that styles modules created by gadgets are now concatenated before core modules, which is a behavioral change to how we used to sort inclusions (and different from debug=true order). It this a problem ?

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

Is this really correct? This is not how I understand it. The way I understand it, gadgets with styles only, or scripts only, won't need to have their definitions changed. Gadgets that have both styles and scripts can be changed to "type=general", assuming that the styles only affect HTML that is added by the script.

@Nirmos is correct. Gadgets with only styles or only scripts don't need to change anything.

I've written some additional text, posted at https://www.mediawiki.org/wiki/RL/MGU#Gadget_type.

Hmm, it seems that styles modules created by gadgets are now concatenated before core modules, which is a behavioral change to how we used to sort inclusions (and different from debug=true order).

Did this merge create the regression T147667 and https://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Gadget_CSS_order ?

I don't get the second point at the docs:

If the styles in your gadget are there to provide styling for things that are part of the skin or on the page, then the scripts and styles do not belong together in the same gadget. Convert the gadget to two separate gadgets. One gadget that is styles-only, and the other with the scripts. If the scripts gadget also has some styles, be sure to set "type=general" on that one.

For example, there is this gadget:

The CSS part applies to the skin of any talkpage, and ALSO to the pages that the JS part marks as discussion pages (e.g. village pumps). It looks unreasonable to create two gadgets for this and ask users to always enable both gadgets together (or disable both together).

There are other gadgets in the same situation (modifying parts of the default skin to make it consistent with the parts created by the gadget itself).

I don't get the second point at the docs:

If the styles in your gadget are there to provide styling for things that are part of the skin or on the page, then the scripts and styles do not belong together in the same gadget. Convert the gadget to two separate gadgets. One gadget that is styles-only, and the other with the scripts. If the scripts gadget also has some styles, be sure to set "type=general" on that one.

For example, there is this gadget: MediaWiki:Gadget-FeedbackHighlight.cssMediaWiki:Gadget-FeedbackHighlight.js
The CSS part applies to the skin of any talkpage, and ALSO to the pages that the JS part marks as discussion pages (e.g. village pumps). It looks unreasonable to create two gadgets for this and ask users to always enable both gadgets together [..]

If it's okay for the page/skin-related styles to load later with the rest of the gadget, then a single gadget with type=general will work fine. But loading styles early will require a non-js gadget.

We can probably use dependencies and hidden to avoid the need for end-users to enable both of these. Only one needs to be exposed.

However, I realise now that this doesn't work. The dependency would be loaded before the JS (thus satisfying the idea of a dependency), but not through the style queue during page load, since it wasn't explicitly requested as part of the page.

Making the dependency the other way around doesn't work either, since we don't (and can't generally) resolve dependencies for style modules.

@He7d3r If we work together and find an elegant way to trigger a load of both modules, would you still oppose to them being separate modules in the definition?

@Krinkle probably not (but it looks weird at first).

By the way, this is still the case on Wikimedia Commons for:

  • CollapsibleTemplates
  • WatchlistNotice
  • AjaxQuickDelete

Is there a place to maintain such list of gadgets yet to migrate?

A tech news note would be helpful too, so we can indicate the wikis to migrate, someone reports on fr.wikipedia the warning for DeluxeHistory.

Dereckson added a project: User-notice.

Reopening, so we can proceed to add migrate instructions for User-notice

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

Could you prepare a migration text?

I'll include it in this week's issue of Tech News (distributed on Monday).

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

Could you prepare a migration text?

This is a misunderstanding. Please see the comments from Krinkle and me above.

There should probably be a note in User-notice with the actions that gadget maintainers will have to do to take advantage of this....

For those following along at home... After the update is deployed next week, gadget authors will be able to add a "type=styles" attribute to gadget definitions which specifies that a gadget only contains styles (no JavaScript). This will prevent ResourceLoader from unnecessarily loading the CSS twice.

Could you prepare a migration text?

This is a misunderstanding. Please see the comments from Krinkle and me above.

I'm in charge of Tech News this week.
If I understand correctly, it appears that change is not the best one. But I'm not sure.

Can anyone confirm that? Thanks.

probably not (but it looks weird at first).

Here's a sketch: https://gerrit.wikimedia.org/r/#/c/322234/. Let me know what you think.

can we please also have a message to the wikitech-ambassadors with some text around the solution. the console message of type=general is not particularly informative. Thx

can we please also have a message to the wikitech-ambassadors with some text around the solution. the console message of type=general is not particularly informative. Thx

@Billinghurst Could you review the documentation at https://www.mediawiki.org/wiki/RL/MGU#Gadget_type?

can we please also have a message to the wikitech-ambassadors with some text around the solution. the console message of type=general is not particularly informative. Thx

@Billinghurst Could you review the documentation at https://www.mediawiki.org/wiki/RL/MGU#Gadget_type?

@Krinkle Thanks, though could we please have examples giving example code. Plus Is it only the choice of "general" or "styles" forcing both before or both after? If you want to apply the styles though javascript can come after what would need to happen, can you define them split?

  • popups[ResourceLoader|type=general|type=styles]|popups.js|navpop.css ????

@Billinghurst No, type can only have 1 value. Can you explain what you were trying to do? Please know that a single gadget with type=general modules is allowed to have both styles and scripts.

Change 337954 had a related patch set uploaded (by Krinkle):
Change warning link about type=general from phab to mediawiki.org

https://gerrit.wikimedia.org/r/337954

probably not (but it looks weird at first).

Here's a sketch: https://gerrit.wikimedia.org/r/322234. Let me know what you think.

rEGADdc4ea6cb2139: Implement 'peers' feature for loading extra styles-type gadgets has landed and will roll out this week. I've written up some documentation at https://www.mediawiki.org/wiki/RL/MGU#Gadget_peers and updated https://www.mediawiki.org/wiki/RL/MGU#Gadget_type.

Change 337954 merged by jenkins-bot:
Change warning link about type=general from phab to mediawiki.org

https://gerrit.wikimedia.org/r/337954

wee, finally a way to counter Twinkle's vector menu FOUC !

Draft message for Tech News (User-notice):

Change 338004 had a related patch set uploaded (by Krinkle):
Change warning link about type=general from phab to mediawiki.org

https://gerrit.wikimedia.org/r/338004

Change 338004 merged by jenkins-bot:
Change warning link about type=general from phab to mediawiki.org

https://gerrit.wikimedia.org/r/338004

Change 353586 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Gadgets@master] Remove duplicate loading of styles (assume type=general if content is mixed)

https://gerrit.wikimedia.org/r/353586

Change 353586 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Remove duplicate loading of styles (assume type=general if content is mixed)

https://gerrit.wikimedia.org/r/353586

Jdforrester-WMF subscribed.

Mass-moving all items tagged for MediaWiki 1.30.0-wmf.3, as that was never released; instead, we're using -wmf.4.

Now that by default, and automatically, all gadgets get type "general", except CSS-only (and without dependencies) gadgets which get type "styles",

should we continue to specify the type (namely, for JS+CSS gadgets adding a "type=general"), or is it fine to omit it?

This comment was removed by Od1n.

Now that by default, and automatically, all gadgets get type "general", except CSS-only (and without dependencies) gadgets which get type "styles",

should we continue to specify the type (namely, for JS+CSS gadgets adding a "type=general"), or is it fine to omit it?

Still relevant question.

Docs at

seem to be in a dire need of an update, which hasn't been done despite this has been resolved 6 years ago.

It's fine to omit the type. It'll automatically take the type as "styles" if possible (no JS + no dependencies) and general otherwise. Feel free to update the docs!