Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chrome's throbber_small.svg was mis-optimized by svgo #1672

Closed
randomascii opened this issue Apr 29, 2022 · 7 comments
Closed

Chrome's throbber_small.svg was mis-optimized by svgo #1672

randomascii opened this issue Apr 29, 2022 · 7 comments
Labels

Comments

@randomascii
Copy link

Describe the bug
When optimizing throbber_small.svg with this tool a few important directives were removed which changed the behavior. An example removal was to{transform:rotate(360deg)}. The diffs can be seen here:
https://chromium-review.googlesource.com/c/chromium/src/+/3591043/2/ui/webui/resources/images/throbber_small.svg#b1

To Reproduce
Steps to reproduce the behavior:

  1. Copy the contents of this .svg file: https://source.chromium.org/chromium/chromium/src/+/main:ui/webui/resources/images/throbber_small.svg;l=1;bpv=0
  2. Optimize it
  3. Notice that instead of going continuously clockwise it changes direction
  4. See crbug.com/1318796 for more details

Expected behavior
The behavior of the .svg should have been retained

Desktop (please complete the following information):

  • SVGO Version Unknown
  • NodeJs Version Unknown
  • OS: Windows

Additional context
This all happens in the context of a Chromium repo.

@randomascii
Copy link
Author

Update: we're running v1.2.0 which is quite old so we will try updating it to see if the bug is fixed.

@freshp86
Copy link

Update: we're running v1.2.0 which is quite old so we will try updating it to see if the bug is fixed.

I just checked with the latest version and the problem still happens. Have not tried selectively disabling any of the optimizations done by svgo by default though.

@freshp86
Copy link

freshp86 commented Apr 29, 2022

The bug seems to happen because svgo is removing the to part of each transform. For example the following style tag

<style>
  @keyframes rotate {
    0% {transform:rotate(0deg)}
    to {transform:rotate(360deg)}
  }
  @keyframes fillunfill {
    0% {stroke-dashoffset:58.8}
    50% {stroke-dashoffset:0}
    to {stroke-dashoffset:-58.4}
  }
  @keyframes rot {
    0% {transform:rotate(0deg)}
    to {transform:rotate(-360deg)}
  }
</style>

becomes

<style>
  @keyframes rotate {
    0% {transform:rotate(0deg)}
  }
  @keyframes fillunfill {
    0% {stroke-dashoffset:58.8}
    50% {stroke-dashoffset:0}
  }
  @keyframes rot {
    0% {transform:rotate(0deg)}
  }
</style>

where all the to lines have been removed. Same happens if instead of to the code uses 100%. After a quick inspection it this sounds similar to #888, and the issue is fixed by disabling the minifyStyles plugin in the config file.

@randomascii
Copy link
Author

I'm no SVG expert but it seems odd for such significant changes to be made unless minifyStyles is disabled. Is it worth making the defaults safer?

@freshp86
Copy link

I think this is a bug with the minifyStyles plugin, and disabling it just bypasses the issue, as I don't see how removing the last part of a keyframe (whether it is using to or 100%) can possibly result in the same visual behavior.

aarongable pushed a commit to chromium/chromium that referenced this issue May 3, 2022
Produced by running the following command
./tools/resources/svgo.py `find ui/webui/resources/ -name '*.svg'`

Also disabling the 'minifyStyles' optimization since it is now known to
cause incorrect modifications to some files, for example
svg/svgo#1672.

Bug: 1309977
Change-Id: Ia6af9ea460f964117130fd79e2cd592db45c62cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3622435
Auto-Submit: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998682}
@XhmikosR
Copy link
Contributor

For reference, this is the same as #1631

@SethFalco
Copy link
Member

This was fixed in v3.0.0. I've double-checked the file shared in the issue, and can confirm it no longer breaks when processed by SVGO. 🎉

Closing as resolved.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 2, 2023
Specifically updating:
 - svgo: 2.8.0  -> 3.0.2
   Includes a fix for [1] which was affecting some SVG files, and has
   resulted in using 'minifyStyles: false' in
   tools/resources/svgo.config.js as a workaround, see [2].
 - terser: 5.16.2 -> 5.20.0
 - rollup: 3.12.0 -> 3.29.4
 - eslint: 8.33.0 -> 8.50.0

With the latest version of Rollup, bundled files are unfortunately a
bit larger, due to fixing a previous bug [3], which incorrectly only
kept the first line of all license header comments. The new behavior
is correct, so perhaps we should look into ways of deduplicating license
header comments in the future, to further reduce the file size.

[1] svg/svgo#1672
[2] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
[3] rollup/rollup#4953

Bug: None
Change-Id: I08fc9a6ad0cf37e75e0b5e3c4dcc61f4ad555838
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903168
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204162}
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 2, 2023
…ns."

This reverts commit aca5179.

Reason for revert: 

May cause failures in https://ci.chromium.org/ui/p/chromium/builders/ci/linux-presubmit/8328/overview

Original change's description:
> WebUI: Update several toolchain dependencies to latest versions.
>
> Specifically updating:
>  - svgo: 2.8.0  -> 3.0.2
>    Includes a fix for [1] which was affecting some SVG files, and has
>    resulted in using 'minifyStyles: false' in
>    tools/resources/svgo.config.js as a workaround, see [2].
>  - terser: 5.16.2 -> 5.20.0
>  - rollup: 3.12.0 -> 3.29.4
>  - eslint: 8.33.0 -> 8.50.0
>
> With the latest version of Rollup, bundled files are unfortunately a
> bit larger, due to fixing a previous bug [3], which incorrectly only
> kept the first line of all license header comments. The new behavior
> is correct, so perhaps we should look into ways of deduplicating license
> header comments in the future, to further reduce the file size.
>
> [1] svg/svgo#1672
> [2] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
> [3] rollup/rollup#4953
>
> Bug: None
> Change-Id: I08fc9a6ad0cf37e75e0b5e3c4dcc61f4ad555838
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903168
> Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
> Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1204162}

Bug: None
Change-Id: I168e5d1a7f15b339dda21299698af046d30f5adf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908630
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Auto-Submit: Jian Li <jianli@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204278}
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 3, 2023
…ons.

Reland notes: Fixed presubmit test failures in
 - tools/resources/svgo_presubmit_test.py
 - ui/webui/resources/tools/bundle_js_test.py

Specifically updating:
 - svgo:   2.8.0  -> 3.0.2
 - terser: 5.16.2 -> 5.20.0
 - rollup: 3.12.0 -> 3.29.4
 - eslint: 8.33.0 -> 8.50.0

rollup notes:
With the latest version of Rollup, bundled files are unfortunately a
bit larger, due to fixing a previous bug [1], which incorrectly only
kept the first line of all license header comments. The new behavior
is correct (see updated tests), so perhaps we should look into ways of
deduplicating license header comments in the future, to further reduce
the file size.

svgo notes:
 - Latest version includes a fix for [2] which was affecting some SVG
   files, and has resulted in using 'minifyStyles: false' in
   tools/resources/svgo.config.js as a workaround, see [3].
 - Updated svgo.config.js to account for the breaking change at [4].
 - Updated svgo_presubmit_test.py since it now re-arranges
   the order of SVG attributes as part of the optimization.

[1] rollup/rollup#4953
[2] svg/svgo#1672
[3] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
[4] svg/svgo@6295c60

Bug: None
Change-Id: I12ca6f1204868a8592d03b24852c12c97e4e5b0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908394
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204712}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants