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

toSvg regression for explicit CSS values equal to inherit #90

Closed
2 tasks done
zm-cttae opened this issue Dec 19, 2022 · 5 comments
Closed
2 tasks done

toSvg regression for explicit CSS values equal to inherit #90

zm-cttae opened this issue Dec 19, 2022 · 5 comments

Comments

@zm-cttae
Copy link

zm-cttae commented Dec 19, 2022

Use case: description, code

jsfiddle

<div id='dom-node'>
<span><a href='#dom-node'>dom-to-image-more/issues/90</a></span>
</div>

<div id='result'>
<img src='data:image/svg+xml;charset=utf-8,<svg xmlns="http://www.w3.org/2000/svg" width="232" height="132"><foreignObject x="0" y="0" width="100%" height="100%"><div id="dom-node" xmlns="http://www.w3.org/1999/xhtml" style="background-color: rgb(211, 211, 211); block-size: 100px; display: block; height: 100px; inline-size: 200px; padding-block: 16px; padding: 16px; padding-inline: 16px; text-align: center; width: 200px;">%0A<span><a href="%23dom-node" style="cursor: pointer; text-decoration-line: underline; text-decoration-thickness: initial;">dom-to-image-more/issues/90</a></span>%0A</div></foreignObject></svg>'></img>
</div>
#dom-node {
  color: #000000;
  height: 100px;
  width: 200px;
  background-color: lightgrey;
  text-align: center;
  padding: 1em;
}

a[href="#dom-node"] {
  color: #000000;
}

#result {
  margin-top: 10px;
}

Expected behavior

Related issues & PRs: #37 #71

It was taken as true that unset had no gotchas. I tried using toSvg on .Box in https://github.com/pulls. copyUserComputedStyle is an over-eager optimisation and has value collisions.

In this case, link color overrides that match inherit (as in .Box .Link--primary) are stripped away prematurely.

Here's an explanation of the copyUserComputedStyle regression:

  1. Initial value from user-agent origin = -webkit-link/-moz-hyperlinktext
  2. Stylesheet from author origin sets it to an explicit color value, which happens to be the computed value of inherit
  3. copyUserComputedStyle assigns unset inline which is computed as inherit
  4. Because the computed value of unset and and the explicit color value match, that value is not added to the inline style
  5. 👉 The output SVG's inline style has no explicit color style. So instead of 2, we're literally back to square 1. 😃

Actual behavior (stack traces, console logs etc)

When you open up the #result image src from the jsfiddle demo, it appears purple.

Library version

Regression confirmed in https://github.com/1904labs/dom-to-image-more/releases/tag/v2.12.0 and present since https://github.com/1904labs/dom-to-image-more/releases/tag/v2.9.1.

Browsers

  • Chrome 49+
  • Firefox 45+
@IDisposable
Copy link
Member

Whoa, nice catch, sorry for not seeing this when first PRed. I'll introduce a regression test and make it pass :) Seems your approach to the fast-path should work.

@zm-cttae
Copy link
Author

zm-cttae commented Dec 20, 2022

I was of the impression iframe insertion would break with the newish Trusted Type API. (There is a createHTML filter but it only filters DOM strings e.g. innerHTML setter.)

The library continued to work fine with the following code and corresponding CSP:
trustedTypes.createPolicy('default', {
    // This code block needs security review, as it's capable of causing DOM XSS.
    createHTML(dirty) { return DOMPurify.sanitize(dirty, {RETURN_TRUSTED_TYPE: true}) }
})

This means we are all green to rely on copyUserComputedStyleFast!

@zm-cttae
Copy link
Author

Strangely enough, I think the edge case in this issue is described in the comments:

// If the style does not match the default, or it does not match the parent's, set it. We don't know which
// styles are inherited from the parent and which aren't, so we have to always check both.

copyUserComputedStyleFast should be "good enough" to avert the heavy data output problem in #36 and be the only active method going forward.

Need to make a tracking issue to see if there's a really robust method for working with CSS inheritance (both A. stripping extraneous inheritable CSS props, and B. adding back values when CSS explicitly sets inherit or its computed value).

@IDisposable
Copy link
Member

I'm addressing this in a new PR... would love to give credit on the package and readmd, but all I have is your github ID

IDisposable added a commit that referenced this issue Dec 20, 2022
Fixes `toSvg` regression for explicit CSS values equal to `inherit` #90 by killing off the entire raster/vector decision path and always using the `copyUserComputedStyleFast` method.
Cleaned up unit testing a bit more and moved to the npm packaged version of imagediff.
@IDisposable
Copy link
Member

Addressed this more completely in #91

IDisposable added a commit that referenced this issue Dec 20, 2022
Fixes `toSvg` regression for explicit CSS values equal to `inherit` #90 by killing off the entire raster/vector decision path and always using the `copyUserComputedStyleFast` method.
Cleaned up unit testing a bit more and moved to the npm packaged version of imagediff.
IDisposable added a commit that referenced this issue Dec 20, 2022
Fixes `toSvg` regression for explicit CSS values equal to `inherit` #90 by killing off the entire raster/vector decision path and always using the `copyUserComputedStyleFast` method.
Cleaned up unit testing a bit more and moved to the npm packaged version of imagediff.
IDisposable added a commit that referenced this issue Dec 20, 2022
Fixes `toSvg` regression for explicit CSS values equal to `inherit` #90 by killing off the entire raster/vector decision path and always using the `copyUserComputedStyleFast` method.
Cleaned up unit testing a bit more and moved to the npm packaged version of imagediff.
IDisposable added a commit that referenced this issue Dec 20, 2022
Fixes `toSvg` regression for explicit CSS values equal to `inherit` #90 by killing off the entire raster/vector decision path and always using the `copyUserComputedStyleFast` method.
Cleaned up unit testing a bit more and moved to the npm packaged version of imagediff.
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

No branches or pull requests

2 participants