Page MenuHomePhabricator

Tooling: Should not be possible to ship un-optimised SVGs
Open, HighPublic

Description

When deploying the new header we shipped a couple of non-optimised SVGS (see T159979).
To protect against this we should get Jenkins to check if svgo can optimise the SVGs and if so complain when they have not been optimised.
We do something similar to ensure compiled assets are checked in inside Popups.

Thoughts?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 5 2017, 10:48 PM

I've run into the same problem on various occassions and products. That seems reasonable. One problem remaining is a threshold.
We've had a similar discussion on OOUI and it turned out that the SVGO optimization (chosen back then) caused visual regressions on some icons.

Yeah, unfortunately svgo isn't perfect so can't run in unsupervised mode, and I'd not be shocked if svgo would find an icon "compressible" when doing so would lead to rendering errors; having to force-V+2 the repo forever wouldn't be a good outcome. :-) Complain is not the same as vote-against, though we don't really have non-error dev/merge CI that people listen to right now…

You can maintain a whitelist of svgs that shouldn't be compressed. In MobileFrontend with the --pretty parameter we have not run into any issues.

pretty just makes the output formatted (pretty-printed, inserting some EOLs and indentation).

I've found from my very low threshold of acceptable visual impact (=no impact preferred) when optimizing images, that those SVGO options might cause trouble and should be disabled:

  • Round/rewrite number lists (can cause unprecise rendering)
  • Prefer viewbox to width/height (troubles in some browsers)
  • Remove raster images (generally not a good idea)
  • Remove title (not good for accessibility reasons)
  • Remove viewbox (troubles with some other browsers)
  • Remove XML instructions (disables viewing as standalone file in browsers)

My goto tool to try the different options out is SVGOMG by Jake Archibald.

bmansurov added a subscriber: bmansurov.

Can we have some guidance from the Front-end standards group on this?

@bmansurov Put it on the agenda for this Wednesday's meeting. What are the specifics that you want to have as outcome and do you want to join the meeting?

@Volker_E done. Yes, I'd like to participate. I'm mainly interested in who'll own this work and how our team can support the owner.

Volker_E triaged this task as High priority.Apr 12 2017, 5:53 PM

Yeah, unfortunately svgo isn't perfect so can't run in unsupervised mode, and I'd not be shocked if svgo would find an icon "compressible" when doing so would lead to rendering errors; having to force-V+2 the repo forever wouldn't be a good outcome. :-)

We used to have various issues with that (I could dig up the relevant tasks/commits), but that was ~2 years ago. I'm not following SVGO development, but it seems to be actively developed and it seems to have caught more traction since then. These breaking bugs might very well have been fixed since then.

Jdlrobson added a project: Epic.

We talked about this in the frontend standards group and the general advice is to add a grunt tool that runs svgo and complains if the diff is different (similar to how Page Previews checks compiled assets have been checked in). Any project can do this now without any need for a change.

We could update Extension:BoilerPlate or Manual:Assets to be the canonical place for new setups.
I've created T162827 for the MobileFrontend use case.

We do this in MobileFrontend now. The tooling is generic and can be used by other extensions if they need to.

On further tests, above recommendations for SVGO have been proven mostly fail-safe so far, with one addition, disabling 'cleanupIDs' is necessary on more complex SVGs, in our case exported SVGs from Sketch.
Also, I've added https://gerrit.wikimedia.org/r/#/c/395931/ for adding SVGO to OOUI's dist SVG files.
And started SVG coding conventions capturing the learnings from last couple of months.