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

Unable to take a screenshot of a fully loaded page #893

Open
gsnedders opened this issue Apr 10, 2017 · 18 comments
Open

Unable to take a screenshot of a fully loaded page #893

gsnedders opened this issue Apr 10, 2017 · 18 comments
Milestone

Comments

@gsnedders
Copy link
Member

gsnedders commented Apr 10, 2017

At the moment, with WebDriver, you can trivially wait until after the load event has fired before taking a screenshot. However, not everything delays the load event (see HTML's notion of "critical subresources" and "delays the load event"). Anything you can detect whether or not it has loaded using the DOM can equally be dealt with through the script APIs.

However, CSS is especially awkward because of w3c/csswg-drafts#1088. At the moment, Edge doesn't block the load event on background-image and list-style-image and these are undetectable through the DOM.

We need some proper solution to this problem in a general sense. As a web developer, to do any sort of visual testing, you want to be able to take a screenshot of a page after everything has loaded (of course, this can get complicated by long polling XHR, WebSockets, etc.).

I think there's at least a couple of options options:

  • Convince everyone (WGs, implementers) that the page should be in a stable state after the load event has fired
  • Add some custom notion to WebDriver of loadedness (OperaDriver has something like this with opera.idle)

Notably, this is causing problems for wptrunner running reftests, as we need some way to take a screenshot after everything has loaded. (web-platform-tests/wpt#5412)

@shs96c
Copy link
Contributor

shs96c commented Apr 10, 2017

I realise that your first solution was probably flippant, and your second the desired outcome, but I feel the first is the correct solution.

The problem is that for anyone to actually implement the second suggestion, the first must be true. Worse, as new standards emerge, someone would have to be responsible for making sure that the load event actually meant loaded. This would either mean endless minor updates to this spec (through the w3c approval process to get to Recommendation) or the relevant specs should include language to indicate how they influence the load event.

From the linked WPT bug, it seems like at least some browser vendors have considered this and are treating all "remote loads" as impacting the load event (which seems sensible to me), and from w3c/csswg-drafts#1088 it sounds like the CSS folks are considering the matter too, if I read @fantasai's comment correctly.

Closing this here, but please feel free to reopen if you feel that more discussion is necessary or more reasoning is lax.

@shs96c shs96c closed this as completed Apr 10, 2017
@gsnedders
Copy link
Member Author

@shs96c I'd on the whole agree the first solution is probably the correct solution!

Realistically, I filed this bug primarily because this is something that needs to be doable through WebDriver and currently isn't, and a few of us decided a bug on the WebDriver spec was probably an appropriate thing for it, even though the correct solution lies elsewhere, especially when this has been an open issue in CSS for over five years. (It is, prima-facie, a limitation of WebDriver today, after all.)

@shs96c
Copy link
Contributor

shs96c commented Apr 12, 2017

Technically, we're waiting for the underlying page to reach a document readiness state of complete. That's dependent on the other specs correctly interacting with how this value is changed. There are issues in other specs that webdriver is exposing --- this is a Good Thing, especially since we always claimed that one of the audiences for this spec are spec authors :)

@jgraham
Copy link
Member

jgraham commented Jun 30, 2017

I don't really understand the discussion on this bug.

There's no technical reason that the load event has to be the only relevant thing. In fact I don't think there's any browser where that's entirely true; @gsnedders has claimed that it's true for Firefox, but in implementing the reftest harness for Firefox I have to explicitly flush a layout and then check if there are any MozPaint events pending before taking the screenshots. I think this might work for WebDriver just because the IPC overhead is large enough that we can't observe the pre-paint state, but there's certainly no spec I'm aware of that claims that the load event also means that a layout and paint occured, even before we consider whether web fonts must be loaded after load fires &c.

It seems entirely reasonable for WebDriver spec to either state that taking screenshots in particular must wait until a paint has happened and webfonts have loaded and whatever else, or to have another load strategy that waits for these things.

@jgraham jgraham reopened this Jun 30, 2017
@gsnedders
Copy link
Member Author

So I think there's a few things here, thinking about this some more and talking with @jgraham:

  • There needs to be some way to take a screenshot after the load event has fired.

    Notably, the current approach of:

    response = driver.execute_async_script("""
      const done = arguments[0];
      document.onload = function(){
        done();
      });
    """)
    driver.save_screenshot("foo.png")
    

    doesn't suffice because I don't think it's currently defined that the screenshot command will only be executed after the load event has completed firing (i.e., as opposed to merely after the listener that called done was invoked). We probably therefore want to tie screenshots (maybe all WebDriver commands?) to the browsing context event loop.

  • As @jgraham pointed out in the previous comment, we need some way to guarantee a style/layout/paint has occurred before the screenshot is taken. For the load case, we can't just guarantee that scripts have completed executing, we need to guarantee everything that follows that (style/etc.) has completed. It's probably reasonable to say that we want to guarantee that style/etc. has happened before all screenshots, given wanting to take a screenshot after some DOM event is probably the common use of screenshots.

@gsnedders
Copy link
Member Author

Oh, and:

  • In the hypothetical future, we probably want it to be possible to take screenshots before page load. e.g., one can imagine as part of performance work wanting to take a screenshot of first paint and another after page load to guarantee above the fold content is painted identically initially

@andreastt
Copy link
Member

andreastt commented Jul 1, 2017 via email

@shs96c
Copy link
Contributor

shs96c commented Jul 3, 2017

Perhaps we should modify the behaviour of the normal page loading strategy to indicate that not only should we wait for the load event, but also at least one paint, and the "main thread" is idle. Not sure waiting for web fonts to load is a Good Idea --- I've never had a great experience with those loading, and it'll ruin test performance.

Attempting time-sensitive operations in an environment without real-time guarantees seems inherently racey and prone to failure. At the moment, WebDriver implementations can effectively be observers of page loading. With the change proposed in #893 (comment) we'd need to be able to pause loading long enough to take that screenshot.

@jgraham
Copy link
Member

jgraham commented Jul 3, 2017

I agree that the "screenshot on first paint" case doesn't match the current WebDriver design.

Fonts are arguably supposed to block the load event anyway, although I think browser behaviour differs here. In any case it should certainly be possible to wait for them to load since it's needed for some use cases. This is incidentially why I didn't suggest modifying the existing load strategies.

@shs96c shs96c modified the milestone: Level 1 Jul 8, 2017
@andreastt
Copy link
Member

andreastt commented Jul 28, 2017 via email

@whimboo
Copy link
Contributor

whimboo commented Aug 3, 2017

Does this only affect screenshots or also other webdriver commands?

I'm asking because there can also be other ongoing requests eg. triggered via AJAX which cause some kind of page load activity. If we only care about page load status here, we would miss those changes and content which gets dynamically added would not be present in the screenshot.

So if this problem is screenshot only maybe the command itself should ensure that the main thread is idle and such? That would guarantee that screenshots would always be accurate (as best as possible).

@shs96c
Copy link
Contributor

shs96c commented Aug 3, 2017

@whimboo, if there's AJAX calls going on, then the test author should make use of something like Selenium's Wait helpers to ensure that those calls have finished.

@whimboo
Copy link
Contributor

whimboo commented Aug 3, 2017

What about other dynamic changes which do not involve AJAX but only JS, and which results in some items added to the main thread? Do we want to force users to always use Wait in those cases?

@shs96c
Copy link
Contributor

shs96c commented Aug 4, 2017 via email

shs96c added a commit to shs96c/webdriver-spec that referenced this issue Aug 9, 2017
This waits for all resources of a page to have finished
loading, and not just for the readyState to reach
"complete".

Addresses w3c#893.
@shs96c shs96c modified the milestones: Level 2, Level 1 Aug 23, 2017
@shs96c
Copy link
Contributor

shs96c commented Aug 23, 2017

This is waiting on the "conservative" page loading strategy, so punted to Level 2.

@SetTrend
Copy link

SetTrend commented Aug 5, 2018

IMHO, this is something that cannot be computed, because there is no "moment of final load". Think of rotating ads or carussels. With these, the page will never be finally loaded. See this example:

@jgraham
Copy link
Member

jgraham commented Aug 5, 2018

Sure, it's widely understood that pages may never reach a static render state.

The point of this issue is that these exist some subresources that are loaded as part of the normal document loading pipeline, but don't block the load event. These don't neccessarily expose an API that allows one to determine when they have been loaded. It is perfectly reasonable to want to take a screenshot after fonts and other CSS subresources have loaded, without wanting to solve the problem that pages may never reach a stable state.

@SetTrend
Copy link

SetTrend commented Aug 5, 2018

Understood. Pardon me for interfering.

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

No branches or pull requests

6 participants