Skip to content
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

Need to wait for a paint before taking a screenshot #895

Open
jgraham opened this issue Apr 17, 2017 · 8 comments
Open

Need to wait for a paint before taking a screenshot #895

jgraham opened this issue Apr 17, 2017 · 8 comments
Labels
Milestone

Comments

@jgraham
Copy link
Member

jgraham commented Apr 17, 2017

jugglinmike/chrome-screenshot-race#1 details an issue where using ChromeDriver to take screenshots on page load produces unreliable behaviour because it isn't waiting for the content to paint before taking the screenshot. The specification should require webdriver implementations to wait for a paint before taking the screenshot, otherwise the implementation isn't reliable.

cc @jugglinmike

@jgraham jgraham added the bug label Apr 17, 2017
@jgraham jgraham added this to the Level 1 milestone Apr 17, 2017
@gsnedders
Copy link
Member

Related to #893.

@jgraham
Copy link
Member Author

jgraham commented Apr 17, 2017

This is related to #893 but somewhat different because it isn't about resources that aren't loaded by load but about the fact that load firing doesn't guarantee a paint has happened.

@andreastt
Copy link
Member

Would adding the step

Wait until the user agent event loop has spun enough times to process the DOM events generated by the previous step.

from the Element Click command help?

We are somewhat vague about what that means, but in practice, what Marionette does wait for window.requestAnimationFrame to return:

interaction.flushEventLoop = function* (win) {
  let unloadEv;

  return new Promise((resolve, reject) => {
    if (win.closed) {
      reject();
      return;
    }

    unloadEv = reject;
    win.addEventListener("unload", unloadEv, {once: true});

    win.requestAnimationFrame(resolve);
  }).then(() => {
    win.removeEventListener("unload", unloadEv);
  });
};

@jgraham
Copy link
Member Author

jgraham commented Apr 17, 2017

No. In gecko terms we should wait for MozAfterPaint (although I think that rAF is equivalent). But I'm not sure if it's guaranteed that a paint actually happens before that runs. The spec sort of implies that it probably ought to, but maybe in practice it doesn't.

shs96c added a commit to shs96c/webdriver-spec that referenced this issue Aug 9, 2017
shs96c added a commit to shs96c/webdriver-spec that referenced this issue Aug 22, 2017
We couch this in the language of `requestAnimationFrame` so that
we can be sure that there are no pending paint events. This does
not guarantee that all resources on the page are fully laoded.

This also brings the language used in `Take Element Screenshot`
into line with the language used in `Take Screenshot` by using
`trying` instead of checking the return value.

Starts to address w3c#895
AutomatedTester pushed a commit that referenced this issue Aug 23, 2017
We couch this in the language of `requestAnimationFrame` so that
we can be sure that there are no pending paint events. This does
not guarantee that all resources on the page are fully laoded.

This also brings the language used in `Take Element Screenshot`
into line with the language used in `Take Screenshot` by using
`trying` instead of checking the return value.

Starts to address #895
@shs96c
Copy link
Contributor

shs96c commented Aug 23, 2017

We now using the same language as requestAnimationFrame, so there shouldn't (ha!) be pending paint events. Waiting for all content to be loaded (including non-critical ones) is out of scope for Level 1. Pushing to level 2.

@shs96c shs96c modified the milestones: Level 2, Level 1 Aug 23, 2017
jlipps pushed a commit to jlipps/webdriver that referenced this issue Oct 11, 2017
We couch this in the language of `requestAnimationFrame` so that
we can be sure that there are no pending paint events. This does
not guarantee that all resources on the page are fully laoded.

This also brings the language used in `Take Element Screenshot`
into line with the language used in `Take Screenshot` by using
`trying` instead of checking the return value.

Starts to address w3c#895
@foolip
Copy link
Member

foolip commented Mar 7, 2018

@Hexcles and I are discussing how to deal with https://crbug.com/507054 and need to understand the exact timing of Take Screenshot to do that.

In gecko terms we should wait for MozAfterPaint (although I think that rAF is equivalent)

@jgraham, this isn't the case I think. rAF is a before layout/paint callback, as can be seen in https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering. "run the animation frame callbacks" comes before "update the rendering" with no async-y bits in between, so AFAICT the current WebDriver spec guarantees that the screenshot is right before the next paint, if we take "update the rendering" to mean paint. (@chrishtr, is that right?)

The way that "Take Screenshot" is implemented in ChromeDriver is to force a redraw in paint, here:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_impl.cc?l=1615&rcl=f04536cae9016aabeb9e45eff1364b0e90e90838
https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?l=1244&rcl=2faeeb15178698f9bd50db3c334e1db4df03fc10

So... I think that to match this WebDriver would be to, in the "remote end steps", not say "When the user agent is next to run the animation frame callbacks" but rather have a hook after the last step of the "update the rendering" steps in HTML. And perhaps something to cause those steps to run, since they may never run again otherwise.

@andreastt, would that match what's implemented in GeckoDriver and Marionette, or what timing do you have?

@andreastt
Copy link
Member

Gecko doesn’t currently have any synchronisation, i.e. does not conform to WebDriver here.

@foolip
Copy link
Member

foolip commented Mar 8, 2018

Will it just grab whatever was last painted then, so not forcing a new layout or paint? Do you think that's a better behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants