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

Work on table padding issue #95 #104

Merged
merged 5 commits into from
Jan 15, 2023
Merged

Work on table padding issue #95 #104

merged 5 commits into from
Jan 15, 2023

Conversation

IDisposable
Copy link
Member

@IDisposable IDisposable commented Jan 12, 2023

TARGETS master for the v2-release-chain

Changed getDefaultStyle to create the element in the sandbox so it is pure (not tainted by CSS / ShadowDOM in the main document).
Never clone SCRIPT elements (that's just asking for trouble).
Removed imagediff package as it's not useful anymore.

Changed `getDefaultStyle` to create the element in the sandbox so it is pure (not tainted by CSS / ShadowDOM in the main document).
Never clone SCRIPT elements (that's just asking for trouble).
Removed `imagediff` package as it's not useful anymore.
@IDisposable
Copy link
Member Author

@meche-gh can you see if this is makes sense to you?

I THINK the problem is that we're creating the element for the tagName to extract the default styles in our actual window.contentWindow.document, not in the sandbox.

This means that global styles that are applied to the tags like a css rule td { padding: 0px; } in the actual DOM will copy those "not really default" properties over to the sandboxed element. Later, when we compare the source element's style it looks unchanged from the defaults, but that's not true.

Rather, I think should be creating the element against the sandbox.contentWindow's document so the properties are not tainted and reflect what the browser would really have defaulted to.

@zm-cttae
Copy link

Sounds good to me. I noticed that the iframe getComputedStyle is a little slower (i.e. forcing a layout recompute within the iframe) - if that's a needed investment then this is the way

@IDisposable
Copy link
Member Author

IDisposable commented Jan 12, 2023

Much playing around later, it's not possible to fix because something is keeping the padding: 0px; from making it from the original element to the cloned element SVG, even though the values are coded to set on the cloned node, but in the cloned node they are not set. I think the style cloning is not seeing the class-driven values.

I've verified this by adding to the original fiddle leveraging the ability to insert the cloned node into the existing page and you can see that it did NOT inline the padding.

See https://jsfiddle.net/Lrgkwt5m/117/ or https://jsfiddle.net/Lrgkwt5m/119/ with this current source loaded instead

Note that setting the TH, TD { padding: 0.00001px; } gets things emitted without changing the original layout visibly... so there is a hack-around.

The allows inspecting the properties of the clone.
Removed the setting of the `top` and `left`
Set the `width` and `height` on the `foreignObject` element to the target size.
Removed setting of `width` and `height` on the `svg` element.
Falls back to the old `scrollWidth`/`scrollHeight` logic if the `width` and `height` aren't set in `px`.
@IDisposable
Copy link
Member Author

Pushed the final state of the v2 based code cleanup. Still doesn't fix the original bug (because I think it's a browser issue), but this code is cleaner and less likely to break.

tagNameDefaultStyles[tagName] = defaultStyle;
return defaultStyle;
}

function removeSandbox() {
if (!sandbox) {
return;
if (sandbox) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In an earlier commit of this branch, I accidentally left the ! on this line. The net effect is that we never dropped the sandbox. Sorry.

@IDisposable
Copy link
Member Author

@meche-gh feel free to make any comments before I merge this tomorrow.

})
.then(function (foreignObject) {
return `<svg xmlns="http://www.w3.org/2000/svg" width="${width}" height="${height}">${foreignObject}</svg>`;
return `<svg xmlns="http://www.w3.org/2000/svg">${foreignObject}</svg>`;
Copy link

@zm-cttae zm-cttae Jan 12, 2023

Choose a reason for hiding this comment

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

I personally think we should set a viewport here

Choose a reason for hiding this comment

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

Or we could do both! whatwg/html#3510 (comment) - seems like SVGs just need both for good cross-browser compat..

Copy link
Member Author

@IDisposable IDisposable Jan 12, 2023

Choose a reason for hiding this comment

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

viewBox is problematic because those values are floats, not integers. so there's a great deal of vagueness about the whole thing. Additionally BIMI icons forbid them (for good reason) so I'm leaning away from this. We can ALREADY force the width and height in the call options

Copy link

@zm-cttae zm-cttae Jan 13, 2023

Choose a reason for hiding this comment

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

This lib would be generating SVGs without a bounding box. I consider this to be "canvas mode" - it's an image but without any notion of natural dimensions. Less useful and likely a major change in semver.
It's more convenient for devs to compute the natural dimension matching the original element plus its border. width="${width}" height="${height}" is valuable imho

Copy link
Member Author

Choose a reason for hiding this comment

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

I've restore the width and height on the SVG image.

@IDisposable
Copy link
Member Author

IDisposable commented Jan 13, 2023 via email

@IDisposable
Copy link
Member Author

Had a thought last night at the hockey game (the Blues were being terrible in the third).

What if the issue is that at the time the styles are being copied over, the destination TH/TD has all the default properties so we don't mechanistically set the padding. Then later in the process we set the table-cell style and that's when the 1px padding is kicking in... if this is the case, I would have to figure out the right order to copy style properties over in... that sounds like a nightmare.

@zm-cttae
Copy link

THEY'Re fixing it!! 🎉

@zm-cttae
Copy link

zm-cttae commented Jan 13, 2023

The only way to bypass the Chromium issue, and also support the default styling on descendants of <ruby>, is to make defaultElement wrapped recursively with the parents of targetElement in getDefaultStyle.

We would iterate upwards on the DOM tree and get all the parent tag names going up to document.documentElement - that would definitely futureproof it. This would make caching much more complex and arguably break it however.

Merging this PR through is fine but I'd rather just let the bug be fixed and take a short break from CSSOM

@IDisposable
Copy link
Member Author

This sounds like an excellent path forward... I'm going to merge where we are to get back to sanity on the other PRs. Issue #106 capture the remaining issue and expected solution.

@IDisposable IDisposable merged commit eeed471 into master Jan 15, 2023
@IDisposable IDisposable deleted the table_padding branch January 15, 2023 23:24
zm-cttae added a commit to zm-cttae/dom-to-image-more that referenced this pull request Jan 19, 2023
IDisposable added a commit that referenced this pull request Mar 23, 2023
Never clone SCRIPT tags
Better table padding #104
Remove mozImageSmoothing #125
Defer image resolution for Firefox tsayen#214
Ignore failure.html for testing.
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.

2 participants