Page MenuHomePhabricator
Create Task
Maniphest T244558

ResourceLoaderOOUIIconPackModule doesn't allow mixing of images and icons
Open, LowPublic
Actions

Description

When I create a module

		"skins.minerva.mainMenu.icons": {
			"class": "ResourceLoaderOOUIIconPackModule",
			"variants": [],
			"selectorWithoutVariant": ".mw-ui-icon-minerva-{name}:before",
			"defaultColor": "#4A4F53",
			"useDataURI": false,
			"icons": [ "userContributions" 	 ],
			"images": {
				"login": {
					"file": {
						"ltr": "resources/skins.minerva.mainMenu.icons/logIn-ltr.svg",
						"rtl": "resources/skins.minerva.mainMenu.icons/logIn-rtl.svg"
					}
				},
				"home": "resources/skins.minerva.mainMenu.icons/home.svg",
				"logout": {
					"file": {
						"ltr": "resources/skins.minerva.mainMenu.icons/logOut-ltr.svg",
						"rtl": "resources/skins.minerva.mainMenu.icons/logOut-rtl.svg"
					}
				},
				"nearby": "resources/skins.minerva.mainMenu.icons/nearby.svg",
				"random": "resources/skins.minerva.mainMenu.icons/random.svg",
				"settings": "resources/skins.minerva.mainMenu.icons/settings.svg",
				"watchlist": "resources/skins.minerva.mainMenu.icons/watchlist.svg"
			}
		},

I get no complaints that icons and images can be mixed, but the files in images cause the following issue as they are assumed to be in core:

[f17d98ebb5c9164e792b9a01] /w/load.php?modules=skins.minerva.mainMenu.icons&image=settings&format=rasterized&skin=fallback&version=1s0p7 MWException from line 294 of /Users/jrobson/git/core/includes/resourceloader/ResourceLoaderImage.php: File '/Users/jrobson/git/core/resources/skins.minerva.mainMenu.icons/settings.svg' does not exist
Backtrace:

#0 /Users/jrobson/git/core/includes/resourceloader/ResourceLoader.php(1068): ResourceLoaderImage->getImageData(ResourceLoaderContext)
#1 /Users/jrobson/git/core/includes/resourceloader/ResourceLoader.php(791): ResourceLoader->makeModuleResponse(ResourceLoaderContext, array, array)
#2 /Users/jrobson/git/core/load.php(46): ResourceLoader->respond(ResourceLoaderContext)
#3 {main}

In this situation we should either forbid the mixing of the values of update the path for the files (the latter would be very helpful for migrating ResourceLoaderImageModules which have icons that are not yet in OOUI!)

Details

SubjectRepoBranchLines +/-
[ResourceLoader] Allow use of ResourceLoaderOOUIconPackModule with custom selectorsmediawiki/coremaster+58 -15
Customize query in gerrit

Related Objects

Event Timeline

Jdlrobson created this task.Feb 7 2020, 3:06 AM
Restricted Application added a project: Performance-Team. · View Herald TranscriptFeb 7 2020, 3:06 AM
Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript
Ammarpad subscribed.Feb 7 2020, 1:52 PM
• Gilles subscribed.Feb 25 2020, 5:38 PM

Have you worked around the issue? How important is this for you?

Jdlrobson added a comment.Feb 25 2020, 6:45 PM

This is useful for T244444 which helps standardisation efforts as well as reducing our RL module burden.

We can work around it by adding new ResourceLoader modules but that adds caching concerns and bloats the startup module.

Having this capability would speed up the removal of several of these ResourceLoaderImage modules in our product which would lead to a smaller startup script so may be of interest to performance team from that perspective.

• Gilles added a project: Editing-team.Mar 2 2020, 9:12 PM
• Gilles added a subscriber: matmarex.

@matmarex it seems like you built this specialised RL class originally, would you be able to take care of this?

• Gilles moved this task from Inbox, needs triage to Radar on the Performance-Team board.Mar 2 2020, 9:13 PM
• Gilles edited projects, added Performance-Team (Radar); removed Performance-Team.
matmarex added a comment.Mar 3 2020, 12:15 AM
In T244558#5934751, @Gilles wrote:

@matmarex it seems like you built this specialised RL class originally, would you be able to take care of this?

I can't deny that, unfortunately, but I don't think I will have the time to work on this.

FWIW I never intended to allow using images in ResourceLoaderOOUIIconPackModule, it's supposed to only be used to create a "sub-pack" of existing OOUI icons, rather than define new ones. I should have made it throw an exception.

You can instead use ResourceLoaderOOUIImageModule to define new icons.

Alternatively, maybe you can use ResourceLoaderFilePath objects instead of strings for file paths (you'd have to define the module in PHP code rather than in JSON), to avoid the wrong base path being added, but if you do, don't ever ask me to look at that code :)

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.Mar 16 2020, 9:29 PM
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.Apr 6 2020, 7:56 PM
JTannerWMF moved this task from Untriaged to Freezer on the Editing-team board.Apr 7 2020, 3:51 PM
JTannerWMF subscribed.

It looks like @matmarex said he isn't able to work on this task and provided a workaround, so I am moving this to the freezer. Please ping me if this is a problem.

Krinkle triaged this task as Low priority.Apr 7 2020, 6:49 PM
Jdlrobson mentioned this in T282625: Figure out icon system architecture for WVUI.May 12 2021, 3:58 PM
gerritbot added a comment.Feb 1 2022, 6:29 PM

Change 758938 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] [ResourceLoader] Allow use of ResourceLoaderOOUIconPackModule with custom selectors

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

gerritbot added a project: Patch-For-Review.Feb 1 2022, 6:29 PM
Jdlrobson added a comment.Feb 1 2022, 6:53 PM

Thinking about this a bit differently, now that OOUI icon pack is more mature, I think this can be resolved by allowing icons to be associated with arbitrary selectors. This is needed for T261391 given the external link icons use a very specific selector.

gerritbot added a comment.Apr 13 2022, 10:04 PM

Change 758938 abandoned by Jdlrobson:

[mediawiki/core@master] [ResourceLoader] Allow use of ResourceLoaderOOUIconPackModule with custom selectors

Reason:

Too many things going on. This one is not enough of a priority right now for me to wrangle feedback from the right people.

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

Maintenance_bot removed a project: Patch-For-Review.Apr 13 2022, 10:31 PM
alistair3149 subscribed.Dec 7 2022, 7:12 AM
Krinkle removed a project: Performance-Team (Radar).Aug 18 2023, 8:14 PM
Restricted Application added a project: MediaWiki-Platform-Team. · View Herald TranscriptAug 18 2023, 8:14 PM
Krinkle moved this task from Inbox, needs triage to Radar on the MediaWiki-Platform-Team board.Aug 21 2023, 1:08 PM
Krinkle edited projects, added MediaWiki-Platform-Team (Radar); removed MediaWiki-Platform-Team.
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