Page MenuHomePhabricator

[Regression] Minerva: Avoid onBeforePageDisplay for page/skin stylesheets
Open, MediumPublic

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.

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.

ovasileva raised the priority of this task from Low to Medium.May 25 2021, 7:16 PM
ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.