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.

Sign up for GitHub

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

Jump to bottom

Another v1.0.x regression: inlineStyles causing style loss for foreignObject #823

Closed
zmwangx opened this issue Nov 1, 2017 · 8 comments
Closed

Another v1.0.x regression: inlineStyles causing style loss for foreignObject #823

zmwangx opened this issue Nov 1, 2017 · 8 comments

Comments

@zmwangx
Copy link

zmwangx commented Nov 1, 2017

Take the same sample image as in #820:

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="50">
  <foreignObject width="100%" height="100%">
    <style>div { color: red; }</style>
    <body xmlns="http://www.w3.org/1999/xhtml"><div>hello, world</div></body>
  </foreignObject>
</svg>

v0.7.2:

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="50"><foreignObject width="100%" height="100%"><style>div{color:red}</style><body xmlns="http://www.w3.org/1999/xhtml"><div>hello, world</div></body></foreignObject></svg>

v1.0.1:

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="50"><foreignObject width="100%" height="100%"><body xmlns="http://www.w3.org/1999/xhtml"><div color="red">hello, world</div></body></foreignObject></svg>

v1.0.1 --disable=inlineStyles (same as v0.7.2):

<svg xmlns="http://www.w3.org/2000/svg" width="100" height="50"><foreignObject width="100%" height="100%"><style>div{color:red}</style><body xmlns="http://www.w3.org/1999/xhtml"><div>hello, world</div></body></foreignObject></svg>

Apparently, inlineStyles is inlining the CSS color property into a color attribute, but HTML div does not support the color attribute.

This is just a simple case; inlineStyles is causing a lot of very confusing destruction to my more complex foreignObjects with more complex styling. --disable=inlineStyles fixes those cases. Maybe inlineStyles should be disabled altogether for foreignObjects?
@strarsis
Copy link
Contributor

strarsis commented Nov 1, 2017 •
edited
Loading

@zmwangx: The conversion of styles into attributes (like color in your sample)
is not done by inlineStyles but another plugin instead (propably convertStyleToAttrs).
I guess the convertStyleToAttrs plugin requires an additional check for styles compatible as attributes.

@zmwangx
Copy link
Author

zmwangx commented Nov 1, 2017

Okay, then why does disabling inlineStyles fix the problem?

@strarsis
Copy link
Contributor

strarsis commented Nov 1, 2017 •
edited
Loading

@zmwangx: Good point - Because the convertStyleToAttrs can only convert inline styles to attributes, it isn't able to resolve other styles that come from <style>, it needs the inlined styles from inlineStyles in order to do that (although with issues).

@zmwangx
Copy link
Author

zmwangx commented Nov 1, 2017

Thanks for the explanation. Sure, convertStyleToAttrs is the culprit then.

@strarsis
Copy link
Contributor

strarsis commented Nov 1, 2017

@zmwangx: Btw, disabling convertStyleToAttrs fixes the problem, too. :)

@strarsis
Copy link
Contributor

strarsis commented Nov 2, 2017

Is there already data about what properties are supported by what tags?
svgo already contains some data, but I cannot find something related to attribute support per tag:
https://github.com/svg/svgo/blob/master/plugins/_collections.js
Maybe on MDN...

@strarsis strarsis mentioned this issue Nov 2, 2017
Open
@GreLI
Copy link
Member

GreLI commented Nov 3, 2017

What about inlineStyles not working on styles inside <foreignObject>?

@GreLI GreLI closed this as completed in ceccf1f Nov 3, 2017
@GreLI
Copy link
Member

GreLI commented Nov 3, 2017

Made in so in v1.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@GreLI @zmwangx @strarsis