Page MenuHomePhabricator

[Regression] Minerva: Avoid onBeforePageDisplay for page/skin stylesheets
Closed, ResolvedPublic

Description

Follow-up from T233160 (/cc @Volker_E @Jdlrobson)

Change 674441 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Message box styles for Minerva come from core

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

+	public static function onBeforePageDisplay( OutputPage $out, Skin $skin ) {
+		if ( …) {
+			$out->addModuleStyles( [
+				'skins.minerva.messageBox.styles'
+			] );
+		}
+	}

Avoid using onBeforePageDisplay for the purpose of page-related or skin-related stylesheets. This means they are missing from Parse API requests and various use cases (including alignment with T140664). This is what Skin::getDefaultModules is for, as done via SkinMinerva->getSkinStyles for various other use cases already.

Notes

This task will hopefully lead to an understanding in how we add CSS to page views in skins.

Event Timeline

Perhaps as a separate task, but consider whether this is really deservant of another module. Minerva is booming at a rather high total of 23 registered bundle entry points.

The transfer size of this code seems to be 350 bytes after compression in isolation, or when compared to the main payload with and without this module, only 200 bytes (due to better shared compression). I don't know if that's worth not priming in the cache for better cache reuse, but let's say for now it's worth it in the long run to not support this class in content and omit it from page views. That means it should move out of the main payload. That doesn't mean it should get its own bundle entry point. The default approach should be, unless dictated otherwise by a demonstrated performance need, to add it to a different payload. Really any secondary minerva CSS payload of your choosing would work. Combining some of those may be "hard", but that's imho the price for doing this propery without incurring Technical-Debt.

(It's also worth taking into account what the long-term impact is of the overall maintainbility of our platform when we tolerate individual skins or extensions deciding to unload a core style class, that then seemingly-randomly does not work there on article pages but works elsewhere; with no way for MW or extensions to detect this, let alone controll it. And the impact of users re-implementing these styles in a default-on gadget, loaded unconditionally on all pages; instead of re-using core styles.)

11,801 - 11,596 = 205 B

$ curl 'https://he.m.wikipedia.org/w/load.php?modules=skins.minerva.messageBox.styles&only=styles' | gzip -9 | wc -c
357 B

$ curl 'https://he.m.wikipedia.org/w/load.php?lang=en-gb&modules=ext.dismissableSiteNotice.styles%7Cext.echo.styles.badge%7Cext.wikimediaBadges%7Cmediawiki.hlist%7Cmediawiki.ui.button%2Cicon%7Cmobile.init.styles%7Coojs-ui.styles.icons-alerts%7Cskins.minerva.amc.styles%7Cskins.minerva.base.styles%7Cskins.minerva.content.styles%7Cskins.minerva.content.styles.images%7Cskins.minerva.icons.loggedin%2Cwikimedia%7Cskins.minerva.loggedin.styles%7Cskins.minerva.mainMenu.icons%2Cstyles%7Cskins.minerva.mainPage.styles&only=styles&skin=minerva' | gzip -9 | wc -c
11,596 B

$ curl 'https://he.m.wikipedia.org/w/load.php?lang=en-gb&modules=ext.dismissableSiteNotice.styles%7Cext.echo.styles.badge%7Cext.wikimediaBadges%7Cmediawiki.hlist%7Cmediawiki.ui.button%2Cicon%7Cmobile.init.styles%7Coojs-ui.styles.icons-alerts%7Cskins.minerva.amc.styles%7Cskins.minerva.base.styles%7Cskins.minerva.content.styles%7Cskins.minerva.content.styles.images%7Cskins.minerva.icons.loggedin%2Cwikimedia%7Cskins.minerva.loggedin.styles%7Cskins.minerva.mainMenu.icons%2Cstyles%7Cskins.minerva.mainPage.styles|skins.minerva.messageBox.styles&only=styles&skin=minerva' | gzip -9 | wc -c
11,801 B

Minerva is booming at a rather high total of 23 registered bundle entry points.

The majority of these relate to icons. Can the performance team help prioritize T248912 ? Fixing that would allow us to remove approx 10 of these modules.

Recommendation here sounds good.

Note that the scope of this task is intended as a functional problem (or "bug"), not a performance issue. This isn't about the fact that the module is being loaded or its costs. It is about how it is loaded. The current way means that the stylesheet is missing for API consumers who render the content. Stylesheets that target elements in the content area, must be queued via the ParserOutput, not via the Skin OutputPage.

(The thread went on about whether the module is even needed and whether it can be merged, hence the last few comments are about that).

Change 888106 had a related patch set uploaded (by Kimberly Sarabia; author: Kimberly Sarabia):

[mediawiki/skins/MinervaNeue@master] [WIP] Remove onBeforePageDisplay for page/skin stylesheets

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

Change 888106 abandoned by Kimberly Sarabia:

[mediawiki/skins/MinervaNeue@master] [WIP] Remove onBeforePageDisplay for page/skin stylesheets

Reason:

Creating a new branch

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

All done here! Thanks Kim!

In case anyone else is wondering how the task is resolved when the only associated commit was abandoned... It was accidentally tagged with an unrelated task:

Change 888726 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Remove onBeforePageDisplay for Minerva

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