tsayen / dom-to-image

Generates an image from a DOM node using HTML5 canvas
Other
10.32k stars 1.68k forks source link

Add fix for scroll bar positioning #142

Open cnatis opened 7 years ago

cnatis commented 7 years ago

Fixes https://github.com/tsayen/dom-to-image/issues/121

This is very beta/porotype code as it is, I did test in Chrome 59 with both horizontal and vertical overflows and a combination of both, but I suggest it be tested a bit more, I just don't have the time at the moment to do more rigorous testing.

stryju commented 7 years ago

this doesn't work as expected if children are positioned absolutely in source - you override their position when cloned

cnatis commented 7 years ago

@stryju Yes that is correct, I did note that down somewhere (I think in the readme maybe), but its easily fixed, simply put your positioned items in a container that is not absolutely positioned. Also worst case scenario they decide they dont want the scrollfix (its disabled by default) then they still end up with the same result as before.

If they did want the fix and had a combination of non positioned items and positioned items then that would be the case where they either have to change their markup, or not use the fix.

stryju commented 7 years ago

I think "change your markup" is a wrong way to approach this problem; this functionality is broken - I think you should check if child already has position: absolute, if so - adjust the coordinates accordingly (can be tricky for some edge cases, but doable)

cnatis commented 7 years ago

@stryju Yeah that does make sense, I'll see if I can get that done in the next few weeks, not much time unfortunately but it shouldn't be too hard to offset elements based on their original positions.

cemjotform commented 7 years ago

Thanks for the fix it works without any problem!

phyesix commented 7 years ago

Oh, thank you for quick fix 💯 💯 💯 💯 💯 💯 💯 💯 💯 💯 💯 💯 💯 It works!

EFSO

william-at-cp commented 6 years ago

Hi, how do we pull this version where scrollOffset is fixed from package.json?? The changes are on master and not on the latest release. The latest "2.6.0" does not contain this fix.

cnatis commented 5 years ago

For anyone following, I am going to try and finish this over the next month, there is only the absolute positioned elements to complete and it should be ready

cnatis commented 5 years ago

@tsayen Please review, I have completed the required changes and this now works as expected with a combination of children elements, some position absolute (relative to its parent), and some position static.

I have tested in the latest Chrome.

IDisposable commented 5 years ago

FYI, will be pulling this into https://github.com/1904labs/dom-to-image-more as soon as I can clean up the code a bit. If you want to directly address, let me know @cnatis

cnatis commented 5 years ago

@IDisposable I will be able to address these changes within a week or two if you can wait, it would most likely happen on a weekend around the end of the month.

IDisposable commented 5 years ago

I can certainly wait, thanks for tagging back :)

cnatis commented 5 years ago

Will try and complete this on the weekend

cnatis commented 5 years ago

@IDisposable I have completed the requested changes, I left the HashMap vs Array stuff as is