fix(removeHiddenElems): handle defs better by KTibow · Pull Request #1879 · svg/svgo

KTibow · 2023-12-09T17:28:01Z

Relocate logic to run in node.exit instead of checking in svg's exit:

remove broken uses
Changed from checking each use to making a list of uses, to fix Regression in 3.0.4: incorrectly removes reusable elements, but leaves references to it #1851
remove unused defs
Fixed a typo so it actually removes them, and add some safeguards
remove empty defs
Changed from checking each def to making a list of defs

KTibow · 2023-12-09T17:51:45Z

Okay so turns out that the unused definitions logic was just always broken, so I'm having to fix the existing implementation

SethFalco

Hey! Thanks for submitting a PR.

I've left a few comments, but the proposed fix makes sense. There are just a few issues with the implementation.

Can you please also add a test that reproduces the issue you're fixing.

KTibow · 2023-12-09T20:48:12Z

Now I'm running into the problem where it has a different behavior in multipass than singlepass, so it fails no matter what.
Context: in this

<svg xmlns="http://www.w3.org/2000/svg">
    <defs>
        <linearGradient id="a">
        </linearGradient>
    </defs>
</svg>

singlepass converts it to this, because the empty defs remover runs before the final pass

<svg xmlns="http://www.w3.org/2000/svg">
    <defs />
</svg>

but multipass converts it to this

<svg xmlns="http://www.w3.org/2000/svg" />

What's the most reasonable solution to this? Is there an elegant way to run the empty defs remover after the final pass?

SethFalco · 2023-12-09T20:52:49Z

Is there an elegant way to run the empty defs remover after the final pass?

Perhaps the best approach would be to take from here:

svgo/plugins/convertOneStopGradients.js

Line 36 in 3dc2f6f

const effectedDefs = new Set();

So whenever we remove a node from a <defs>, store the effected <defs> node in a Set.
Then in root you can iterate these and check if they're empty there.

(And drop the behavior from element#exit.)

You can put the logic to store a Set of <defs> here:

https://github.com/svg/svgo/pull/1879/files#diff-49d33e65cfa6a5a38ff7d8248656645a92ea72527bd1ae3841555bd5cc69d823R92

KTibow · 2023-12-09T20:59:47Z

While there's more to address I'm off to lunch, I'll fix this soon

SethFalco

This looks good now! And thanks for resolving the other issues you found.
Just left a few very petty notes, but happy to merge overall.

Co-authored-by: Seth Falco <seth@falco.fun>

SethFalco · 2023-12-09T22:25:24Z

Great! Thanks for resolving the issue, and improving the plugin in general.

[![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.0.5` -> `3.1.0`](https://renovatebot.com/diffs/npm/svgo/3.0.5/3.1.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/svgo/3.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/svgo/3.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/svgo/3.0.5/3.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/svgo/3.0.5/3.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>svg/svgo (svgo)</summary> ### [`v3.1.0`](https://togithub.com/svg/svgo/releases/tag/v3.1.0) [Compare Source](https://togithub.com/svg/svgo/compare/v3.0.5...v3.1.0) #### What's Changed ##### Bug Fixes - [**Prefix IDs**](https://svgo.dev/docs/plugins/prefix-ids/), correctly prefix IDs when `<style>` tag contains XML comments. By [@john-neptune](https://togithub.com/john-neptune) in [https://github.com/svg/svgo/pull/1736](https://togithub.com/svg/svgo/pull/1736) - [**Remove Hidden Elements**](https://svgo.dev/docs/plugins/remove-hidden-elems/), improve handling of definitions (`<defs>`), namely when `<defs>` are defined at the end of the document. By [@KTibow](https://togithub.com/KTibow) in [https://github.com/svg/svgo/pull/1879](https://togithub.com/svg/svgo/pull/1879) - [**Cleanup Enable Background**](https://svgo.dev/docs/plugins/cleanup-enable-background/), clean up inline-styles too. By [@SethFalco](https://togithub.com/SethFalco) in [https://github.com/svg/svgo/pull/1866](https://togithub.com/svg/svgo/pull/1866) - [**Cleanup IDs**](https://svgo.dev/docs/plugins/cleanup-ids/), handle URI encoded references properly. By [@SethFalco](https://togithub.com/SethFalco) in [https://github.com/svg/svgo/pull/1880](https://togithub.com/svg/svgo/pull/1880) - [**Inline Styles**](https://svgo.dev/docs/plugins/inline-styles/), no longer crashes on non-inlined selectors with a pseudo-class. By [@SethFalco](https://togithub.com/SethFalco) in [https://github.com/svg/svgo/pull/1865](https://togithub.com/svg/svgo/pull/1865) - Improve handling of URL references in reference attributes (`url('#a')`) across various plugins. By [@SethFalco](https://togithub.com/SethFalco) in [https://github.com/svg/svgo/pull/1881](https://togithub.com/svg/svgo/pull/1881) ##### SVG Optimization - [**Remove Unknowns and Defaults**](https://svgo.dev/docs/plugins/remove-unknowns-and-defaults/), now removes defaults in XML declarations too, enabled by default and can be toggled with `defaultMarkupDeclarations`. By [@SethFalco](https://togithub.com/SethFalco) in [https://github.com/svg/svgo/pull/1872](https://togithub.com/svg/svgo/pull/1872) #### Metrics Before and after using vectors from various sources, with the default preset of each respective version: | SVG | Original | v3.0.5 | v3.1.0 | Delta | |---|---|---|---|---| | [Arch Linux Logo](https://archlinux.org/art/) | 9.529 KiB | 4.608 KiB | 4.162 KiB | ⬇️ 0.446 KiB | | [Blobs](https://gitlab.gnome.org/GNOME/gnome-backgrounds/-/blob/main/backgrounds/blobs-d.svg) | 50.45 KiB | 42.949 KiB | 42.949 KiB | | | [Isometric Madness](https://inkscape.org/~Denis_Kuznetsky/%E2%98%85isometric-madness) | 869.034 KiB | 550.153 KiB | 550.153 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 | 117.146 KiB | 116 KiB | ⬇️ 1.146 KiB | Before and after of the browser bundle of each respective version: | | v3.0.5 | v3.1.0 | Delta | |---|---|---|---| | svgo.browser.js | 657.5 kB | 660.9 kB | ⬆️ 3.4 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. --- - [ ] 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).

fix(removeHiddenElems): handle defs better

b179707

Fix all regression tests

2ad4495

SethFalco reviewed Dec 9, 2023

View reviewed changes

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js

KTibow added 2 commits December 9, 2023 12:36

address review (cleanup code and look at all href)

790767a

add tests (untested)

dcd805f

Remove empty defs without multipass

70a2d93

fix a typo in the test, track all defs like before

9e508ae

KTibow force-pushed the patch-rhe branch from b89748a to 9e508ae Compare December 9, 2023 21:23

remove a out of scope change

2b93feb

SethFalco reviewed Dec 9, 2023

View reviewed changes

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js Outdated

Comment thread plugins/removeHiddenElems.js Outdated

Apply suggestions from code review

4298044

Co-authored-by: Seth Falco <seth@falco.fun>

SethFalco merged commit 2d9e101 into svg:main Dec 9, 2023

BrewTestBot mentioned this pull request Dec 11, 2023

svgo 3.1.0 Homebrew/homebrew-core#156926

Merged

X-oss-byte mentioned this pull request May 25, 2024

[Snyk] Upgrade svgo from 3.0.2 to 3.2.0 X-oss-byte/upgrading-ethereum-book#262

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(removeHiddenElems): handle defs better#1879

fix(removeHiddenElems): handle defs better#1879
SethFalco merged 8 commits intosvg:mainfrom
KTibow:patch-rhe

KTibow commented Dec 9, 2023 •

edited

Loading

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco commented Dec 9, 2023 •

edited

Loading

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SethFalco commented Dec 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

KTibow commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco commented Dec 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KTibow commented Dec 9, 2023

Uh oh!

SethFalco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SethFalco commented Dec 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KTibow commented Dec 9, 2023 •

edited

Loading

SethFalco commented Dec 9, 2023 •

edited

Loading