Height of snapshot is wrong when using images with dynamic height

trotzig commented 7 years ago

We compute a rectangle to crop out the relevant parts from the full-screen screenshot that Selenium produces. This rectangle is computed immediately after the component is rendered. The screenshot will wait for images to load. If images don't have a set height (and in some cases, width) the snapshot will be cropped according to the original rectangle (before image was loaded).

Here's an example: http://s3.amazonaws.com/diffux_ci-diffs/585a9297-0df1-44ab-847a-8a5fbc70d72b/index.html

In a way, this is good to know about a component. Because users are most likely experiencing a bouncy page load. But I think happo should do better here.

lencioni commented 7 years ago

Is that an <img> or a background image? If it is an <img> I've recently done something in a project to make Happo wait. I'm planning on moving it here soon, which would fix this issue for you.

If it is a background image, I think that the fix will be either much harder (maybe impossible), more expensive (make Happo runs slow), or require configuration per component that needs it.

trotzig commented 7 years ago

To fix this, I see two options:

add code in happo-runner.js that will wait for all external resources. We can use stuff from here as inspiration: https://github.com/diffux/diffux-core/blob/master/lib/diffux_core/script/take-snapshot.js
compute the rectangle after taking the screenshot

Since Selenium is already waiting for resources, the second option is probably simpler.

trotzig commented 7 years ago

It's a regular <img>.

trotzig commented 7 years ago

@lencioni do you see any issues with making a secondary call to get the rect?

lencioni commented 7 years ago

You might take the screenshot before the image has loaded, which is actually the problem I am running into. We need to wait before taking the screenshot, so we might as well get the rectangle first to avoid having to bounce back and forth.

I forgot about being able to look at when resources are requested and whatnot via webdriver. I think that would make this work well even for bg images.

trotzig commented 7 years ago

You might take the screenshot before the image has loaded

From my experience, webdriver/Selenium will always wait for images. Can you share a bit about when that doesn't apply?

lencioni commented 7 years ago

We've had a bunch of spurious diffs of <img> tags that have data-uris for their images that weren't fully rendered when the screenshot was taken. I think that perhaps the resource was downloaded (because it was inlined in the JS), but the browser didn't have time to actually render it.

trotzig commented 7 years ago

Interesting. It definitely waits for images loaded through an external URL in src. So data-uris might be something to look into.

I can't seem to find how to tell webdriver to wait for resources. It could be that this is baked into the screenshotting function. Or it might even be up to the browser itself.

lencioni commented 7 years ago

In the meantime, here are the functions we are using in all of our Happo tests to wait for images to render:

function waitForImageToLoad(url) {
  return new Promise((resolve, reject) => {
    const img = new Image();
    img.onerror = reject;
    img.onload = resolve;
    img.src = url;
  });
}

function waitForImagesToRender() {
  return new Promise((resolve) => {
    const promises = Array.from(document.querySelectorAll('img'))
      .map(img => img.src)
      .filter(Boolean)
      .map(url => waitForImageToLoad(url));

    if (promises.length === 0) {
      // There are no images to wait for, so we can just resolve right away.
      resolve();
    }

    Promise.all(promises).then(() => {
      // Now that the images have loaded, we need to wait for a couple of
      // animation frames to go by before we think they will have finished
      // rendering.
      requestAnimationFrame(() => {
        // Start render
        requestAnimationFrame(() => {
          // Finish rendering
          resolve();
        });
      });
    });
  });
}

Which we use by:

waitForImagesToRender().then(done);

trotzig commented 7 years ago

I've been looking into how Selenium does this through its screenshot_as method. If I'm not mistaken, they the drawWindow to paint the window onto a canvas, then feed the contents of that canvas back. https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawWindow

I believe this is the method used: https://github.com/SeleniumHQ/selenium/blob/ceaf3da79542024becdda5953059dfbb96fb3a90/javascript/firefox-driver/js/screenshooter.js#L23

I was drawn here because I was looking for any code that waits for resources. I can't find any of that however, so I don't understand how that works. Evidently, you and I are experiencing different things @lencioni. You have to preload images, we don't. 😕

The link I sent to the way we did this in diffux-core isn't really relevant because we're using PhantomJS internals (page.onResourceRequested). Your code that waits for images might be better, although (as you said) not catching background-images, fonts etc.

After looking at the docs for context.drawWindow some more, I wonder if we can circumvent Selenium a little? If I'm reading the docs correctly, you can pass a rectangle of things to draw. Since we already have the rect computed we can use it when snapshotting. That should speed up runs by a good deal I think, because right now we always take a full-screen snapshot that we then crop in ruby land.

lencioni commented 7 years ago

You have to preload images, we don't.

FWIW, I think it is less about waiting for the image to download, and more about waiting for it to render (since we are seeing this with inline data-uri images). For our use case, we could probably simplify my code to avoid the new Image stuff and just do the pair of requestAnimationFrames.

If I'm reading the docs correctly, you can pass a rectangle of things to draw.

Oh, this is exactly what I was looking for when digging into #62. Good find! This will definitely speed up runs, especially on larger viewport sizes.

However, it looks like it will only work in Firefox. We'd like to expand Happo to support more browsers (#73), so I am worried that this will make that harder. I poked around the selenium repo a bit and couldn't find similar methods for other drivers.

trotzig commented 7 years ago

I agree about wanting to expand. Making this type of change would lock us with ff a little deeper. But I think it could be worth it anyway.

On Saturday, 8 October 2016, Joe Lencioni notifications@github.com wrote:

You have to preload images, we don't.

FWIW, I think it is less about waiting for the image to download, and more about waiting for it to render (since we are seeing this with inline data-uri images). For our use case, we could probably simplify my code to avoid the new Image stuff and just do the pair of requestAnimationFrames.

If I'm reading the docs correctly, you can pass a rectangle of things to draw.

Oh, this is exactly what I was looking for when digging into #62 https://github.com/Galooshi/happo/issues/62. Good find! This will definitely speed up runs, especially on larger viewport sizes.

However, it looks like it will only work in Firefox. We'd like to expand Happo to support more browsers (#73 https://github.com/Galooshi/happo/issues/73), so I am worried that this will make that harder. I poked around the selenium repo a bit and couldn't find similar methods for other drivers.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Galooshi/happo/issues/162#issuecomment-252425159, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjS5ahQnIHVAjRSiVl4IFilg0xNmMhgks5qx52cgaJpZM4KN9SO .

lencioni commented 7 years ago

We could fall back to a more supported method.

On Sat, Oct 8, 2016, 9:31 AM Henric Trotzig notifications@github.com wrote:

I agree about wanting to expand. Making this type of change would lock us with ff a little deeper. But I think it could be worth it anyway.

On Saturday, 8 October 2016, Joe Lencioni notifications@github.com wrote:

You have to preload images, we don't.

FWIW, I think it is less about waiting for the image to download, and more about waiting for it to render (since we are seeing this with inline data-uri images). For our use case, we could probably simplify my code to avoid the new Image stuff and just do the pair of requestAnimationFrames.

If I'm reading the docs correctly, you can pass a rectangle of things to draw.

Oh, this is exactly what I was looking for when digging into #62 https://github.com/Galooshi/happo/issues/62. Good find! This will definitely speed up runs, especially on larger viewport sizes.

However, it looks like it will only work in Firefox. We'd like to expand Happo to support more browsers (#73 https://github.com/Galooshi/happo/issues/73), so I am worried that this will make that harder. I poked around the selenium repo a bit and couldn't find similar methods for other drivers.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Galooshi/happo/issues/162#issuecomment-252425159, or mute the thread < https://github.com/notifications/unsubscribe-auth/AAjS5ahQnIHVAjRSiVl4IFilg0xNmMhgks5qx52cgaJpZM4KN9SO

.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/Galooshi/happo/issues/162#issuecomment-252434242, or mute the thread https://github.com/notifications/unsubscribe-auth/AAL7zrAJQcoaaSFclcNDjeLMR8_OdAB1ks5qx8VXgaJpZM4KN9SO .

trotzig commented 7 years ago

I guess that would make sense. Although that would likely keep the process a little too complicated for our current use-case. Without having to worry about other browsers (at this point) we would just make renderExample return the image directly. That will simplify both the client js code and the "backend" ruby code.

If we want to fully support Chrome at this point for instance, we'd have to come up with a strategy to work around their limitation of only screenshotting the visible portion of the window. A possible solution would be to scroll the page, take screenshots as we go along and then stitch everything together in the end.

trotzig commented 7 years ago

Okay, I tried the ctx.drawWindow function, and I believe it was added in a fairly recent Firefox version. It's not working in 47.0.1, which is the latest version I know we can run with Selenium. 😞

trotzig commented 7 years ago

I was wrong about when it was added. ctx.drawWindow has been in Firefox for a while, it's just that you need special permissions to execute it. Selenium has that, but it wraps all script execution in a sandbox so that you can't abuse those permissions. As I see it, the only way to get a pre-cropped screenshot would be to make a PR to Selenium.

This is where the sandbox for script execution is created: https://github.com/SeleniumHQ/selenium/blob/ceaf3da79542024becdda5953059dfbb96fb3a90/javascript/firefox-driver/js/firefoxDriver.js#L240

This is where Selenium's Firefox driver takes a screenshot: https://github.com/SeleniumHQ/selenium/blob/ceaf3da79542024becdda5953059dfbb96fb3a90/javascript/firefox-driver/js/firefoxDriver.js#L896

If we dig deeper, we end up here (where the actual canvas is created and drawn to): https://github.com/SeleniumHQ/selenium/blob/ceaf3da79542024becdda5953059dfbb96fb3a90/javascript/firefox-driver/js/screenshooter.js

Galooshi / happo

Height of snapshot is wrong when using images with dynamic height #162