Page MenuHomePhabricator

Merge ResourceLoaderFileModule and ResourceLoaderImageModule (or make latter a mixin)
Open, MediumPublic

Description

A ResourceLoaderImageModule is needed to ship icons in MediaWiki so if you need icons you will need at least one additional ResourceLoader module. In many situations this should be unnecessary. For example a skin is already likely to have two modules - skin.name.styles and skin.name.scripts. Either could ship those icons.

In MobileFrontend/Minerva this problem is well illustrated - In Minerva there are 8 modules shipping icons. The skins.minerva.mainMenu.icons module however could be merged into skins.minerva.mainMenu.styles and reduce the ResourceLoader modules by one leading to less modules overall and a much smaller startup dependency tree.

Please consider allowing ResourceLoaderFileModules to support icons so that we can reduce the amount of modules that are needed.

Event Timeline

Krinkle triaged this task as Medium priority.Apr 5 2020, 12:09 AM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

For better understanding of how the image module works, I went to https://meta.wikimedia.org/w/load.php?modules=skins.minerva.content.styles.images and manually cleaned up the CSS included, which with formatting is

toast.mw-notification-type-error,
.mw-notification.mw-notification-type-error {
	background-image: url(/w/load.php?modules=skins.minerva.content.styles.images&image=toast.mw-notification-type-error,.mw-notification.mw-notification-type-error&format=rasterized&skin=fallback&version=12akb);
	background-image: linear-gradient(transparent,transparent),url(/w/load.php?modules=skins.minerva.content.styles.images&image=toast.mw-notification-type-error,.mw-notification.mw-notification-type-error&format=original&skin=fallback&version=12akb)
}
a.external {
	background-image: url(/w/load.php?modules=skins.minerva.content.styles.images&image=a.external&format=rasterized&lang=qqx&skin=fallback&version=12akb);
	background-image: linear-gradient(transparent,transparent),url(/w/load.php?modules=skins.minerva.content.styles.images&image=a.external&format=original&lang=qqx&skin=fallback&version=12akb)
}
a.external--reference {
	background-image: url(/w/load.php?modules=skins.minerva.content.styles.images&image=a.external&variant=reference&format=rasterized&lang=qqx&skin=fallback&version=12akb);
	background-image: linear-gradient(transparent,transparent),url(/w/load.php?modules=skins.minerva.content.styles.images&image=a.external&variant=reference&format=original&lang=qqx&skin=fallback&version=12akb)
}

this comes from the module definition

"skins.minerva.content.styles.images": {
	"class": "ResourceLoaderImageModule",
	"selectorWithoutVariant": "{name}",
	"selectorWithVariant": "{name}--{variant}",
	"defaultColor": "#36c",
	"useDataURI": false,
	"variants": {
		"reference": {
			"color": "#69f"
		}
	},
	"images": {
		"toast.mw-notification-type-error,.mw-notification.mw-notification-type-error":
			"resources/skins.minerva.content.styles.images/error.svg",
		"a.external": {
			"file": {
				"ltr": "resources/skins.minerva.content.styles.images/link-external-ltr.svg",
				"rtl": "resources/skins.minerva.content.styles.images/link-external-rtl.svg"
			},
			"variants": [
				"reference"
			]
		}
	}
},

So the image module just adds background images based on the selectors (and possible variants) configured?
What about adding this as an extra option to ResourceLoaderFileModule, to configure multiple sub image modules? Eg in a normal ResourceLoaderFileModule

"some.module.name": {
	"styles": [
		"resources/file.css",
		"resources/other.less"
	],
	"images": [
		{
			"class": "ResourceLoaderImageModule",
			"selector": ".skin-icon-bad-selector-{name}",
			"images": {
				"error": "resources/icons/error.svg",
				"warning": "resources/icons/warning.svg"
			}
		},
		{
			"class": "ResourceLoaderImageModule",
			"selector": ".skin-icon-good-selector-{name}",
			"images": {
				"success": "resources/icons/success.svg",
				"neutral": "resources/icons/neutral.svg"
			}
		}
	]
},

or something? Just an example, these are dummy entries. This can maintain support for different images needed different selector structures, because internally the array entries under "images" would each be converted to their own ResourceLoaderImageModule and the styles from that would just be added. This should also eventually work with ResourceLoaderOOUIIconPackModule too.

If this approach would be okay, I can try to spend some time working on it with MinervaNeue since there are no core modules that use ResourceLoaderImageModule.

We could also introduce a new module type, ResourceLoaderFileAndImageModule (name tbd) that just delegates to ResourceLoaderFileModule and ResourceLoaderImageModule, instead of needing to have ResourceLoaderFileModule handle images too. The new module would likewise internally just have an array of image modules to include.

Extra notes:

  • update ResourceLoaderContext to try to retrieve images from file module too
  • inherit local base path from file module if not specified

I don't think there is any reason why ResourceLoaderImageModule could not extend ResourceLoaderFileModule. I don't think we should need to change the format in skin.json.

Merging the definitions of skins.minerva.mainMenu.styles and skins.minerva.mainMenu.styles would be a good experiment to start with (ResourceLoaderOOUIIconPackModule extends ResourceLoaderImageModule).

I don't think there is any reason why ResourceLoaderImageModule could not extend ResourceLoaderFileModule. I don't think we should need to change the format in skin.json.

Merging the definitions of skins.minerva.mainMenu.styles and skins.minerva.mainMenu.styles would be a good experiment to start with (ResourceLoaderOOUIIconPackModule extends ResourceLoaderImageModule).

I proposed the structure above so that you could define images for multiple selector types in the same module, rather than needing different modules when the names are different patterns. Eg to allow merging skins.minerva.icons.page.issues.uncolored and skins.minerva.icons.page.issues.default.color because they are only used together, but are separate because they have different selectors.

Oh I see. Yes, I think eventually we'll want to find a way to consolidate icons with different patterns, but this bug was simply for the cases where Icon/Image modules could be merged with file modules. That in itself would be a huge win, I reckon we could shave away quite a few modules with that alone. I think the different patterns is more an artifact of the lack of standardization on the icon front which will hopefully be addressed in future by the move to Vue.js

Oh I see. Yes, I think eventually we'll want to find a way to consolidate icons with different patterns, but this bug was simply for the cases where Icon/Image modules could be merged with file modules. That in itself would be a huge win, I reckon we could shave away quite a few modules with that alone. I think the different patterns is more an artifact of the lack of standardization on the icon front which will hopefully be addressed in future by the move to Vue.js

So you would prefer that I *not* implement the proposal above of allowing multiple sub-modules for images with different formats, but just the same current input structure but combined for images and files?

So you would prefer that

I have no preference but this seems like it might be a separate but equally valid task. @Krinkle and @Catrope who maintain ResourceLoader would be best to provide input on that subject as I assume they'd be doing the code review here.

This ticket is just about making ResourceLoaderImageModule extend ResourceLoaderFileModule, which is more pressing to me and my team. However, the two problems might be related.