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

feat(removeDeprecatedAttrs): new removeDeprecatedAttrs plugin #1869

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

jdufresne
Copy link
Contributor

@jdufresne jdufresne commented Dec 3, 2023

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
Copy link
Contributor

KTibow commented Dec 3, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

KTibow commented Dec 3, 2023

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

@jdufresne
Copy link
Contributor Author

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.

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.

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 Outdated Show resolved Hide resolved
plugins/preset-default.js Outdated Show resolved Hide resolved
plugins/preset-default.js Outdated Show resolved Hide resolved
plugins/_collections.js Outdated Show resolved Hide resolved
plugins/removeDeprecatedAttrs.js Outdated Show resolved Hide resolved
@jdufresne jdufresne force-pushed the remove-deprecated-attributes branch 2 times, most recently from 09a6402 to 84a8f17 Compare December 4, 2023 00:46
@jdufresne
Copy link
Contributor Author

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

  1. Removed the plugin from default
  2. Dropped xlink attribute from the "deprecated" array
  3. Switch style to early return
  4. 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 Show resolved Hide resolved
docs/03-plugins/remove-deprecated-attrs.mdx Outdated Show resolved Hide resolved
plugins/_collections.js Outdated Show resolved Hide resolved
plugins/_collections.js Outdated Show resolved Hide resolved
@jdufresne jdufresne force-pushed the remove-deprecated-attributes branch 3 times, most recently from 16ef45b to 614bffb Compare December 8, 2023 04:35
@jdufresne
Copy link
Contributor Author

@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
Copy link
Member

SethFalco commented Dec 12, 2023

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
Copy link
Contributor Author

@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.

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.

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 Outdated Show resolved Hide resolved
plugins/_collections.js Outdated Show resolved Hide resolved
test/plugins/_collections.test.js Show resolved Hide resolved
plugins/_collections.js Outdated Show resolved Hide resolved
plugins/removeDeprecatedAttrs.js Outdated Show resolved Hide resolved
plugins/removeDeprecatedAttrs.js Outdated Show resolved Hide resolved
@jdufresne jdufresne force-pushed the remove-deprecated-attributes branch 4 times, most recently from 85ef1cc to 18290a4 Compare December 31, 2023 17:29
@jdufresne
Copy link
Contributor Author

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

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.

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:

  1. The write-up above regarding handling styles. So, to remove deprecated presentation attributes from inline styles and stylesheets.
  2. Investigate if it's worth conditionally removing enable-background as safe by duplicating or referencing the logic from the cleanupEnableBackground plugin.
  3. 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.

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

@SethFalco SethFalco Dec 31, 2023

Choose a reason for hiding this comment

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

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:

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 SethFalco merged commit 59796a8 into svg:main Jan 15, 2024
8 checks passed
@SethFalco
Copy link
Member

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

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.

Remove deprecated version attribute from top svg element
3 participants