search

bubkoo / html-to-image

✂️ Generates an image from a DOM node using HTML5 canvas and SVG.
MIT License
5.41k stars 505 forks source link

fix: clone iframe nodes better #352

Closed DustinBrett closed 1 year ago

DustinBrett commented 1 year ago

My previous PR adding iframe cloning was incomplete.

Description

There were 2 specific reasons it didn't work:

To solve instanceof I've changed to comparing such as: Object.getPrototypeOf(node).constructor.name === instance.name where instance would be a prototype such as HTMLImageElement. This will also check recursively to see for example if something like HTMLDivElement also has Element in it's prototype chain and is therefore an instance of it.

Another solution suggested in MDN could be something like myNode instanceof myNode.ownerDocument.defaultView.SVGElement, but I had no luck (Illegal invocation) implementing this and I feel the method I have is a bit more ideal than just checking tagName's.

Motivation and Context

I want to fix this as I added iframe capture changes recently which caused this new issue to be visible.

Types of changes

Self Check before Merge

codecov[bot] commented 1 year ago

Codecov Report

Base: 63.36% // Head: 63.24% // Decreases project coverage by -0.12% :warning:

Coverage data is based on head (728755c) compared to base (d58db90). Patch coverage: 65.00% of modified lines in pull request are covered.

:exclamation: Current head 728755c differs from pull request most recent head 79d631d. Consider uploading reports for the commit 79d631d to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #352 +/- ## ========================================== - Coverage 63.36% 63.24% -0.12% ========================================== Files 10 10 Lines 565 555 -10 Branches 134 129 -5 ========================================== - Hits 358 351 -7 + Misses 147 146 -1 + Partials 60 58 -2 ``` | [Impacted Files](https://codecov.io/gh/bubkoo/html-to-image/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96) | Coverage Δ | | |---|---|---| | [src/util.ts](https://codecov.io/gh/bubkoo/html-to-image/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96#diff-c3JjL3V0aWwudHM=) | `60.86% <ø> (ø)` | | | [src/clone-node.ts](https://codecov.io/gh/bubkoo/html-to-image/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96#diff-c3JjL2Nsb25lLW5vZGUudHM=) | `75.92% <56.25%> (-0.44%)` | :arrow_down: | | [src/embed-images.ts](https://codecov.io/gh/bubkoo/html-to-image/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96#diff-c3JjL2VtYmVkLWltYWdlcy50cw==) | `81.81% <100.00%> (+3.76%)` | :arrow_up: | | [src/embed-webfonts.ts](https://codecov.io/gh/bubkoo/html-to-image/pull/352?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96#diff-c3JjL2VtYmVkLXdlYmZvbnRzLnRz) | `30.63% <0.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

haodaking commented 1 year ago

Can you take a look at the demo I wrote? The height of the iframe is missing, which is not as expected. thanks

https://codesandbox.io/s/sweet-knuth-3n57ws?file=/index.html

DustinBrett commented 1 year ago

Can you take a look at the demo I wrote? The height of the iframe is missing, which is not as expected. thanks

https://codesandbox.io/s/sweet-knuth-3n57ws?file=/index.html

I'll take a look as I didn't have this specific issue.

DustinBrett commented 1 year ago

Can you take a look at the demo I wrote? The height of the iframe is missing, which is not as expected. thanks

https://codesandbox.io/s/sweet-knuth-3n57ws?file=/index.html

To me this doesn't seem so bad but maybe I am missing something. It did clone the iframe and the image inside, but didn't respect the height of the iframe you are saying? I'm not sure I have a solution for this atm and also I am wondering if this issue is more related to my previous merged PR where iframe cloning began. Could you clarify the expected results and suggestions if you have any? I'm not sure if this PR needs to solve this specific issue unless the code changes caused it.

haodaking commented 1 year ago

To me this doesn't seem so bad but maybe I am missing something. It did clone the iframe and the image inside, but didn't respect the height of the iframe you are saying? I'm not sure I have a solution for this atm and also I am wondering if this issue is more related to my previous merged PR where iframe cloning began. Could you clarify the expected results and suggestions if you have any? I'm not sure if this PR needs to solve this specific issue unless the code changes caused it.

image image image

The solution for #351 PR is to replace the iframe tag with a div tag, set the div as the style of the iframe, and then clone the content in the iframe body

DustinBrett commented 1 year ago

To me this doesn't seem so bad but maybe I am missing something. It did clone the iframe and the image inside, but didn't respect the height of the iframe you are saying? I'm not sure I have a solution for this atm and also I am wondering if this issue is more related to my previous merged PR where iframe cloning began. Could you clarify the expected results and suggestions if you have any? I'm not sure if this PR needs to solve this specific issue unless the code changes caused it.

image image image

The solution for #351 PR is to replace the iframe tag with a div tag, set the div as the style of the iframe, and then clone the content in the iframe body

Ok thanks for clarification. It would be good if the iframe could be cloned without it being a div, unless that is the ideal solution. I'll look into it more.

DustinBrett commented 1 year ago

To me this doesn't seem so bad but maybe I am missing something. It did clone the iframe and the image inside, but didn't respect the height of the iframe you are saying? I'm not sure I have a solution for this atm and also I am wondering if this issue is more related to my previous merged PR where iframe cloning began. Could you clarify the expected results and suggestions if you have any? I'm not sure if this PR needs to solve this specific issue unless the code changes caused it.

image image image

The solution for #351 PR is to replace the iframe tag with a div tag, set the div as the style of the iframe, and then clone the content in the iframe body

I've looked into this and it turns out that although #351 had a solution, it was not because of the div but because of changing the iframe (inline frame) to display as block. After looking at https://stackoverflow.com/a/28954135/5895982 I think it might make sense for this use case. I've applied that change in the latest commit and tested on your codesandbox and it seemed to give the correct result now.

vivcat[bot] commented 1 year ago

:tada: This PR is included in version 1.11.6 :tada:

The release is available on:

Your semantic-release bot :package::rocket: