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

html2canvas.Parse() page scrolls to top #57

Closed
imrannaqvi opened this Issue Feb 21, 2012 · 24 comments

Comments

Projects
None yet
@imrannaqvi

imrannaqvi commented Feb 21, 2012

Hi, i am using v0.30. When html2canvas.Parse() is called the page automatically scrolls to top, I have also spotted it in v0.32.

@cobexer

This comment has been minimized.

Show comment
Hide comment
@cobexer

cobexer Feb 21, 2012

Contributor

scrolling is currently to be expected: https://github.com/niklasvh/html2canvas/blob/master/src/Parse.js#L14
...
https://github.com/niklasvh/html2canvas/blob/master/src/Core.js#L38
// TODO add scroll position to bounds, so no scrolling of window necessary
@niklasvh welcomes pull requests with open arms, so have a look and fix it if you can ;)

Contributor

cobexer commented Feb 21, 2012

scrolling is currently to be expected: https://github.com/niklasvh/html2canvas/blob/master/src/Parse.js#L14
...
https://github.com/niklasvh/html2canvas/blob/master/src/Core.js#L38
// TODO add scroll position to bounds, so no scrolling of window necessary
@niklasvh welcomes pull requests with open arms, so have a look and fix it if you can ;)

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Feb 21, 2012

Owner

As @cobexer mentioned, the scrolling is intentional, but removing it would be ideal. It however, isn't as easy as it sounds.

Owner

niklasvh commented Feb 21, 2012

As @cobexer mentioned, the scrolling is intentional, but removing it would be ideal. It however, isn't as easy as it sounds.

@sjhewitt

This comment has been minimized.

Show comment
Hide comment
@sjhewitt

sjhewitt Jul 10, 2012

I know this is an old issue, but I've started trying to implement a fix here:

https://github.com/sjhewitt/html2canvas/tree/no-scroll

I'll open a pull request after I've fully tested it across all browsers

sjhewitt commented Jul 10, 2012

I know this is an old issue, but I've started trying to implement a fix here:

https://github.com/sjhewitt/html2canvas/tree/no-scroll

I'll open a pull request after I've fully tested it across all browsers

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Jul 10, 2012

Owner

Great! One thing that needs to be taken into account is that the user may be scrolling the page when the parse happens, so a single one-time scroll status check won't be enough.

Owner

niklasvh commented Jul 10, 2012

Great! One thing that needs to be taken into account is that the user may be scrolling the page when the parse happens, so a single one-time scroll status check won't be enough.

@sjhewitt

This comment has been minimized.

Show comment
Hide comment
@sjhewitt

sjhewitt Jul 10, 2012

I guess that's a problem in the existing implementation as well then? Is there a test case or steps to replicate documented somewhere?

What do you think about adding an 'onscroll' listener to the window for the duration of the parse phase, and updating the cached scrollInfo object with the latest details when the user scrolls?

sjhewitt commented Jul 10, 2012

I guess that's a problem in the existing implementation as well then? Is there a test case or steps to replicate documented somewhere?

What do you think about adding an 'onscroll' listener to the window for the duration of the parse phase, and updating the cached scrollInfo object with the latest details when the user scrolls?

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Jul 10, 2012

Owner

Well, that's the reason the current implementation is forcing the scroll to the top, since otherwise scrolling may end up placing elements in wrong positions. An onscroll listener may work, but not sure if it updates fast/frequently enough as even a small ms delay will end up messing the page up completely.

Don't have any specific examples to test this, but simple scrolling up and down a page during the screenshot capture used to be enough to see noticeable errors in the positioning of elements.

Owner

niklasvh commented Jul 10, 2012

Well, that's the reason the current implementation is forcing the scroll to the top, since otherwise scrolling may end up placing elements in wrong positions. An onscroll listener may work, but not sure if it updates fast/frequently enough as even a small ms delay will end up messing the page up completely.

Don't have any specific examples to test this, but simple scrolling up and down a page during the screenshot capture used to be enough to see noticeable errors in the positioning of elements.

@sjhewitt

This comment has been minimized.

Show comment
Hide comment
@sjhewitt

sjhewitt Jul 10, 2012

What are the asynchronous points during the parse/render process where the user could scroll the window? If all of the position calculations are done without releasing control back to the UI thread, then it's safe to just calculate the offset once.

As I see it, getting the scroll info isn't needed on every getBounds call. Instead, to keep processing time down to a minimum, the info should be updated either when the user scrolls via a scroll listener, or whenever the async callbacks are called.

sjhewitt commented Jul 10, 2012

What are the asynchronous points during the parse/render process where the user could scroll the window? If all of the position calculations are done without releasing control back to the UI thread, then it's safe to just calculate the offset once.

As I see it, getting the scroll info isn't needed on every getBounds call. Instead, to keep processing time down to a minimum, the info should be updated either when the user scrolls via a scroll listener, or whenever the async callbacks are called.

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Jul 10, 2012

Owner

Parsing is done fully synchronously (apart from image handling) at the moment, although that hopefully will change in the future (both parsing and rendering would ideally be asynchronous).

Your point seems very valid, and I don't see why it wouldn't work. I can't recall whether the parsing was synchronous when I last attempted to fix this, or whether I just missed something in the bound calculation which resulted in the images being skewed with scrolling.

Owner

niklasvh commented Jul 10, 2012

Parsing is done fully synchronously (apart from image handling) at the moment, although that hopefully will change in the future (both parsing and rendering would ideally be asynchronous).

Your point seems very valid, and I don't see why it wouldn't work. I can't recall whether the parsing was synchronous when I last attempted to fix this, or whether I just missed something in the bound calculation which resulted in the images being skewed with scrolling.

@sjhewitt

This comment has been minimized.

Show comment
Hide comment
@sjhewitt

sjhewitt Jul 10, 2012

I'll see if I can add some tests in the pull request that will cover the issue so it doesn't break in the future

sjhewitt commented Jul 10, 2012

I'll see if I can add some tests in the pull request that will cover the issue so it doesn't break in the future

@ajbkr

This comment has been minimized.

Show comment
Hide comment
@ajbkr

ajbkr Aug 30, 2012

I added a quick and dirty temporary fix to my codebase for this. I just saved the original pageXOffset and pageYOffset before the call to window.scroll(), and then inserted a call to window.scroll(), specifying the original values just prior to the Parse() method returning.

ajbkr commented Aug 30, 2012

I added a quick and dirty temporary fix to my codebase for this. I just saved the original pageXOffset and pageYOffset before the call to window.scroll(), and then inserted a call to window.scroll(), specifying the original values just prior to the Parse() method returning.

@comp615

This comment has been minimized.

Show comment
Hide comment
@comp615

comp615 Jan 2, 2013

Any updates on this? Would love to see this fix as it can be very inconvenient in long pages. Let me know if I can help test anything!

comp615 commented Jan 2, 2013

Any updates on this? Would love to see this fix as it can be very inconvenient in long pages. Let me know if I can help test anything!

@louisrli

This comment has been minimized.

Show comment
Hide comment
@louisrli

louisrli Apr 18, 2013

Just wanted to write that I've run into this, too. Going to give Andrew's fix a try.

louisrli commented Apr 18, 2013

Just wanted to write that I've run into this, too. Going to give Andrew's fix a try.

@InfernoZeus

This comment has been minimized.

Show comment
Hide comment
@InfernoZeus

InfernoZeus Jul 16, 2013

For anyone who still has this issue, I've uploaded my patch to a Gist here: https://gist.github.com/InfernoZeus/6010328#file-html2canvas-scroll-patch-diff

InfernoZeus commented Jul 16, 2013

For anyone who still has this issue, I've uploaded my patch to a Gist here: https://gist.github.com/InfernoZeus/6010328#file-html2canvas-scroll-patch-diff

@rhys-e

This comment has been minimized.

Show comment
Hide comment
@rhys-e

rhys-e Jan 23, 2014

In terms of scrolling causing issues when rendering to the canvas - could someone fill me in on why that is exactly? Would it only be a problem on sites where scrolling explicitly causes the DOM to change or is it something more subtle than that? Or is it something to do with the position of elements changing relative to the viewport mid-render? Thanks

rhys-e commented Jan 23, 2014

In terms of scrolling causing issues when rendering to the canvas - could someone fill me in on why that is exactly? Would it only be a problem on sites where scrolling explicitly causes the DOM to change or is it something more subtle than that? Or is it something to do with the position of elements changing relative to the viewport mid-render? Thanks

@brcontainer

This comment has been minimized.

Show comment
Hide comment
@brcontainer

brcontainer Jan 23, 2014

Contributor

@rhys-e if 0.4.1 version, provide an example of the problem, using an online link or http://jsfiddle.net

Contributor

brcontainer commented Jan 23, 2014

@rhys-e if 0.4.1 version, provide an example of the problem, using an online link or http://jsfiddle.net

@InfernoZeus

This comment has been minimized.

Show comment
Hide comment
@InfernoZeus

InfernoZeus Jan 23, 2014

I don't think @rhys-e has an example, which is why he's asking someone to fill him in on what the problem exactly is...

InfernoZeus commented Jan 23, 2014

I don't think @rhys-e has an example, which is why he's asking someone to fill him in on what the problem exactly is...

@brcontainer

This comment has been minimized.

Show comment
Hide comment
@brcontainer

brcontainer Jan 23, 2014

Contributor

@InfernoZeus I agree with you, I misread. Thanks!

Contributor

brcontainer commented Jan 23, 2014

@InfernoZeus I agree with you, I misread. Thanks!

@chrisinajar

This comment has been minimized.

Show comment
Hide comment
@chrisinajar

chrisinajar Mar 19, 2014

Just wanted to say that InfernoZeus's fix still works great.

Also, in response to the worries of async execution: Scroll handler is not the way to go. You can't scroll while the javascript thread is blocked. This means you only ever need to worry about scrolling between asyc steps, never during a function's execution. Additionally, the scrolling positions do not visually update until the javascript thread goes back to the event loop.

window.scrollTo(any, value); // data
window.scrollTo(any, value); // is
window.scrollTo(any, value); // updated
window.scrollTo(any, value); // instantly
window.scrollTo(any, value); // but
window.scrollTo(any, value); // only this one will visually appear to the user

So lets abuse that.

Scroll at the start and end of every async step. Create a prepare and a finalize functions that run before/after each part. All they need to do is store the current scroll position, process things synchronously, then scroll back to where it was and queue the next async step. This could even be built into the internal async control flow model.

Assuming the async chunks are broken up very very very well you should be able to scroll smoothly while it's parsing without worry of data corruption or flickering.

chrisinajar commented Mar 19, 2014

Just wanted to say that InfernoZeus's fix still works great.

Also, in response to the worries of async execution: Scroll handler is not the way to go. You can't scroll while the javascript thread is blocked. This means you only ever need to worry about scrolling between asyc steps, never during a function's execution. Additionally, the scrolling positions do not visually update until the javascript thread goes back to the event loop.

window.scrollTo(any, value); // data
window.scrollTo(any, value); // is
window.scrollTo(any, value); // updated
window.scrollTo(any, value); // instantly
window.scrollTo(any, value); // but
window.scrollTo(any, value); // only this one will visually appear to the user

So lets abuse that.

Scroll at the start and end of every async step. Create a prepare and a finalize functions that run before/after each part. All they need to do is store the current scroll position, process things synchronously, then scroll back to where it was and queue the next async step. This could even be built into the internal async control flow model.

Assuming the async chunks are broken up very very very well you should be able to scroll smoothly while it's parsing without worry of data corruption or flickering.

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Mar 19, 2014

Owner

This is resolved in 0.5, where the document is cloned into its own context where any scrolling done won't affect the main view, which allows it to work async as well.

Owner

niklasvh commented Mar 19, 2014

This is resolved in 0.5, where the document is cloned into its own context where any scrolling done won't affect the main view, which allows it to work async as well.

@niklasvh niklasvh closed this Mar 19, 2014

@bitflower

This comment has been minimized.

Show comment
Hide comment
@bitflower

bitflower Jul 31, 2014

When will 0.5 be published?

bitflower commented Jul 31, 2014

When will 0.5 be published?

@cvsguimaraes

This comment has been minimized.

Show comment
Hide comment
@cvsguimaraes

cvsguimaraes Nov 25, 2014

+1

Why close the issue if the fix is not tested yet?

cvsguimaraes commented Nov 25, 2014

+1

Why close the issue if the fix is not tested yet?

@niklasvh

This comment has been minimized.

Show comment
Hide comment
@niklasvh

niklasvh Nov 25, 2014

Owner

It is fixed and tested

Owner

niklasvh commented Nov 25, 2014

It is fixed and tested

@cvsguimaraes

This comment has been minimized.

Show comment
Hide comment
@cvsguimaraes

cvsguimaraes Nov 25, 2014

@niklasvh My bad, I've downloaded from the releases page...
Thank you sir!

cvsguimaraes commented Nov 25, 2014

@niklasvh My bad, I've downloaded from the releases page...
Thank you sir!

@derekm

This comment has been minimized.

Show comment
Hide comment
@derekm

derekm Aug 28, 2015

The fix in 0.5-alpha may be tested, but it isn't well-tested and it isn't fool-proof. I'm attaching two screenshoots, one from the latest stable release and another from the latest alpha release.

I'm using Highcharts, Gridstack, and canvg.

Latest stable release:

dashboard_thumb_69_1440783278

Latest alpha release:

dashboard_thumb_69_1440781895

derekm commented Aug 28, 2015

The fix in 0.5-alpha may be tested, but it isn't well-tested and it isn't fool-proof. I'm attaching two screenshoots, one from the latest stable release and another from the latest alpha release.

I'm using Highcharts, Gridstack, and canvg.

Latest stable release:

dashboard_thumb_69_1440783278

Latest alpha release:

dashboard_thumb_69_1440781895

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment