w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
368 stars 41 forks source link

`browsingContext.captureScreenshot`: restrict to top-level contexts only? #441

Closed thiagowfx closed 1 week ago

thiagowfx commented 1 year ago

For https://w3c.github.io/webdriver-bidi/#command-browsingContext-captureScreenshot:

Should we make the spec restrict capturing screenshots only in top-level contexts?

Rationale: Basically, CDP does not properly support it (confirmed with @OrKoN and @sadym-chromium), as rendering is a super optimised process.

Filing as per @sadym-chromium's guidance.

jimevans commented 1 year ago

@thiagowfx One thing WebDriver Classic has that BiDi doesn't (yet), is taking a screenshot of an element, which seems like it might fall into the same category here. If we want to be able to implement classic over bidi, that seems like a use case we need to support.

thiagowfx commented 1 year ago

Side note: Element screenshots is tracked by https://github.com/w3c/webdriver-bidi/issues/229

whimboo commented 1 year ago

In classic you will not have the possibility to take a screenshot of a huge iframe which has overflow:auto set and most of its contents is hidden and only accessible by scroll bars.

I feel this is an important feature to have and as such I think that we should support taking screenshots from iframes and even full screenshots. We should not judge on features based on the fact that some browsers might not have support for it yet. Please note that for some of the features in BiDi we have to write most of the code from scratch given that it doesn't exist at all. And in some cases we know that all possible scenarios cannot be supported immediately.

jrandolf-2 commented 1 year ago

@whimboo Are you saying that with BiDi, you want to allow for support of huge iframes where most of its content can be hidden?

whimboo commented 1 year ago

Consider a fullpage screenshot for just an iframe's content. How would you do that when you can only screenshot from the top-level browsing context?

jrandolf-2 commented 1 year ago

Considering CDP's implementation, the largest image you can capture of an iframe is the iframe itself. For example, if a document has a 300x500-sized iframe, the largest image you can capture is 300x500.

The reason we want to capture top-level is because capturing the contents of an oversized element in iframe is just not found in practice. If you have an iframe, then it's likely that

If the iframe is cross-origin, then the user can also just open a new page with that iframe (which will now be a top-level context) and capture it.

css-meeting-bot commented 3 weeks ago

The Browser Testing and Tools Working Group just discussed BrowsingContext.captureScreenshot: restrict to top-level contexts only.

The full IRC log of that discussion <simonstewart> Topic: BrowsingContext.captureScreenshot: restrict to top-level contexts only
<simonstewart> github: https://github.com/w3c/webdriver-bidi/issues/441
<simonstewart> sadym: in bidi you can capture a screenshot, and you can capture a screenshot of an iframe beyond its viewport. It's not feasible to implement in chrome
<simonstewart> sadym: our proposal is to keep the standard implementable.
<jgraham> q+
<simonstewart> sadym: we suggest restricting screenshots beyond viewports to the top-level browsing context
<gsnedders> q? to ask if we know of any use-cases for entire viewport screenshots within a frame
<simonstewart> jgraham: gecko can implement this. You're allowed to return an unsupported operation if an implementation can't do it
<simonstewart> jgraham: this doesn't seem like an important enough point of difference to say "if one browser can't do it, no-one can do it"
<gsnedders> q+ to ask if we know of any use-cases for entire viewport screenshots within a frame
<gsnedders> ack jgraham
<simonstewart> jgraham: people usually take screenshots for informative purposes
<simonstewart> q+
<simonstewart> jimevans: do we need a spec change to allow people to not return a result?
<simonstewart> jgraham: yes
<AutomatedTester> ack gsnedders
<Zakim> gsnedders, you wanted to ask if we know of any use-cases for entire viewport screenshots within a frame
<simonstewart> jgraham: no it doesnt need a spec change
<simonstewart> ack next
<simonstewart> gsnedders: do we know of any use case of people wanting to take a screenshot of the viewport of a frame
<jgraham> simonstewart: We used to get bug reports in selenium
<jgraham> simonstewart: People don't generally know the distinction between the top level viewport and an iframe
<jgraham> simonstewart: Can Chrome implement print for an iframe?
<jimevans> q+
<jgraham> sadym: I don't think so.
<AutomatedTester> ack jimevans
<simonstewart> jimevans: with respect to screenshots, the most common requested case for full-page screenshots is at the top level
<simonstewart> jimevans: and if that page contains an iframe, typically speaking, the iframe has a size associated with it.
<jgraham> q+
<simonstewart> jimevans: what you get in the screenshot is whatever is currently displayed in the iframe, bounded by the size of the iframe's viewport
<simonstewart> jimevans: someone asking for a screenshot of the entire content of an iframe is an unusual occurrence, even from the selenium context
<simonstewart> jimevans: I think it's okay that if the browsing context of an iframe is given to the snapshot command, it's okay to return an unsupported operation exception (because I think it's a rare edge case)
<AutomatedTester> ack jgraham
<simonstewart> jgraham: if some implementations can support it and some can't, this doesn't seem like a major interop issue
<simonstewart> sadym: OK. Sounds fine
<simonstewart> AutomatedTester: so long as we have the viewport in all cases, that's fine. Just thinking of the visual testing cases
whimboo commented 3 weeks ago

@sadym-chromium, reading through the notes from Friday I assume that we can close this issue? As @jgraham pointed out no spec change is necessary. Step 3 of the remote end steps cover that already.

sadym-chromium commented 2 weeks ago

@sadym-chromium, reading through the notes from Friday I assume that we can close this issue? As @jgraham pointed out no spec change is necessary. Step 3 of the remote end steps cover that already.

right. One thing discussed but was not minuted though: we agreed on marking the related WPT test as optional.