Page MenuHomePhabricator
Create Task
Maniphest T147667

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

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

Details

SubjectRepoBranchLines +/-
Move gadget styles from main stylesheet request to site requestmediawiki/extensions/Gadgetsmaster+4 -0
Move gadget styles from main stylesheet request to site requestmediawiki/extensions/Gadgetsmaster+4 -0
resourceloader: Ensure user.styles and site.styles having their own requestmediawiki/coremaster+16 -3
resourceloader: Add tests for OutputPage::buildExemptModulesmediawiki/coremaster+81 -1
Customize query in gerrit

Related Objects

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 added projects: WMF-General-or-Unknown, Regression.
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 added a project: MediaWiki-extensions-Gadgets.Oct 8 2016, 9:01 AM
Aklapper updated the task description. (Show Details)
Aklapper mentioned this in T42284: The CSS of each gadget is included two times.
Aklapper added a subscriber: Krinkle.
EvilXANA subscribed.Oct 18 2016, 5:15 AM
DuncanHill subscribed.Nov 12 2016, 1:33 PM
Dispenser subscribed.Feb 14 2017, 1:27 AM

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
In T147667#2701268, @Aklapper wrote:

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.

Krinkle edited projects, added MediaWiki-ResourceLoader; removed WMF-General-or-Unknown.Feb 14 2017, 3:40 AM
Dispenser added a subscriber: • Catrope.Feb 14 2017, 1:06 PM

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?

Dispenser added a comment.Apr 29 2017, 3:09 PM

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 Medium.
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.
Krinkle moved this task from Inbox to Confirmed Problem on the MediaWiki-ResourceLoader board.Apr 29 2017, 8:04 PM
Krinkle added a project: Performance-Team.
gerritbot subscribed.May 2 2017, 12:27 AM

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

gerritbot added a project: Patch-For-Review.May 2 2017, 12:27 AM
Krinkle claimed this task.May 2 2017, 12:27 AM
• Gilles moved this task from Inbox, needs triage to Doing (old) on the Performance-Team board.May 3 2017, 7:37 PM
gerritbot added a comment.May 4 2017, 1:16 AM

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 4 2017, 2:00 AM
Krinkle moved this task from Confirmed Problem to Assigned on the MediaWiki-ResourceLoader board.May 4 2017, 4:14 AM
Krinkle closed this task as Resolved.May 5 2017, 6:36 PM
Krinkle removed projects: Patch-For-Review, MediaWiki-ResourceLoader.
matmarex mentioned this in T165040: MediaWiki:Common.css not applied if it uses '@import' rules and user has any style-only gadgets enabled (works when using ?debug=true and when disabling all gadgets).May 11 2017, 2:09 PM
Krinkle reopened this task as Open.EditedMay 11 2017, 5:56 PM
In T147667#3233920, @gerritbot wrote:

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.

Krinkle removed a project: MW-1.30-release-notes (WMF-deploy-2017-05-09_(1.30.0-wmf.1)).May 11 2017, 6:13 PM
gerritbot added a comment.May 12 2017, 10:31 PM

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

gerritbot added a project: Patch-For-Review.May 12 2017, 10:31 PM

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

Krinkle added a comment.May 12 2017, 10:32 PM
In T147667#3259929, @gerritbot wrote:

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).

Krinkle added a project: MediaWiki-ResourceLoader.May 12 2017, 10:32 PM
gerritbot added a comment.May 23 2017, 1:00 AM

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

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

ReleaseTaggerBot added a project: MW-1.30-release-notes.May 23 2017, 2:00 AM
gerritbot added a comment.May 26 2017, 3:39 PM

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

ReleaseTaggerBot edited projects, added MW-1.30-release-notes (WMF-deploy-2017-05-30_(1.30.0-wmf.3)); removed MW-1.30-release-notes.May 26 2017, 4:00 PM
Jdforrester-WMF edited projects, added MW-1.30-release-notes (WMF-deploy-2017-06-06_(1.30.0-wmf.4)); removed MW-1.30-release-notes (WMF-deploy-2017-05-30_(1.30.0-wmf.3)).Jun 6 2017, 8:38 PM
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.

gerritbot added a comment.Jun 7 2017, 6:36 PM

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

Krinkle removed a project: MW-1.30-release-notes (WMF-deploy-2017-06-06_(1.30.0-wmf.4)).Jun 21 2017, 6:56 PM
gerritbot added a comment.Jun 21 2017, 8:32 PM

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-06-27_(1.30.0-wmf.7)).Jun 21 2017, 9:00 PM
Jdforrester-WMF removed a project: Patch-For-Review.Jun 21 2017, 9:17 PM

Resolved?

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"/>
Dispenser awarded a token.Jun 22 2017, 3:12 AM
Dispenser mentioned this in T191941: Gadget CSS should be applied after Site styles.Apr 10 2018, 11:44 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits