Page MenuHomePhabricator

Colorize SVG task doesn't work any more due to <title> tags
Closed, ResolvedPublic


When defining something like

"variants": {
		"invert": {
			"color": "#000",
			"global": true
		"progressive": {
			"color": "#c63",
			"global": true
		"destructive": {
			"color": "#33d"
		"warning": {
			"color": "#005dff"

this is the output

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="" width="20" height="20" viewBox="0 0 20 20"><g fill="#33d">
	<path fill="#fff" d="M18.72 11L9 1.28A1 1 0 0 0 8.35 1H2a1 1 0 0 0-1 1v6.35a1 1 0 0 0 .28.65L11 18.72a1 1 0 0 0 1.37 0l6.38-6.38a1 1 0 0 0-.03-1.34zM5 7a2 2 0 1 1 2-2 2 2 0 0 1-2 2z"/>

Event Timeline

Volker_E created this task.Jan 11 2019, 1:37 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 11 2019, 1:37 AM

Change 483660 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[oojs/ui@master] build: Fix colorize SVG regression on title elements

Change 483660 merged by jenkins-bot:
[oojs/ui@master] build: Fix colorize SVG regression on title elements

I can't reproduce the problem. As far as I can tell, SVG colorizing works equally well before and after this patch. The presence of the <title> element doesn't break it.

If something is actually broken before this patch and fixed after, then the same fix must be applied in MediaWiki in ResourceLoaderImage::variantize(). It will be trickier because that code actually uses "proper DOM manipulation" and not "regexp magic", which y'all scoffed at in comments on Gerrit ;)

Might be a Chrome specific issue. Don't have the time to deep-dive into it right now. Definitely the fill <g> wrapping <title> and all paths seemed to have thrown it off, even though the spec says differently (example of a g wrapping a title

While I still don't see any rendering problems with wrapping the <title> inside the <g>, on a closer look, it seems that doing so is semantically incorrect: it changes the meaning of the title to apply to just the group, rather than the entire file. In practice, this means that when the SVG file is opened separately in a browser (this doesn't really apply to how we use them), the browser's tab/window title will not show the title we specify. The spec you linked also says this:

Authors should always provide a ‘title’ child element to the outermost svg element within a stand-alone SVG document. The ‘title’ child element to an ‘svg’ element serves the purposes of identifying the content of the given SVG document fragment.

Change 484561 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] ResourceLoaderImage: Avoid wrapping toplevel <title> in <g>

@Volker_E What is the <title> element for? Is this something we could strip from the source with e.g. SVGO, or is it required for something?

Restricted Application added a project: Performance-Team. · View Herald TranscriptJan 15 2019, 10:50 PM

@Krinkle title can be used for [[ User agents may, however, for example, display the ‘title’ element as a tooltip, as the pointing device moves over particular elements. Alternate presentations are possible, both visual and aural, which display the ‘desc’ and ‘title’ elements but do not display ‘path’ elements or other graphics elements. | tooltips or alternative presentation like in a screen reader ]].

In OOUI we've added toplevel title attributes for ensuring accessibility for the case where OOUI icons are used/uploaded to places outside of direct OOUI usage like Wikimedia Commons or article usages.
When loading through RL as background-image we could pretty safely assume that the title is safe to be stripped from sending to the client. That should be part of T36812 or T61086 – not clear where the exact differences are between these two tasks.

Change 484561 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Avoid toplevel <title> under <g> in ResourceLoaderImage

Volker_E triaged this task as Normal priority.Jan 16 2019, 2:19 AM
Volker_E moved this task from Backlog to Reviewing on the OOUI board.
Volker_E closed this task as Resolved.Jan 16 2019, 9:46 PM
Volker_E assigned this task to matmarex.
Volker_E moved this task from Reviewing to OOUI-0.30.2 on the OOUI board.
Volker_E edited projects, added OOUI (OOUI-0.30.2); removed OOUI.
Volker_E removed a project: Patch-For-Review.
Volker_E removed a subscriber: gerritbot.

Change 486131 had a related patch set uploaded (by Jforrester; owner: VolkerE):
[mediawiki/core@master] Update OOUI to v0.30.2

Change 486131 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.30.2