Page MenuHomePhabricator

[EPIC] Unify and optimize SVG markup across Foundation products
Closed, ResolvedPublic

Description

SVGs in our products feature all kinds of markup, in parts with editor specific metadata, resulting in unnecessary inconsistency and bloat.
We're using SVGO for optimization and go for manual QA, both on code and visual level.
Optimizations achieved are between 3%–27% file size on average before gzipping, in a few cases, like MMV even up to 78%.

Among changes:

  • Unify XML declaration, preferable (altough maybe slighly esoteric) with uppercase UTF-8: <?xml version="1.0" encoding="UTF-8"?>
    • Removing standalone="no" attribute as its default
  • Removing additional DOCTYPE
  • Setting of width and height attributes together with viewport for widest cross-browser compatibility, example: <svg xmlns="http://www.w3.org/2000/svg" width="20" height="20" viewBox="0 0 20 20">
    • Removing px value unit as it's default
    • Removing non-standard enable-background attribute, removed from SVG standard altogether
  • Reduction of ids where applicable
  • Changing indentation to use tabs & reduce whitespace where applicable, example: "/> not " />
  • Minify and lowercase colors, example: #fff not #FFFFFF
  • Collapsing useless groups g
  • Merging paths where applicable
  • Removing editor (InkScape, Illustrator etc.) data
  • Removing unnecessary style attributes where applicable
  • Removing unused defs
  • Removing raster images where applicable
  • Sorting attributes

Above optimizations are a mix of SVGO's available plugins and manual optimizations. They've been collected now at https://www.mediawiki.org/wiki/Manual:Coding_conventions/SVG


Products optimized:

Themes:

Extensions:

Projects:

Sign off steps

  • Make sure all subtasks are resolved.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 387192 merged by jenkins-bot:
[wikimedia/portals@master] SVG Sprites - Adding new SVG files

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

@Jdlrobson The team inferred that you are doing the code review for the Web team stuff. Is that right?

Change 390557 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/Flow@master] Unify SVG markup

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

Per discussion with Nirzar, I'll review https://gerrit.wikimedia.org/r/389290 tomorrow after the train is cut. This will maximise testing time.

ovasileva triaged this task as Medium priority.Nov 13 2017, 7:14 PM

@ABorbaWMF @Nirzar Following are the MinveraNeue patch icons where more than just simple reordering of attributes was done (additional viewBox attributes for example):

  • magnifying glass,
  • external link icon & its RTL version,
  • userAnonymous
  • watch & watched
  • edit & editLocked
  • error

Change 390557 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Unify SVG markup

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

Change 389290 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Unify SVG markup

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

Over to you @ABorbaWMF to get those icons tested. We'll need to review icons in the mobile site on the beta cluster.

So far things look good. I am not sure if I have the right icons for external link icon & its RTL version. I also don't think we have an edit-locked article on beta right now.







@ovasileva In this task there's are still products that will see patches/need code review. Only the MinervaNeue part was resolved recently. StructuredDiscussions, MMV and the Style Guide are among those TBD.

Insightful read on further optimization, in case we want to go crazier on SVG optimization and gzip/brotli preparation:
https://blog.usejournal.com/of-svg-minification-and-gzip-21cd26a5d007

Featuring

  • unclosing paths (remove end z) as they're auto-closing or
  • making use of more backreferences (we already go for example for only hex color values

Most of the presented steps generally don't scale as it needs per-file dev supervision. In some cases like the Wikipedia logo it could be useful though.

After more detailed feedback on XML declaration necessity in SVG here, reciting:

GreLI commented on #836:
AFAIK, Firefox 3.6 doesn't support SVG except in <object>, and Android 2.3 doesn't support SVG at all. These are surely “issues”, but I think, it doesn't worth supporting SVG for them—a fallback should be provided in a such case.
I've heard nothing about something couldn't render SVG because of absent prolog. The only case was, that some primitive utils like file couldn't get MIME-type right, discussed in #306.

See also https://www.w3.org/TR/REC-xml/#charencoding

Should we also get rid of the XML declarations globally and would we be on the safe side not running into a weird encoding hell?
Opinions, @Krinkle or @Esanders ?

@Volker_E The main concern I'd have is mime-type checking (e.g. https://github.com/svg/svgo/issues/306).

Many mime-type tools wrongly interpret files starting with <svg as something different.

With <?xml header: image/svg+xml.
Without:

  • PHP 7.1 mime_content_type on macOS: text/html
  • file command on Linux: text/plain
  • file command on macOS: image/svg (promising!)

I know that within our systems, ResourceLoader (CSSMin) and MediaWiki more generally prefer mime-type guessing based on file name in many cases, but not all cases. So I can't confidently say nothing will break when we remove the header.

@Krinkle For the OOUI icons via RL @embed XML processing information should be considered then… Where would be the best place of removal then, probably in CSSMin.php?!

@Krinkle For the OOUI icons via RL @embed XML processing information should be considered then… Where would be the best place of removal then, probably in CSSMin.php?!

[..] within our systems (ResourceLoader, CSSMin) [..] generally prefer mime-type guessing based on file name [..]

This means they'd return image/svg+xml based on the filename ending in .svg, regardless of what is inside the file.

I'm fairly sure that within ResourceLoader nothing would be affected by your proposed change. However, we do use mime_content_type() in many other places in MediaWiki, and even CSSMin does use it as a fallback, after checking a fixed set of common file extensions. I can't say for certain whether or not one of those other places will scan an OOUI file at some point.

Also, at Wikimedia production, serving static files (e.g. debug mode) will touch code beyond ResourceLoader (Varnish, Nginx, Apache, wmf-config static.php). Paths like /static/current are handled by Apache I believe, not MediaWiki PHP. I don't know if that uses content-based mime checking.

Paths like /w/resources/ are nowadays proxied by static.php (in wmf-config), which uses StreamFile::contentTypeFromPath (MediaWiki PHP), and works similar to CSSMin (guess type based on file name).

From a quick glance it should be fine, at least for ResourceLoader and related Wikimedia production infrastructure. But I'm not sure if we want to depend on that.

I agree that it might be best to keep the XML header in the files and strip it only as an optimisation when embedding. For that, CSSMin is indeed the appropiate place. I'd recommend elaborating on the existing encodeStringAsDataURI method, with a case specific to the $type value for SVG files.

Change 399133 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/RelatedArticles@master] Unify SVG markup

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

Change 399136 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/TwoColConflict@master] Unify SVG markup

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

Change 399136 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Unify SVG markup

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

Change 399133 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Unify SVG markup

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

Change 388833 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Unify SVG markup

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

Checked in betalabs (including Saucelabs IE11 Windows combination), checked different skins - as far as I could see, no regressions of any sort are present.

@Volker_E why is this task still open? I'm a little confused about what's outstanding here. Could you update the description - maybe adding an unchecked checkbox?

Jdlrobson renamed this task from Unify and optimize SVG markup across Foundation products to [EPIC] Unify and optimize SVG markup across Foundation products.Mar 3 2020, 7:07 PM
Jdlrobson added a project: Epic.
Jdlrobson updated the task description. (Show Details)

Change 575812 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Run .svg minifier in Vector skin

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

Change 575812 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Re-crush SVG files with unified SVGO rules

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