feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

jdufresne · 2023-12-03T19:44:53Z

The new removeDeprecatedAttributes removes deprecated attributes from
the SVG document. For example, the "version" attribute on the "svg"
element is deprecated and not recommended as documented on MDN:

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/version

Deprecated: This feature is no longer recommended. Though some
browsers might still support it, it may have already been removed from
the relevant web standards, may be in the process of being dropped, or
may only be kept for compatibility purposes. Avoid using it, and
update existing code if possible; see the compatibility table at the
bottom of this page to guide your decision. Be aware that this feature
may cease to work at any time.

The document:

<svg version="1.1" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
  <rect x="10" y="10" width="80" height="80"/>
</svg>

Becomes:

<svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg">
  <rect x="10" y="10" width="80" height="80"/>
</svg>

The plugin has a concept of "safe" and "unsafe" deprecated attributes. A
safe attribute is on that, when removed, does not alter the rendering.
An example is the "version" attribute described above. An unsafe
attribute is one that does change the rendering. By default, the plugin
only removes the safe attributes. The param "removeUnsafe" can be used
to also remove the unsafe ones.

Deprecated attributes may also appear in the SVG stylesheet as a
selector. When this occurs, the attribute won't be removed from the
document to avoid additional rendering changes.

The "xml:lang" attribute is deprecated but also has a replacement,
"lang", so this attribute is treated as a special case and is safe when
its replacement exists.

The plugin is built for easy expansion as new deprecated attributes are
discovered or announced. Simply add to ones of the "deprecated" sets in
plugins/_collections.js.

Fixes #1701

KTibow · 2023-12-03T19:46:48Z

If we go by the MDN sidebar there are many more deprecated attributes. Some affect how the SVG looks though. Example: xml:space, which is supposed to change whitespace handling, although it didn't change it in my tests in Firefox.

jdufresne · 2023-12-03T19:49:38Z

Thanks!

Are you suggesting they too be included in the deprecated list? I'm happy to do that if you'd like.

Any that affect rendering can be omitted as unsafe.

KTibow · 2023-12-03T19:50:53Z

Yeah I would suggest adding all ones marked as deprecated by MDN that are verified to not affect rendering for multiple renderers

plugins/removeDeprecatedAttrs.js

jdufresne · 2023-12-03T20:05:56Z

Yeah I would suggest adding all ones marked as deprecated by MDN that are verified to not affect rendering for multiple renderers

I pushed a new revision that includes all attributes marked deprecated on MDN.

SethFalco

Hey! Thanks for opening the PR.

The plugin makes sense, but I wouldn't make it a default.
This will likely break many SVGs in non-browser clients, so it should be opt-in.

I think it'd be cool to merge though if disabled by default, and the issue with how the deprecated attributes are stored in _collections.js is resolved.

plugins/_collections.js

plugins/preset-default.js

plugins/_collections.js

plugins/removeDeprecatedAttrs.js

jdufresne · 2023-12-04T00:52:09Z

Thanks for the review! I believe I have addressed all feedback:

Removed the plugin from default
Dropped xlink attribute from the "deprecated" array
Switch style to early return
Moved the array away from "svg". As I understand the documentation, these attributes are universally deprecated, so I've created a single "deprecatedAttrs" array that applies to all elements. Let me know how you feel about this approach. If you still think it should be broken down per element, I can that path too.

docs/03-plugins/remove-deprecated-attrs.mdx

plugins/_collections.js

jdufresne · 2023-12-08T04:39:27Z

@SethFalco Thanks for all reviews!

I believe I have addressed all feedback, but definitely let me know.

In the latest revision I have set a deprecated array per element type in _collections.js.

There is also a new attrsGroupsDeprecated to define deprecated attributes for group attributes.

All additional feedback is welcome.

SethFalco · 2023-12-12T02:35:08Z

I've looked a bit into this, and there are four properties that could be handled differently, however taking a migration path seems to be complicated for these plugins, so it might be better to shelve that idea for now.

While investigating this, I made some notes, so I'm just leaving them here for future reference.

`clip`

Convert to the CSS clip-path property. As KTibow pointed out in #1869 (comment), this can be translated to the inset. However, I have found some weirdness that makes this annoying…

Client	Supports `clip`	Supports equivalent	Impacts rendering
Eye of GNOME	❌	❌	No
Inkscape	❌	❌	No
Firefox (SVG)	✅	➖	Yes but…¹
Firefox (Inline)	✅	➖	Yes but…¹
Chromium (SVG)	❌	✅	Yes
Chromium (Inline)	❌	✅	Yes

¹ clip-path has a bug in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1869456

`requiredFeatures`

If the same node doesn't have requiredExtensions or systemLanguage, and the parent is a <switch>, move it to the parent node and drop the <switch> along with the rest of its children.

`xml:lang`

Convert to SVG lang attribute. It can be moved as-is. Important for accessibility.

node.attributes.lang = node.attributes['xml:lang'];
delete node.attributes['xml:lang'];

`xml:space`

Convert to CSS white-space property. But this also has some messiness…

MDN suggests using white-space instead. From what I can see, the equivalent would be replacing all whitespace characters with a single space (/\s/g → ), then using white-space:pre.

Here is the support, though:

Client	Supports `xml:space`	Supports equivalent	Impacts rendering
Eye of GNOME	✅	❌	Yes
Inkscape	❌	✅	Yes
Firefox (SVG)	✅	✅	No
Firefox (Inline)	❌	✅	Yes
Chromium (SVG)	❌	❌	No
Chromium (Inline)	❌	❌	No

I was thinking that migrating would be good to avoid issues with breaking modern clients… but nothing is that simple, I'll investigate those more later but as separate plugins. ^-^'

Let's keep this plugin focused on removing params, but could you please add a parameter that separates the safe and unsafe attributes? Even if we ignore migrating, we can see in the tables above that dropping some of these attributes can impact rendering on modern clients/browsers. I'd prefer if the plugin ran in a "safe mode" by default. Otherwise, it wouldn't be very practical to use.

I'd be happy to help you decide which is which in a separate comment later this week. Some clear examples are that version is safe to remove, while clip, xml:space, and xml:space are unsafe to remove… though I am wary that version might be the only safe one which would make that redundant. 🤔

Then by default, this plugin can perform safe operations only, and the parameter can be passed to include unsafe operations for those that want it. The docs for the unsafe parameter should clarify that it may impact rendering, even on modern clients and browser versions.

jdufresne · 2023-12-24T18:22:57Z

@SethFalco I believe I have addressed the feedback in the latest revision.

The deprecated attributes are now split into safe/unsafe. By default, only safe attributes are removed but through a param, unsafe can also be removed. At this time, the only safe attribute is "version".

Do you think this change would allow making this plugin included by default now?

Thanks for the continued reviews.

SethFalco

Hey! Thanks for continuing to work on this.

I've left a few more comments, but this is looking pretty good now. Very happy to see us open a path to remove version by default too.

At this time, the only safe attribute is "version".

We'll figure out if we can do any of the others post-merge. Would be nice if we could at least tackle xml:lang in this PR too though if lang is defined.

Do you think this change would allow making this plugin included by default now?

Yeah! I think this would be good for a default plugin. Happy to have it enabled now that it's safe by default.

Welcome to tell me if any of my comments confuse you, if you're short on time. Happy to help out. 👍🏽

docs/03-plugins/remove-deprecated-attrs.mdx

plugins/_collections.js

test/plugins/_collections.test.js

plugins/_collections.js

plugins/removeDeprecatedAttrs.js

jdufresne · 2023-12-31T18:17:14Z

Thanks again for the review. I believe I have addressed all comments including the lang attribute and CSS check. All feedback is welcome.

SethFalco

Hey! Thanks for handling all the feedback, this looks great.

Some other ways we could improve the plugin, but I'd be happy to merge it as-is, and create an issue to tackle styles in future:

The write-up above regarding handling styles. So, to remove deprecated presentation attributes from inline styles and stylesheets.
Investigate if it's worth conditionally removing enable-background as safe by duplicating or referencing the logic from the cleanupEnableBackground plugin.
Investigate all the other deprecated attributes and see if we can identify when some of them can be safely removed.

If you are willing and have time to tackle styles in this PR, that would be very welcome, but no pressure. I'm not sure what the best approach for handling enable-background is, so we can figure that out later (ideas welcome), same with investigating other deprecated parameters as that will take too long, so we'll work on it iteratively.

SethFalco · 2023-12-31T18:47:23Z

plugins/_collections.js

+  animationAttributeTarget: { unsafe: new Set(['attributeType']) },
+  conditionalProcessing: { unsafe: new Set(['requiredFeatures']) },
+  core: { unsafe: new Set(['xml:base', 'xml:lang', 'xml:space']) },
+  presentation: {


Presentation attributes can be used in styles as well. We should probably be removing them too.

Do you have time to add a check, where if the attribute being removed is a presentation attribute, also parse the style attribute and remove instances from there too. If the style attribute becomes empty, delete the style attribute entirely.

I worked on something like this recently in the cleanupEnableBackground plugin, so you can use it as a reference:

https://github.com/svg/svgo/pull/1866/files#diff-0805bdcf601afdd4c7674c3595fee8ea9da61677f6f36a4fdff25bd3f39fb840R45-R62

Basically, if the node defines the style attribute, parse the styles and if it's a DeclarationList, iterate the list and remove unwanted styles. Finally, you can generate the new styles with csstree.generate and save it back, or delete the attribute if it's become empty.

Instead of collecting the declarations like I did, you can just remove them immediately from the list from inside css.walk I think. 🤔

Important to note that the same attribute can be redundantly defined more than once in CSS, so it's worth looping through all styles and not exit early if the attribute is found.

It could be worth updating the stylesheet (<style> tags) too.

jdufresne · 2023-12-31T19:24:43Z

Thanks @SethFalco!

I like the idea of merging the PR as-is and then documenting the future enhancements as issues. Especially if you believe this is in a usable state.

jdufresne · 2024-01-04T04:00:58Z

Rebased and switched to the new ESM.

The new removeDeprecatedAttributes removes deprecated attributes from the SVG document. For example, the "version" attribute on the "svg" element is deprecated and not recommended as documented on MDN: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/version > Deprecated: This feature is no longer recommended. Though some > browsers might still support it, it may have already been removed from > the relevant web standards, may be in the process of being dropped, or > may only be kept for compatibility purposes. Avoid using it, and > update existing code if possible; see the compatibility table at the > bottom of this page to guide your decision. Be aware that this feature > may cease to work at any time. The document: ``` <svg version="1.1" viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg"> <rect x="10" y="10" width="80" height="80"/> </svg> ``` Becomes: ``` <svg viewBox="0 0 100 100" xmlns="http://www.w3.org/2000/svg"> <rect x="10" y="10" width="80" height="80"/> </svg> ``` The plugin has a concept of "safe" and "unsafe" deprecated attributes. A safe attribute is on that, when removed, does not alter the rendering. An example is the "version" attribute described above. An unsafe attribute is one that does change the rendering. By default, the plugin only removes the safe attributes. The param "removeUnsafe" can be used to also remove the unsafe ones. Deprecated attributes may also appear in the SVG stylesheet as a selector. When this occurs, the attribute won't be removed from the document to avoid additional rendering changes. The "xml:lang" attribute is deprecated but also has a replacement, "lang", so this attribute is treated as a special case and is safe when its replacement exists. The plugin is built for easy expansion as new deprecated attributes are discovered or announced. Simply add to ones of the "deprecated" sets in plugins/_collections.js. Fixes svg#1701

SethFalco · 2024-01-15T21:48:52Z

Sorry for the delay with merging this. This looks good, thanks for working on it and being so receptive to feedback.

KTibow reviewed Dec 3, 2023

View reviewed changes

plugins/removeDeprecatedAttrs.js Outdated Show resolved Hide resolved

jdufresne force-pushed the remove-deprecated-attributes branch from dbb6b72 to f8e4fa6 Compare December 3, 2023 20:05

SethFalco requested changes Dec 3, 2023

View reviewed changes

jdufresne force-pushed the remove-deprecated-attributes branch 2 times, most recently from 09a6402 to 84a8f17 Compare December 4, 2023 00:46

jdufresne force-pushed the remove-deprecated-attributes branch from 84a8f17 to e11379a Compare December 4, 2023 01:02

SethFalco reviewed Dec 7, 2023

View reviewed changes

docs/03-plugins/remove-deprecated-attrs.mdx Show resolved Hide resolved

docs/03-plugins/remove-deprecated-attrs.mdx Outdated Show resolved Hide resolved

plugins/_collections.js Outdated Show resolved Hide resolved

SethFalco reviewed Dec 7, 2023

View reviewed changes

plugins/_collections.js Outdated Show resolved Hide resolved

jdufresne force-pushed the remove-deprecated-attributes branch 3 times, most recently from 16ef45b to 614bffb Compare December 8, 2023 04:35

jdufresne force-pushed the remove-deprecated-attributes branch from 614bffb to 8512bd3 Compare December 24, 2023 18:16

jdufresne requested a review from SethFalco December 24, 2023 18:23

jdufresne force-pushed the remove-deprecated-attributes branch from 8512bd3 to 571decf Compare December 24, 2023 18:25

SethFalco mentioned this pull request Dec 24, 2023

chore: Remove unused argument "input" in calls to optmize() #1901

Merged

SethFalco requested changes Dec 31, 2023

View reviewed changes

jdufresne force-pushed the remove-deprecated-attributes branch 4 times, most recently from 85ef1cc to 18290a4 Compare December 31, 2023 17:29

SethFalco approved these changes Dec 31, 2023

View reviewed changes

jdufresne force-pushed the remove-deprecated-attributes branch from 18290a4 to a7d4dc4 Compare January 4, 2024 03:57

jdufresne force-pushed the remove-deprecated-attributes branch from a7d4dc4 to 2b3909a Compare January 4, 2024 23:34

SethFalco merged commit 59796a8 into svg:main Jan 15, 2024
8 checks passed

jdufresne deleted the remove-deprecated-attributes branch January 15, 2024 21:50

SethFalco mentioned this pull request Jan 19, 2024

removeXMLSpace plugin: remove xml:space attribute for inline svg usage #1660

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

jdufresne commented Dec 3, 2023 •

edited

KTibow commented Dec 3, 2023 •

edited

jdufresne commented Dec 3, 2023

KTibow commented Dec 3, 2023

jdufresne commented Dec 3, 2023

SethFalco left a comment

jdufresne commented Dec 4, 2023

jdufresne commented Dec 8, 2023

SethFalco commented Dec 12, 2023 •

edited

jdufresne commented Dec 24, 2023

SethFalco left a comment

jdufresne commented Dec 31, 2023

SethFalco left a comment •

edited

SethFalco Dec 31, 2023 •

edited

jdufresne commented Dec 31, 2023

jdufresne commented Jan 4, 2024

SethFalco commented Jan 15, 2024

feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

Conversation

jdufresne commented Dec 3, 2023 • edited

KTibow commented Dec 3, 2023 • edited

jdufresne commented Dec 3, 2023

KTibow commented Dec 3, 2023

jdufresne commented Dec 3, 2023

SethFalco left a comment

Choose a reason for hiding this comment

jdufresne commented Dec 4, 2023

jdufresne commented Dec 8, 2023

SethFalco commented Dec 12, 2023 • edited

clip

requiredFeatures

xml:lang

xml:space

jdufresne commented Dec 24, 2023

SethFalco left a comment

Choose a reason for hiding this comment

jdufresne commented Dec 31, 2023

SethFalco left a comment • edited

Choose a reason for hiding this comment

SethFalco Dec 31, 2023 • edited

Choose a reason for hiding this comment

jdufresne commented Dec 31, 2023

jdufresne commented Jan 4, 2024

SethFalco commented Jan 15, 2024

jdufresne commented Dec 3, 2023 •

edited

KTibow commented Dec 3, 2023 •

edited

SethFalco commented Dec 12, 2023 •

edited

`clip`

`requiredFeatures`

`xml:lang`

`xml:space`

SethFalco left a comment •

edited

SethFalco Dec 31, 2023 •

edited