folletto / Blipshot

Google Chrome Extension to make screenshots
intenseminimalism.com
115 stars 19 forks source link

Full page screenshot not working #17

Closed moeloubani closed 7 years ago

moeloubani commented 7 years ago

Seems to be capturing only the visible part of the tab, noticed on line 60 in screenshotter.DOM.js it says there's a bug where it doesn't screenshot correctly, is this what you meant by that? I could swear this was working a little while ago

moeloubani commented 7 years ago

Found what it was see here: https://stackoverflow.com/questions/45061901/chrome-61-body-doesnt-scroll and here https://bugs.chromium.org/p/chromium/issues/detail?id=157855

Updated screenshotScroll function to look like this:

function screenshotScroll(shared) {

    var scrollNode = document.scrollingElement || document.documentElement;
    var scrollTopCurrent = scrollNode.scrollTop;

    //TODO: bug: doesn't screenshot correctly
    scrollNode.scrollTop += window.innerHeight; // scroll!

    if (scrollNode.scrollTop == scrollTopCurrent || scrollNode.scrollTop > 32766) { // 32766 --> Skia / Chrome Canvas Limitation, see recursiveImageMerge()
      // END ||
      shared.imageDirtyCutAt = scrollTopCurrent % window.innerHeight;
      //console.log(scrollTopCurrent + " % " + window.innerHeight + " = " + shared.imageDirtyCutAt);
      scrollNode.scrollTop = shared.originalScrollTop; // <-[] restore user scrollTop
      screenshotEnd(shared);
    } else {
      // LOOP >>
      setTimeout(function() { screenshotVisibleArea(shared); }, 100);
    }
  }

And that did the trick! Great extension you have here by the way! :)

folletto commented 7 years ago

Great extension you have here by the way! :)

Thank you!

says there's a bug where it doesn't screenshot correctly

Yea, it's a tricky play of scrollbars and scrolling. Would be solved if Chrome implemented native screenshotting, but well. I should probably just remove that todo. :P

Found what it was [...]

Interesting... when you said it worked a while ago I thought it was a reference at this recent change: fdf18a0f13012b12e10ec519d92b53103e62eadb, but seems not...

So if I get correctly your changes:

  1. Uses scrollingElement which is more reliable (with a fallback)
  2. No changes to the calculations

I tested your changes and they seem good!

Would you like to push a PR yourself or are you ok if I just do the change myself? :)

moeloubani commented 7 years ago

:) Good to hear! I'm completely okay with you making the change yourself. Thanks again for the extension!

skillwizard commented 7 years ago

Do I have to do anything to make it work? Restart?

folletto commented 7 years ago

Ok the issue is fixed in 1.2.2 and has been pushed to the Chrome Store. It should be automatically updated by Chrome in the next hours (I'm not sure how long it's going to take exactly).

folletto commented 7 years ago

Do I have to do anything to make it work? Restart?

You can try to manually update to 1.2.2 opening Extensions and then clicking on "Update Extensions Now".

If it updates, reloading the page you want to screenshot (if it was already open) is enough and should immediately work.