Page MenuHomePhabricator

Site and Gadget CSS should apply after core and skin CSS
Closed, ResolvedPublic

Description

I love the feature of black background with green text on Wiki!

How it works
First, go to the "Appearance" tag and switch to the "Monobook" skin (by switching to the Monobook radio button and hitting the Save button at the very bottom). Then go to the Gadgets tab and, at the bottom of the "Appearance" section in that tab, check the box for "Use a black background with green text on the Monobook skin" and hit the Save button at the bottom again.

Problem
It usually works well. However, starting from yesterday, anomaly happened since the left side of screen was no longer black, and the grey icon of Wikipedia was shifted from very left side to top-middle area so that it blocked part of the main text on the top. Any solution for this? Thank you!

See Also: https://en.wikipedia.org/w/index.php?title=Wikipedia:Village_pump_(technical)&oldid=743174734#Display_problem.2C_MonoBook.2C_black_background_gadget

Event Timeline

Saucelord created this task.Oct 7 2016, 6:32 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 7 2016, 6:32 PM
Aklapper renamed this task from Use a black background with green text on the Monobook skin to "Use a black background with green text" gadget on Monobook skin: Shows grey areas; Logo displayed above text.Oct 7 2016, 8:23 PM
Aklapper renamed this task from "Use a black background with green text" gadget on Monobook skin: Shows grey areas; Logo displayed above text to Gadget CSS overwritten by default skin CSS ("Use a black background with green text" gadget on Monobook broken).Oct 7 2016, 8:33 PM
Aklapper added a subscriber: matmarex.
Aklapper removed a subscriber: matmarex.Oct 8 2016, 9:00 AM

https://en.wikipedia.org/w/load.php?debug=true&lang=en&modules=ext.gadget.Blackskin&only=styles&skin=monobook is ignored as the page uses the default Monobook CSS definitions instead.

This is a regression triggered by T42284#2698823

Aklapper updated the task description. (Show Details)
Aklapper added a subscriber: Krinkle.

In the meantime I've updated Blackskin.css with a higher selector specificity and improved display in the Vector skin.

Krinkle closed this task as Declined.Feb 14 2017, 3:40 AM

https://en.wikipedia.org/w/load.php?debug=true&lang=en&modules=ext.gadget.Blackskin&only=styles&skin=monobook is ignored as the page uses the default Monobook CSS definitions instead.
This is a regression triggered by T42284#2698823

Previously they loaded twice. Once in the <head>, alongside the skin css; where both are taken into account before the browser first renders the page. And a second time, 1-5 seconds later, when the JavaScript payload has been fully initialised.

The bug of the underspecified selector may've gone unnoticed as users presumably weren't aware of it loading twice and just thought that the delayed load was working and intended and probably didn't realise it's supposed to work without a delay or flash of non-black variant.

Now that the delayed secondary load is gone, the first load happens only, and it's uncovered that it's selector was not effective as an override. The fact that an equal strength selector overrides a skin one was an undocumented side-effect of a change that was made in the Gadgets extension about 1-2 years ago.

The fact that an equal strength selector overrides a skin one was an undocumented side-effect of a change that was made in the Gadgets extension about 1-2 years ago.

It was started in 2008 and last edited in 2012. According @Catrope to T31693#322480 loading gadget CSS before the skin CSS and then loading it again was a timing hack. The original order IIRC was: Skin CSS, Gadget CSS, User CSS

Dispenser reopened this task as Open.Apr 29 2017, 2:30 PM

Reopening as @Krinkle analysis and history are wrong and my hack of higher specificity is causing me to duplicate CSS

Aklapper triaged this task as Lowest priority.Apr 29 2017, 3:03 PM

How does that challenge the decision to decline this task?

I was under the impression the task was named "Gadget CSS overwritten by default skin CSS" not "Find workaround for resource loader".

Krinkle renamed this task from Gadget CSS overwritten by default skin CSS ("Use a black background with green text" gadget on Monobook broken) to Site and Gadget CSS should apply after core and skin CSS.Apr 29 2017, 7:52 PM
Krinkle raised the priority of this task from Lowest to Normal.
Krinkle added a comment.EditedApr 29 2017, 8:01 PM

You're right.

  1. This did work in 2008 before ResourceLoader existed.
  2. It also worked in 2010 with ResourceLoader, since we always loaded stylesheets like this: 1) core, skin, extensions; 2) cascading "DynamicStyles" marker; 3) site, gadget and user css
  3. Sometime between 2011 and 2015, Gadgets started to support ResourceLoader but did so in a way that caused the styles to be loaded a second time with JavaScript after the page finished loading.
  4. At some point an optimisation was made to consolidate some of the stylesheet requests. Care was taken to keep the site, global user js/css, and user stylesheets in their correct cascading position, but, the gadget stylesheets somehow made it into the main request. Now, only alphabetical sorting order dictates cascading order within this request. And gadgets sort before skins. This error was not noticed because of the bug of styles loading a second time; which kind of "corrected" the problem.
  5. At a later point, JavaScript was made asynchronous, which made this bug worse.
  6. More recently, the bug of loading styles twice was fixed, which finally exposed the bug from point 3.

Change 351226 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Gadgets@master] Move gadget styles from main stylesheet request to site request

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

Krinkle claimed this task.May 2 2017, 12:27 AM
Gilles moved this task from Inbox to Doing on the Performance-Team board.May 3 2017, 7:37 PM

Change 351226 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Move gadget styles from main stylesheet request to site request

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

Krinkle closed this task as Resolved.May 5 2017, 6:36 PM
Krinkle reopened this task as Open.EditedMay 11 2017, 5:56 PM

Change 351226 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Move gadget styles from main stylesheet request to site request
https://gerrit.wikimedia.org/r/351226

Reverted due to causing:

We cannot use group=site because OutputPage assumes all group=site modules that load themselves with addModuleStyles do not use unsupported hacks like having a module contain both scripts and styles but using addModuleStyles to load it. This behaviour has been deprecated since last year and the Gadgets extension was already well-underway on solving this, but it's not finished yet. The Gadgets extension still doesn't assume type=general by default for gadgets that contain both scripts and styles, and still loads them twice (once with addModuleStyles, once with addModules).

We cannot use group=site because per T37562, while we don't support @import everywhere, there is exactly one place where we support it: In the first page of the 'site' or 'user' wiki modules. In other words: Common.css. Adding group=site to gadgets fixed this bug (T147667) by moving its request from before the marker to after the marker, but it also merged it with the 'site' module and because ext.gadget sorts before site it means the gadget is now the first page in that stylesheet, which means @import no longer worked.

We'll have to figure a way to:

  1. To fix T147667: Make gadget styles load after the marker.
  2. To avoid T165031: Cannot use group=site as those are assumed by OutputPage::getRlClient to be styles-only if loaded with addModuleStyles.
  3. To avoid T165040: Not in the same request as 'site.styles' or at least not before it in that request.

Point 2: This is not an issue if we land https://gerrit.wikimedia.org/r/#/c/353586/ first, so that we don't load non-style modules with addModuleStyles().

Point 3: To fix this, we'll need a minor refactor in OutputPage to explicitly give site.styles and user.styles its own request, which we probably should've been doing anyway.

I noticed earlier that a similar bug to point 3 already exists in WMF production now due to the GlobalCssJs extension. It adds its module ext.globalCssJs.user.styles to the 'user' group. Which means, when viewing pages on meta.wikimedia.org, it will be in the same request as user.styles which means @import is broken if used in local common.css and you have a non-empty global.css.

Change 353694 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Add tests for OutputPage::buildExemptModules

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

Change 353695 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Ensure user.styles and site.styles having their own request

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

Change 353695 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Ensure user.styles and site.styles having their own request
https://gerrit.wikimedia.org/r/353695

After this is merged, we can re-apply the reverted 86905f8d78afaea2f5c2de99bd3fe6149bca72a5 (https://gerrit.wikimedia.org/r/351226).

Change 353694 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Add tests for OutputPage::buildExemptModules

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

Change 353695 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Ensure user.styles and site.styles having their own request

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

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.

Change 357657 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/Gadgets@master] Move gadget styles from main stylesheet request to site request

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

Change 357657 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Move gadget styles from main stylesheet request to site request

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

Krinkle closed this task as Resolved.Jun 21 2017, 9:35 PM

Indeed. Verified on the beta cluster.

https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page

Before:

<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=
 ext.PerformanceInspector.noscript,
 ext.gadget.wmfFR2011Style,
 ext.uls.interlanguage%7Cext.visualEditor.desktopArticleTarget.noscript%7Cext.wikimediaBadges%7Cmediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.sectionAnchor%7Cmediawiki.skinning.interface%7Cskins.vector.styles%7Cwikibase.client.init&amp;only=styles&amp;skin=vector"/>
<script async="" src="/w/load.php?debug=false&amp;lang=en&amp;modules=startup&amp;only=scripts&amp;skin=vector"></script>
<meta name="ResourceLoaderDynamicStyles" content=""/>
<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=ext.gadget.wmfFR2011Style&amp;only=styles&amp;skin=vector"/>
<link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=site.styles&amp;only=styles&amp;skin=vector"/>
<meta name="generator" content="MediaWiki 1.30.0-alpha"/>

After:

  <link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=
  ext.PerformanceInspector.noscript,
- ext.gadget.wmfFR2011Style,
  ext.uls.interlanguage%7Cext.visualEditor.desktopArticleTarget.noscript%7Cext.wikimediaBadges%7Cmediawiki.legacy.commonPrint%2Cshared%7Cmediawiki.sectionAnchor%7Cmediawiki.skinning.interface%7Cskins.vector.styles%7Cwikibase.client.init&amp;only=styles&amp;skin=vector"/>
  <script async="" src="/w/load.php?debug=false&amp;lang=en&amp;modules=startup&amp;only=scripts&amp;skin=vector"></script>
  <meta name="ResourceLoaderDynamicStyles" content=""/>
+ <link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=ext.gadget.wmfFR2011Style&amp;only=styles&amp;skin=vector"/>
  <link rel="stylesheet" href="/w/load.php?debug=false&amp;lang=en&amp;modules=site.styles&amp;only=styles&amp;skin=vector"/>
  <meta name="generator" content="MediaWiki 1.30.0-alpha"/>