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

Fixed bug in removeHiddenElems #1925

Merged
merged 4 commits into from
Jan 6, 2024
Merged

Conversation

johnkenny54
Copy link
Contributor

This change removes non-rendering nodes only if they have no referenced children.

This fix is part of the solution to removing the media-flash.svg mismatches in test-regression. On its own, this fix resolves 8 mismatches in test-regression. In conjunction with PR #1923 this fix resolves 14 mismatches in test-regression.

Further Discussion
This plugin has some code that seems like it needs to be removed, but maybe I'm missing something. In addition to the usual return of the visitor functions, this plugin has an explicit call to visit(), with different code than what is in the return value. That causes the tree to be traversed twice, and the <symbol> element was being removed in both sets of code. I removed the code in the explicit call to visit(), but there is still code in the visit() call that removes elements with opacity=0. This seems weird because:

  • traversing the tree twice seems inefficient
  • removing the elements based on what is in a <style> element seems contradictory to what happens in the visitor function that is returned by the plugin (where optimization is disabled in the presence of a <style> element)

So the code I submitted fixes the referenced issue, but it seems like this plugin should also be changed by:

  • removing the explicit call to visit()
  • changing the expected behavior of removeHiddenElems.02.svg so that the source file is not changed

Is there a good reason for the code to have the explicit call to visit()?

Closes #1924

@johnkenny54
Copy link
Contributor Author

It looks like this change introduced 2 mismatches in the W3C test regressions, which I hadn't noticed locally since the overall number of regressions (including the oxygen SVGs) was reduced. I will look into this.

@johnkenny54
Copy link
Contributor Author

Regression tests fixed. Reverted some of the changes, including some of the logic in the explicit call to visit() which were necessary to accumulate list of nonRenderedNodes. Added a test case to catch the problem that was causing regression failure.

With these changes, this fix on its own would not have reduced the number of regression mismatches, but now that PR #1923 has been merged, this fix reduces mismatches by 6.

It looks to me like all the logic in the explicit call to visit could be moved to returned function - this would eliminate the extra tree traversal, and also make the plugin easier to follow. But unless it's causing problems as is, that should probably be a separate PR.

Copy link
Member

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for identifying the issue and submitting a patch.

@SethFalco SethFalco merged commit 4da8b38 into svg:main Jan 6, 2024
8 checks passed
@SethFalco
Copy link
Member

I rebased locally to change .svg to .svg.txt and didn't push before merging… 🤦🏽
I'll address that in a separate PR.

@johnkenny54 johnkenny54 deleted the removeHiddenElems branch January 7, 2024 17:50
nono referenced this pull request in cozy/cozy-stack May 13, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [svgo](https://svgo.dev) ([source](https://togithub.com/svg/svgo)) |
[`3.2.0` -> `3.3.2`](https://renovatebot.com/diffs/npm/svgo/3.2.0/3.3.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/svgo/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/svgo/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/svgo/3.2.0/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/svgo/3.2.0/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>svg/svgo (svgo)</summary>

### [`v3.3.2`](https://togithub.com/svg/svgo/releases/tag/v3.3.2)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.3.1...v3.3.2)

##### Notice

An update on what happened with v3.3.0 and v3.3.1. While we have
retained CJS support, the migration to EMS has changed the acceptable
ways to import SVGO, in ways that users depended on before. This
effectively made SVGO v3 a breaking change.

Rather than resolve or workaround these differences, we've opted to
release SVGO v3.3.2, which is effectively a revert to v3.2.0, and
deprecate versions v3.3.0 and v3.3.1. We'll then proceed to work on
releasing v4 which will document the breaking changes, and feature
further breaking changes that were slated for v4, like disabling
`removeViewBox` by default.

Before the v4.0.0 release, I'll put more focus on testing and use
release candidates, just to help make the release go smoothly! 👍🏽

Sorry for the headache, and thanks for your patience.

### [`v3.3.1`](https://togithub.com/svg/svgo/releases/tag/v3.3.1)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.3.0...v3.3.1)

##### Notice

SVGO v3.3.0, which was meant to migrate to ESM without breaking CJS
support, unfortunately broke CJS projects. There was a mistake with
exports, so the `loadConfig` function wasn't available in the CJS bundle
and lead to issues for many users.

Thanks to everyone who raised the issue, and to
[@&#8203;nuintun](https://togithub.com/nuintun) who submitted a pull
request to resolve it so quickly.

I apologize for letting that breaking change through, and will aim to do
better. Namely, by adding more tests to cover our exports, and any other
public interface in general for each distribution of SVGO, so this
doesn't happen again.

SVGO v3.3.1 should resolve the issue for CJS projects, but if you
encounter anything else, do let us know by opening an issue on GitHub.

### [`v3.3.0`](https://togithub.com/svg/svgo/releases/tag/v3.3.0)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.2.0...v3.3.0)

##### Deprecated

This release introduced breaking changes, which have been reverted in
v3.3.2. The bug fixes will be reintroduced in v4.0.0.

##### What's Changed

##### ESM

SVGO is now a dual package, serving for both Common JS and ESM usage. We
believe there shouldn't be any problems, especially as SVGO as largely
stateless, but feel free to open an issue if you encounter problems with
this.

To be explicit, this is not a breaking change, and SVGO should continue
to work in Common JS projects!

Thanks to [@&#8203;jdufresne](https://togithub.com/jdufresne) for doing
the bulk of the work.

##### Default Behavior

- [`convertColors`](), now converts all references to colors excluding
references to IDs to lowercase. This can be disabled by setting
`convertCase` to `false`.

##### Bug Fixes

- [`cleanupIds`](), treat both URI encoded and non-URI encoded IDs as
the same. By [@&#8203;liuweifeng](https://togithub.com/liuweifeng) in
[#&#8203;1982](https://togithub.com/svg/svgo/issues/1982)
- [`collapseGroups`](), check styles as well as attributes. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1952](https://togithub.com/svg/svgo/issues/1952)
- [`collapseGroups`](), move attributes atomically. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1930](https://togithub.com/svg/svgo/pull/1930)
- [`convertPathData`](), fix q control point when item is removed. By
[@&#8203;KTibow](https://togithub.com/KTibow) in
[https://github.com/svg/svgo/pull/1927](https://togithub.com/svg/svgo/pull/1927)
- [`convertPathData`](), preserve vertex for markers only paths. By
[@&#8203;SethFalco](https://togithub.com/SethFalco) in
[#&#8203;1967](https://togithub.com/svg/svgo/issues/1967)
- [`mergePaths`](), don't merge paths if attributes/styles depend depend
on the nodes bounding box. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1964](https://togithub.com/svg/svgo/issues/1964)
- [`moveElemsAttrsToGroups`](), no longer moves the transforms if group
has the `filter` attribute. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1933](https://togithub.com/svg/svgo/issues/1933)
- [`prefixIds`](), fixed issue where some IDs were not prefixed when
style tag contained XML comments. By
[@&#8203;john-neptune](https://togithub.com/john-neptune) in
[#&#8203;1942](https://togithub.com/svg/svgo/issues/1942)
- [`removeHiddenElems`](), don't remove node if child element has a
referenced ID. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1925](https://togithub.com/svg/svgo/pull/1925)
- [`removeHiddenElems`](), treat `path[opacity=0]` as a non-rendering
node. By [@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1948](https://togithub.com/svg/svgo/issues/1948)
-
[`removeUselessDefs`](https://svgo.dev/docs/plugins/remove-useless-defs/),
don't remove node if child element has an ID. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1923](https://togithub.com/svg/svgo/pull/1923)
- When stringifying path data, include a space before numbers
represented in scientific notation. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1961](https://togithub.com/svg/svgo/issues/1961)
- No longer crashes when the output (`-o` argument) ends with a trailing
slash to a location that didn't exist. By
[@&#8203;SethFalco](https://togithub.com/SethFalco) in
[#&#8203;1954](https://togithub.com/svg/svgo/issues/1954)

##### SVG Optimization

- [`convertColors`](), introduce parameter to convert colors to common
casing (lowercase/uppercase). By
[@&#8203;JayLeininger](https://togithub.com/JayLeininger) in
[https://github.com/svg/svgo/pull/1692](https://togithub.com/svg/svgo/pull/1692)
- [`removeDeprecatedAttrs`](), new plugin that is disabled by default to
remove SVG attributes that are deprecated. By
[@&#8203;jdufresne](https://togithub.com/jdufresne) in
[#&#8203;1869](https://togithub.com/svg/svgo/issues/1869)

##### Metrics

Before and after using vectors from various sources, with the default
preset of each respective version:

| SVG | Original | v3.2.0 | v3.3.0 | Delta |
|---|---|---|---|---|
| [Arch Linux Logo](https://archlinux.org/art/) | 9.529 KiB | 4.115 KiB
| 4.097 KiB | ⬇️ 0.018 KiB |
|
[Blobs](https://gitlab.gnome.org/GNOME/gnome-backgrounds/-/blob/main/backgrounds/blobs-d.svg)
| 50.45 KiB | 42.623 KiB | 42.609 KiB | ⬇️ 0.014 KiB |
| [Isometric
Madness](https://inkscape.org/~Denis_Kuznetsky/%E2%98%85isometric-madness)
| 869.034 KiB | 540.582 KiB | 540.073 KiB | ⬇️ 0.509 KiB |
| [tldr-pages
Banner](https://togithub.com/tldr-pages/tldr/blob/main/images/banner.svg)
| 2.071 KiB | 1.07 KiB | 1.07 KiB | |
| [Wikipedia
Logo](https://en.wikipedia.org/wiki/File:Wikipedia-logo-v2.svg) |
161.551 KiB | 111.668 KiB | 111.668 KiB | |

Before and after of the browser bundle of each respective version:

| | v3.2.0 | v3.3.0 | Delta |
|---|---|---|---|
| svgo.browser.js | 910.9 kB | 753.0 kB | ⬇️ 157.9 kB |

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone
Europe/Paris, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/cozy/cozy-stack).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNTEuMiIsInVwZGF0ZWRJblZlciI6IjM3LjM1MS4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIiwibGFiZWxzIjpbXX0=-->
yangwooseong referenced this pull request in channel-io/bezier-react Jun 18, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [svgo](https://svgo.dev) ([source](https://togithub.com/svg/svgo)) |
[`3.2.0` -> `3.3.2`](https://renovatebot.com/diffs/npm/svgo/3.2.0/3.3.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/svgo/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/svgo/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/svgo/3.2.0/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/svgo/3.2.0/3.3.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>svg/svgo (svgo)</summary>

### [`v3.3.2`](https://togithub.com/svg/svgo/releases/tag/v3.3.2)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.3.1...v3.3.2)

#### Notice

An update on what happened with v3.3.0 and v3.3.1. While we have
retained CJS support, the migration to EMS has changed the acceptable
ways to import SVGO, in ways that users depended on before. This
effectively made SVGO v3 a breaking change.

Rather than resolve or workaround these differences, we've opted to
release SVGO v3.3.2, which is effectively a revert to v3.2.0, and
deprecate versions v3.3.0 and v3.3.1. We'll then proceed to work on
releasing v4 which will document the breaking changes, and feature
further breaking changes that were slated for v4, like disabling
`removeViewBox` by default.

Before the v4.0.0 release, I'll put more focus on testing and use
release candidates, just to help make the release go smoothly! 👍🏽

Sorry for the headache, and thanks for your patience.

### [`v3.3.1`](https://togithub.com/svg/svgo/releases/tag/v3.3.1)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.3.0...v3.3.1)

#### Notice

SVGO v3.3.0, which was meant to migrate to ESM without breaking CJS
support, unfortunately broke CJS projects. There was a mistake with
exports, so the `loadConfig` function wasn't available in the CJS bundle
and lead to issues for many users.

Thanks to everyone who raised the issue, and to
[@&#8203;nuintun](https://togithub.com/nuintun) who submitted a pull
request to resolve it so quickly.

I apologize for letting that breaking change through, and will aim to do
better. Namely, by adding more tests to cover our exports, and any other
public interface in general for each distribution of SVGO, so this
doesn't happen again.

SVGO v3.3.1 should resolve the issue for CJS projects, but if you
encounter anything else, do let us know by opening an issue on GitHub.

### [`v3.3.0`](https://togithub.com/svg/svgo/releases/tag/v3.3.0)

[Compare Source](https://togithub.com/svg/svgo/compare/v3.2.0...v3.3.0)

#### Deprecated

This release introduced breaking changes, which have been reverted in
v3.3.2. The bug fixes will be reintroduced in v4.0.0.

#### What's Changed

##### ESM

SVGO is now a dual package, serving for both Common JS and ESM usage. We
believe there shouldn't be any problems, especially as SVGO as largely
stateless, but feel free to open an issue if you encounter problems with
this.

To be explicit, this is not a breaking change, and SVGO should continue
to work in Common JS projects!

Thanks to [@&#8203;jdufresne](https://togithub.com/jdufresne) for doing
the bulk of the work.

##### Default Behavior

- [`convertColors`](), now converts all references to colors excluding
references to IDs to lowercase. This can be disabled by setting
`convertCase` to `false`.

##### Bug Fixes

- [`cleanupIds`](), treat both URI encoded and non-URI encoded IDs as
the same. By [@&#8203;liuweifeng](https://togithub.com/liuweifeng) in
[#&#8203;1982](https://togithub.com/svg/svgo/issues/1982)
- [`collapseGroups`](), check styles as well as attributes. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1952](https://togithub.com/svg/svgo/issues/1952)
- [`collapseGroups`](), move attributes atomically. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1930](https://togithub.com/svg/svgo/pull/1930)
- [`convertPathData`](), fix q control point when item is removed. By
[@&#8203;KTibow](https://togithub.com/KTibow) in
[https://github.com/svg/svgo/pull/1927](https://togithub.com/svg/svgo/pull/1927)
- [`convertPathData`](), preserve vertex for markers only paths. By
[@&#8203;SethFalco](https://togithub.com/SethFalco) in
[#&#8203;1967](https://togithub.com/svg/svgo/issues/1967)
- [`mergePaths`](), don't merge paths if attributes/styles depend depend
on the nodes bounding box. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1964](https://togithub.com/svg/svgo/issues/1964)
- [`moveElemsAttrsToGroups`](), no longer moves the transforms if group
has the `filter` attribute. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1933](https://togithub.com/svg/svgo/issues/1933)
- [`prefixIds`](), fixed issue where some IDs were not prefixed when
style tag contained XML comments. By
[@&#8203;john-neptune](https://togithub.com/john-neptune) in
[#&#8203;1942](https://togithub.com/svg/svgo/issues/1942)
- [`removeHiddenElems`](), don't remove node if child element has a
referenced ID. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1925](https://togithub.com/svg/svgo/pull/1925)
- [`removeHiddenElems`](), treat `path[opacity=0]` as a non-rendering
node. By [@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1948](https://togithub.com/svg/svgo/issues/1948)
-
[`removeUselessDefs`](https://svgo.dev/docs/plugins/remove-useless-defs/),
don't remove node if child element has an ID. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[https://github.com/svg/svgo/pull/1923](https://togithub.com/svg/svgo/pull/1923)
- When stringifying path data, include a space before numbers
represented in scientific notation. By
[@&#8203;johnkenny54](https://togithub.com/johnkenny54) in
[#&#8203;1961](https://togithub.com/svg/svgo/issues/1961)
- No longer crashes when the output (`-o` argument) ends with a trailing
slash to a location that didn't exist. By
[@&#8203;SethFalco](https://togithub.com/SethFalco) in
[#&#8203;1954](https://togithub.com/svg/svgo/issues/1954)

##### SVG Optimization

- [`convertColors`](), introduce parameter to convert colors to common
casing (lowercase/uppercase). By
[@&#8203;JayLeininger](https://togithub.com/JayLeininger) in
[https://github.com/svg/svgo/pull/1692](https://togithub.com/svg/svgo/pull/1692)
- [`removeDeprecatedAttrs`](), new plugin that is disabled by default to
remove SVG attributes that are deprecated. By
[@&#8203;jdufresne](https://togithub.com/jdufresne) in
[#&#8203;1869](https://togithub.com/svg/svgo/issues/1869)

#### Metrics

Before and after using vectors from various sources, with the default
preset of each respective version:

| SVG | Original | v3.2.0 | v3.3.0 | Delta |
|---|---|---|---|---|
| [Arch Linux Logo](https://archlinux.org/art/) | 9.529 KiB | 4.115 KiB
| 4.097 KiB | ⬇️ 0.018 KiB |
|
[Blobs](https://gitlab.gnome.org/GNOME/gnome-backgrounds/-/blob/main/backgrounds/blobs-d.svg)
| 50.45 KiB | 42.623 KiB | 42.609 KiB | ⬇️ 0.014 KiB |
| [Isometric
Madness](https://inkscape.org/~Denis_Kuznetsky/%E2%98%85isometric-madness)
| 869.034 KiB | 540.582 KiB | 540.073 KiB | ⬇️ 0.509 KiB |
| [tldr-pages
Banner](https://togithub.com/tldr-pages/tldr/blob/main/images/banner.svg)
| 2.071 KiB | 1.07 KiB | 1.07 KiB | |
| [Wikipedia
Logo](https://en.wikipedia.org/wiki/File:Wikipedia-logo-v2.svg) |
161.551 KiB | 111.668 KiB | 111.668 KiB | |

Before and after of the browser bundle of each respective version:

| | v3.2.0 | v3.3.0 | Delta |
|---|---|---|---|
| svgo.browser.js | 910.9 kB | 753.0 kB | ⬇️ 157.9 kB |

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on Monday after 10am before 7pm" in
timezone Asia/Seoul, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/channel-io/bezier-react).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJjaG9yZTpkZXBzIl19-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

removeHiddenElems removes <symbol> which contains children with IDs
2 participants