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

Assigned To

Authored By

	Volker_E
	Jan 11 2019, 1:37 AM

Project Tags

Referenced Files

None

Subscribers

Description

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="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 20 20"><g fill="#33d">
	<title>
		tag
	</title>
	<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"/>
</g></svg>

Details

Related Changes in Gerrit:

Subject	Author	Repo	Branch	Lines +/-
Update OOUI to v0.30.2	VolkerE	mediawiki/core	master	+677 -544
resourceloader: Avoid toplevel `<title>` under `<g>` in ResourceLoaderImage	Bartosz Dziewoński	mediawiki/core	master	+15 -2
build: Fix colorize SVG regression on `title` elements	VolkerE	oojs/ui	master	+1 -1

Customize query in gerrit

Related Objects

Mentioned Here: T36812: Implement minification for SVG files in ResourceLoader
T61086: Minify on-the-fly generated SVGs from ResourceLoaderImage

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

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

gerritbot added a project: Patch-For-Review.Jan 11 2019, 1:51 AM

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

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

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 [[ https://www.w3.org/TR/SVG11/struct.html#DescriptionAndTitleElements | 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>

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

@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?

Krinkle added a project: MediaWiki-ResourceLoader.Jan 15 2019, 10:50 PM

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

Krinkle moved this task from Inbox to Confirmed Problem on the MediaWiki-ResourceLoader board.Jan 15 2019, 10:51 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

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.14; 2019-01-22).Jan 16 2019, 1:01 AM

Volker_E triaged this task as Medium 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

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

gerritbot added a project: Patch-For-Review.Jan 23 2019, 7:04 PM

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