Page MenuHomePhabricator

Merge mediawiki.toc.styles styles into ResourceLoaderSkinModule
Open, MediumPublic5 Estimated Story Points

Description

This task is to rename the mediawiki.toc.styles ResourceLoader module in preparation for repurposing. The intended usage is:

  • High-traffic, every pageview styles common for skins, at least Vector and Minerva Neue. The module is enqueued by Skin:getDefaultModules() by default (and thus in Vector and Minerva).
  • Eventual home for at least the MediaWiki checkbox hack styles that may be used in several contexts including: Vector's collapsible table of contents (currently checkboxtoggle), Vector's more menu, Vector's sidebar, MinervaNeue sidebar (currently ToggleList), MinervaNeue user menu (currently ToggleList), MinervaNeue page overflow menu (currently ToggleList) implementation in MinervaNeue (currently ToggleList). (See T246419.)
    • We think it will at least these styles will be universal for all checkbox hack consumers:
checkboxHack.less
.mw-checkbox-hack-checkbox {
	position: absolute;
	// Always lower the checkbox behind the foreground content.
	z-index: -1;
	// The checkbox `display` cannot be `none` since its focus state is used for other selectors.
	opacity: 0;
}
// [todo] replace bindToggleOnClick() with `:focus-visible` when available. See https://css-tricks.com/almanac/selectors/f/focus-visible/.

The module has very little usage currently and is small (69 lines of Less and CSS).

Acceptance criteria

  • A new name is chosen as part of this task (ResourceLoaderSkinModule)
  • mediawiki.toc.styles is renamed without breaking existing clientele. (the module is deprecated and all skins are using an opt in to new features policy for ResourceLoaderSkinModule)
  • The unique table of contents styling inside Minerva is not disrupted by the change (Minerva doesn't use opt in policy so will not be disrupted)
  • Add integration tests for ResourceLoaderSkin class

Open questions

  • Are there other styles we want to plan to put there now?
  • Is this a catchall common styles module for all skins?

Event Timeline

Restricted Application added a project: Performance-Team. · View Herald TranscriptMay 14 2020, 2:25 PM
Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript

What's the proposed name?

Niedzielski updated the task description. (Show Details)May 14 2020, 4:20 PM

The module is not loaded by either skin at time of writing but is small […]

While Vector doesn't queue it explicitly, it is enqueued by the Skin class by default (and thus in Vector) on basically all pages. Minerva deqeueus it in SkinMinerva::getDefaultModules, in favour of its own scripts/styles.

Niedzielski updated the task description. (Show Details)May 15 2020, 1:08 AM
Niedzielski updated the task description. (Show Details)May 20 2020, 12:43 AM

The module is not loaded by either skin at time of writing but is small (69 lines of Less and CSS).

this module is loaded by all skins including Minerva and Vector as part of Skin:getDefaultModules in includes/skins/Skin.php
I think the idea here is that we want to avoid adding a new style module for the checkbox hack. We'd rather bundle both table of contents and checkbox styles together. Is that right Stephen?

My suggestion would be to create the newly desired module (e.g. mediawiki.interface) as empty and add it alongside mediawiki.toc.styles

Alternatively we could lean on ResourceLoaderSkinModule for this purpose. The table of content styles and in future the checkbox styles could live there.

Krinkle added a comment.EditedMay 20 2020, 1:51 AM

Either sounds good to me.

ResourceLoaderSkinModule helps in so far that it allows skins to load/unload/override in a cleaner way. Although its current feature config makes it difficult to introduce new defaults in a way that isn't a hard break. But.. that's a solvable problem. We can also move toward that later and go with the renamed/new module first.

Introducing a new module is effectively the same as renaming, given the need for (non-deprecated) compat during the transition. That might be a better first step so as to keep different endavours not too entangled.

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.

Change 589692 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/core@master] mediawiki.page.ready: add checkbox hack JavaScript

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

Change 589692 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.page.ready: add checkbox hack JavaScript

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

Jdlrobson triaged this task as Medium priority.May 28 2020, 5:28 PM

@Niedzielski could you update the task description/title ?
My understanding is the goal is to move one of the rules in Vector/resources/skins.vector.styles/checkboxHack.less into a core stylesheet.
The question becomes which ResourceLoaderModule to put it. Is that correct?

I personally would favor ResourceLoaderSkinModule over medawiki.toc.styles as I think longer term that should be merged into ResourceLoaderSkinModule

In which case I would update https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderSkinModule.php#L118 to set the new feature as a default. A follow up patch would remove the code from Vector in favor of the new skin module code.

If I've understood correctly and that sounds okay I think this can move to upcoming.

Niedzielski removed Niedzielski as the assignee of this task.Jun 7 2020, 8:48 PM
Niedzielski updated the task description. (Show Details)

Vector/resources/skins.vector.styles/checkboxHack.less into a core stylesheet.

@Jdlrobson, this is true. We want to use a Core ResourceLoader module to share the JavaScript and Less needed for the checkbox hack in any MediaWiki context. However, the original intent of the task was to repurpose mediawiki.toc.styles for arbitrary high-traffic styles including the checkbox hack.

I personally would favor ResourceLoaderSkinModule over medawiki.toc.styles as I think longer term that should be merged into ResourceLoaderSkinModule

Thank you for the reference but I know very little about the tradeoffs of one over the other. My inclination is that I would like all dependencies to be explicit. I trust you and @Krinkle to make the correct judgement on this. If you do choose ResourceLoaderSkinModule, I think that leaves medawiki.toc.styles as a "loose end" and would suggest some kind of follow up action, possibly including termination.

I've attempted to incorporate the other feedback in the comments into the task description but feel free to edit liberally.

Niedzielski updated the task description. (Show Details)Jun 7 2020, 9:11 PM
Jdlrobson renamed this task from Rename mediawiki.toc.styles ResourceLoader module to Checkbox and mediawiki.toc.styles styles should be merged into a single ResourceLoader module.Jun 10 2020, 5:23 PM

I like the idea of composing more of these "core defaults but ultimately skin-owned decision" helper code through ResourceLoaderSkinModule. However, I think it does emphasise more that we need a better way of controling its composition. Right now its "features" array is considered complete and fully hardcodes what is included. This makes it unsuitable for this as it allows us no way to add move things into it whilst preserving the status quo in a compatible manner.

Allowing full control (and thus opt-out of the stability policy) could stil work, but we'd probably want a smoother and less maintenance-heavy way of controlling the default. A minimalist way to do that could be to invert the array to take booleans, where false ensures exclusion, and true ensures inclusion. With unspecified being the default.

This minimal layer of indirection would effectively allow translation from a boolean "options" object to the (definitive) "features" array we have today, and thus open the path for all other skin-related thigns to be moved in here bit by bit. And makes things like T242177#5946331 much easier for us to handle. (Do what we want for WMF and new/existing skins just work.)

Jdlrobson updated the task description. (Show Details)Aug 18 2020, 5:33 PM
Jdlrobson updated the task description. (Show Details)

Change 621039 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Allow core to default features to on in ResourceLoaderSkinModule

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

Change 621040 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] WIP: Make mediawiki.toc.styles a ResourceLoaderSkinModule style

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

ovasileva set the point value for this task to 5.Aug 25 2020, 4:51 PM

Change 621039 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Give SkinModule 'features' option an extensible default

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

Change 627383 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MonoBook@master] Monobook should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627384 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Modern@master] Modern should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627385 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Vector should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627386 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/CologneBlue@master] CologneBlue should use opt-in policy for ResourceLoaderSkinModule features

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

Jdlrobson renamed this task from Checkbox and mediawiki.toc.styles styles should be merged into a single ResourceLoader module to Merge mediawiki.toc.styles styles into ResourceLoaderSkinModule.Sep 14 2020, 10:26 PM
Jdlrobson updated the task description. (Show Details)

I think the checkbox CSS should only be merged into ResourceLoaderSkinModule when we have 2 skins using it. Minerva is the obvious candidate so I've folded this into T259198 and decided its out of scope as it's not clear to me right now what CSS we want to upstream.

Change 627385 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Vector should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627383 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Monobook should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627386 merged by jenkins-bot:
[mediawiki/skins/CologneBlue@master] CologneBlue should use opt-in policy for ResourceLoaderSkinModule features

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

Change 627384 merged by jenkins-bot:
[mediawiki/skins/Modern@master] Modern should use opt-in policy for ResourceLoaderSkinModule features

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

Change 628942 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Allow for expansion of styles with addition of table of contents code

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

Change 628942 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Allow for expansion of styles with addition of table of contents code

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

Jdlrobson reassigned this task from Jdlrobson to Krinkle.Oct 5 2020, 5:33 PM

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/621040 is waiting for input from someone familiar with ResourceLoaderSkinModule. @Krinkle note this is not a super high priority but I assume you would make sense as the reviewer.

Moving to backlog as it's not a super high priority from our perspective. We'll move it back in when the performance team has bandwidth.

Krinkle reassigned this task from Krinkle to Jdlrobson.Tue, Nov 3, 4:27 PM

Reviewed.

Change 639240 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Drop table of contents test assertions around visibility

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

Change 639240 merged by jenkins-bot:
[mediawiki/core@master] Drop table of contents test assertions around visibility

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

Change 641246 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Bump the bundlesize of skins.vector.styles

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

Change 641246 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Bump the bundlesize of skins.vector.styles

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

Change 621040 merged by jenkins-bot:
[mediawiki/core@master] Add new 'toc' feature to ResourceLoaderSkinModule

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