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

Fix clipping on css-transformed elements #1084

Closed
wants to merge 2 commits into from

Conversation

5 participants
@eKoopmans
Copy link
Contributor

commented Mar 26, 2017

Bug: Clipping on CSS-transformed elements

Elements with transform CSS apply a transformation directly to the canvas, adjusting all canvas actions. This causes incorrect clip bounds, since the transformations are not considered when calculating bounds.

bugfix-clip-transform

Fix

Any canvas transform is temporarily undone while applying clips, then re-applied before rendering the element. This involved giving clip access to the container object to test for transforms.

Related issues/pull requests

#220, #349, #510, #978

@eKoopmans eKoopmans force-pushed the eKoopmans:bugfix/clip-transform branch from 2dbd1e3 to 3aafe82 Mar 27, 2017

@oizie

This comment has been minimized.

Copy link

commented Apr 27, 2017

This would a good PR. Plus the webdriver task it's being skipped and at least is the first PR sucessful since long time ago.

@haxxxton

This comment has been minimized.

Copy link

commented Jul 17, 2017

@eKoopmans, just implemented this in my own fork, and found that it caused text to not render. The included examples basically rendered as if the the text had a transparent colour.

The if function is throwing an error when it tries to check .hasTransform on text node elements. The error message:

"Failed to execute 'getComputedStyle' on 'Window': parameter 1 is not of type 'Element'."

I fixed this by changing the NodeContainer.prototype.computedStyle function to be:

NodeContainer.prototype.computedStyle = function(type) {
    if (this.node.nodeType !== 3) {
    	return this.node.ownerDocument.defaultView.getComputedStyle(this.node, type);
    }
    return this.node.ownerDocument.defaultView.getComputedStyle(window.document.createElement('div'), type);
};

Unfortunately, this meant I needed to also update your .shadow function from PR #1086 to check if the shadow wasnt an empty string by changing the drawShadow function to:

var drawShadow = function(shadow) {
    if (shadow !== "") {
        var info = parseShadow(shadow);
        if (!info.inset) {
            context.shadowOffsetX = info.x;
            context.shadowOffsetY = info.y;
            context.shadowColor = info.color;
            context.shadowBlur = info.blur;
            context.fill();
        }
    }
};
@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

Hi @haxxxton, definitely interesting, I wasn't able to reproduce though: Fiddle (text shows up fine for me in this).

You'll notice here that for paintText I send container.parent to clip, so it looks like that parent is then having trouble. .hasTransform calls .parseTransformMatrix which calls .prefixedCss which calls .css, and when it gets there it needs to be an Element for getComputedStyle.

All that said, it should be sufficient to change line 72 of canvas.js to if (container instanceof Element && container.hasTransform()) {, but I'm curious what context it's getting a container that isn't an Element.

@haxxxton

This comment has been minimized.

Copy link

commented Jul 17, 2017

@eKoopmans, huge apologies about this.. as this isnt a pull request on a repo im in control of, i had to copy-paste the changes across, and i missed that subtle difference of container.parent over container. Changing it to the correct code fixes the comment i raised. Again, sorry for wasting your time :)

If you're interested in an npm accessible version of html2canvas that has a number of these PR's merged, you can have a look here.

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2017

Hey @haxxxton, no worries, glad it was a simple fix. Thanks for the npm link!

@gharwood1

This comment has been minimized.

Copy link

commented Jul 20, 2017

Hey @eKoopmans

I believe this is causing a issue with:

.transform {
  -webkit-transform: rotate(-60deg);
  transform: rotate(-60deg);
}

Check out this fiddle

Thanks for your work!

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2017

Hi @gharwood1, you're right that rotations still aren't handled correctly by this pull request (though it's still an improvement) - see issue #1138 for a bit more info, specifically this fiddle illustrates the problem. I've also created a modified version of your fiddle using divs, again to illustrate that clipping isn't being done correctly.

On Chrome at least, the fiddle you posted illustrates a second issue, which is that spans (and anything that's not display: block) shouldn't have any transforms applied to them (info here). So even though you've applied the rotate style, the original span appears un-rotated, but html2canvas rotates it. Definitely interesting!

Back to the original clipping issue: As I mentioned at #1138, fixing the clipping issue on rotated spans would require a much more serious rewrite of how clipping is being done in html2canvas. Right now html2canvas stores a stack of bounds information for all of an element's ancestors, and when drawing each element it applies the full stack of clips based on those bounds. Since the rotated element needs a rotated clip, either the bounds need to be stored in a rotated form and all further bounds need to have that rotation as part of their bounds calculation, or (better IMO) clipping and transforms can be applied as html2canvas descends through its children, so it can be applied correctly once and used for all children.

@niklasvh niklasvh added this to Backlog in Backlog Jul 27, 2017

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Important update: This PR introduces new issues for certain CSS transformations and should be used with caution (see my last comment for more info). I apologize for any problems it may have caused.

@niklasvh, what are your thoughts on applying each node's clipping to the canvas as you descend down the tree, and restoring the canvas to its previous state on the way back out, rather than the current method of keeping an array of clipping bounds?

@niklasvh

This comment has been minimized.

Copy link
Owner

commented Aug 3, 2017

@eKoopmans I had a stab at overflows last night (as well as with transforms). They certainly are a bit problematic.

I think we cannot just apply the clips as we descend the tree as the drawing order of the canvas is based on stacking contexts instead of the DOM tree order (which overflows use). One alternative I thought about was applying transforms on a node level instead of globally for the whole stack and descendants, that way you could apply the clips along with the transforms together, in the right order. I've yet to try it out, and probably will first focus on getting the v1.0.0 branch up to par with the current functionality before I continue working on this.

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

Right, fair enough. The other alternative could be to keep things as is but apply transforms to the clip vectors... Not sure if that would cause other problems.

@niklasvh

This comment has been minimized.

Copy link
Owner

commented Aug 3, 2017

That could be an alternative as well, but may get quite tricky with border-radius bezier curves etc?

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

Yeah that was what stopped me when I started going down that path originally. I don't understand all the bezier calculations, especially with the inner and outer borders.

@niklasvh

This comment has been minimized.

Copy link
Owner

commented Aug 3, 2017

Will need to think about this a bit more, but for now focusing on getting the remaining features in first.

@niklasvh niklasvh force-pushed the niklasvh:master branch 3 times, most recently from 932773c to 1584357 Dec 3, 2017

@eKoopmans

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2017

This PR still needs work (see comment and #1138). The issue isn't yet resolved in v1.0.0-a1 when using non-foreignObject rendering.

@niklasvh niklasvh referenced this pull request Jan 8, 2018

Closed

border-radius bug #1351

kring added a commit to TerriaJS/html2canvas that referenced this pull request Apr 22, 2018

Don't apply clipping for the root element.
The idea is that the root element coincides with the canvas, so anything
outside the bounds of the root element would be outside the canvas
anyway. This isn't quite true in the fancy of fancy CSS transforms, but
it's true in normal cases. Mainly, it works around a bug in html2canvas
where parent clipping regions are applied incorrectly when rendering
child elements. In TerriaJS, this manifests as point markers (e.g. in
`#test`'s TerriaJS Test Data -> GeoJSON -> Test overriding styled and
unstyled features) not showing up in the generated image. More details
of the html2canvas issue can be found here:
niklasvh#1084

@niklasvh niklasvh referenced this pull request May 3, 2019

Merged

Typescript conversion #1828

@niklasvh niklasvh closed this May 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.