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

bug: removeScriptElement throws away xml:space=preserve for children elements #1960

Open
SethFalco opened this issue Feb 11, 2024 · 2 comments
Labels

Comments

@SethFalco
Copy link
Member

SethFalco commented Feb 11, 2024

Describe the bug

The removeScriptElement plugin will remove elements if they contain scripts. However, if the element that was removed had the xml:space="preserve" attribute, which applies to the children of the element too, this is unintentionally removed and spacing is no longer preserved.

To Reproduce
Steps to reproduce the behavior:

Optimize the following with SVGO with only the removeScriptElement plugin:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100">
  <a href="javascript:(() => alert('uwu');)();" xml:space="preserve">
    <text x="10" y="35">
this is a test
    </text>
  </a>
</svg>

Expected behavior

Immediately, I'm thinking that xml:space="preserve" should be forwarded to children of the removed element. For example, in the scenario above, we'd remove the <a> element, and add xml:space="preserve" to the <text> element. In Firefox this resolves the visual difference.

Desktop (please complete the following information):

  • SVGO Version: main
  • NodeJs Version: v20
  • OS: Debian Testing
@SethFalco SethFalco added the bug label Feb 11, 2024
@KTibow
Copy link
Contributor

KTibow commented Feb 12, 2024

Shouldn't this be generalized to all inheritable attributes, or is this a special case caused by it not being marked as inheritable or something?

@SethFalco
Copy link
Member Author

Shouldn't this be generalized to all inheritable attributes

I haven't looked into it enough to say, but valid take.

It probably applies to all inheritable attributes, and probably should be handled in a utility that flattens the given node into its parent, which we can use everywhere we do this. We'd have to exclude children that set the same attribute as the removed parent did.

But then the annoying part, which is why I reported the issue instead. When testing xml:space before, it looks to me that while xml:space="preserve" applies to children, most browsers then ignore xml:space="default" from children so it continues to preserve whitespace despite being overridden, and I wanted to investigate this further.

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

2 participants