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;*">

Details

Reference
bz40284
Related Gerrit Patches:

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
He7d3r created this task.Sep 16 2012, 4:12 PM

This was also brought up in bug 29693 comment 9, see my response there for an explanation of why this is currently happening.

I was going to duplicate with bug 29693. Whatever solution (if any) is considered, it can be applied to Gadgets similarly.

One possible way (though not an option for gadget) is to require that modules be split in a part that contains style for server output and a module with javascript and styling for that.

That's how we do it in core for some of these cases. Note that this split up can not (should not) be automated because some stylesheets are only relevant in a scripted context and visa versa.

The only problem is that we don't have a way to tell gadgets that it should be loaded as a <link> only with only=styles. Effectively adding a new type of module (dynamic bottom, dynamic top, static top), which can't have scripts.

This double loading is going to occur with gadgets and the 'mediawiki.ui.button' module. We recommend gadgets explicitly load their required modules; we will probably add addModuleStyles( 'mediawiki.ui.button' ) on all pages so that simple templates like {{Blue button}} work without JavaScript. So the button CSS will be added to a style tag in the DOM by the former and added as a link by the latter.

The fix could be to add a separate mw.loader.loadCSS/usingCSS() function that can check to see if the CSS has already been loaded using addModuleStyles.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 19 2015, 2:33 PM
He7d3r updated the task description. (Show Details)Jan 6 2016, 9:23 PM
He7d3r set Security to None.
He7d3r removed a subscriber: wikibugs-l-list.
Catrope removed a subscriber: Catrope.Jan 15 2016, 11:00 PM
TheDJ added a subscriber: TheDJ.May 23 2016, 8:44 AM

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.

Majr added a subscriber: Majr.May 26 2016, 5:55 AM
Krinkle claimed this task.Sep 1 2016, 9:25 PM

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

TheDJ added a comment.Sep 30 2016, 1:37 PM

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.

Nirmos added a subscriber: Nirmos.Oct 5 2016, 10:03 AM

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.

TheDJ added a comment.Oct 7 2016, 8:01 AM

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 closed this task as Resolved.Oct 12 2016, 5:18 PM
Krinkle removed a project: Patch-For-Review.

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

Dereckson added a subscriber: Dereckson.EditedNov 1 2016, 3:50 PM

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 reopened this task as Open.Nov 1 2016, 3:56 PM
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?

Johan added a subscriber: Johan.Nov 1 2016, 4:43 PM

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

Nirmos added a comment.Nov 2 2016, 8:48 AM

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.

Krinkle added a comment.EditedNov 19 2016, 12:28 AM

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 ????
Krinkle added a comment.EditedDec 18 2016, 8:02 PM

@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

TheDJ added a comment.Feb 15 2017, 9:45 PM

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

Krinkle closed this task as Resolved.Feb 17 2017, 12:10 AM

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 added a subscriber: Jdforrester-WMF.

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