w3c / webdriver-bidi

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

Update handling of "devicePixelRatio" argument in "browsingContext.setViewport" command. #693

Closed lutien closed 2 months ago

lutien commented 3 months ago

Since the implementation of changing the CSS pixels is problematic for at least Chrome and Firefox (probably Safari as well?) and likely not required, the suggestion here is to rather emulate certain parts, e.g. image source set selection and media queries.


Preview | Diff

lutien commented 2 months ago

@OrKoN, @sadym-chromium, could you take a look if that's somewhat similar to what CDP is doing @gsnedders, could you also assess if that's something Safari could potentially implement

css-meeting-bot commented 2 months ago

The Browser Testing and Tools Working Group just discussed Update handling of "devicePixelRatio" argument in "browsingContext.setViewport" command.

The full IRC log of that discussion <AutomatedTester_> topic: Update handling of "devicePixelRatio" argument in "browsingContext.setViewport" command
<AutomatedTester_> github: https://github.com/w3c/webdriver-bidi/pull/693
<orkon> q+
<jgraham> sasha: At the moment the spec says that we should update the size of a CSS Pixel. But that's not what CDP does, and when we tried to implement it it was complicated. We'd prefer to standardise what CDP does. That's a bit difficult to describe in the spec. We have a PR describing how this works in Gecko, but we need feedback to see if that's similar to what CDP does and others can implement.
<jgraham> ack next
<jgraham> ScribeNick: jgraham
<jgraham> orkon: The pixel size idea doesn't work well. In CDP we get a different result from screenshots and the onscreen display. We currently don't support this kind of emulation for iframes. setViewport makes sense for the top-level context. Are you able to support this in iframes? For users it's important that this has an effect on the screenshot size. I think the current update doesn't cover that. We need
<sasha> q+
<jgraham> to think about that.
<jgraham> ack next
<jgraham> sasha: I'm not 100% sure about iframes; we haven't tested that. We change it on the browsing context, but it might not make sense from the user point of view. For screenshots I think it should make a difference at least in the implementation, it's not clear about the spec.
<jgraham> jgraham: The spec change was based on what Firefox's RDM mode uses, but we need someone to check if that is the same behaviour that other browsers have.
<orkon> I will comment on the PR with what I mentioned after taking an extra look at Chrome implementation
<jgraham> q?
whimboo commented 2 months ago
jgraham: The spec change was based on what Firefox's RDM mode uses, but we need someone to check if that is the same behaviour that other browsers have.

That's actually not accurate. The Firefox RDM implementation also doesn't use fullZoom but exact the same mode as CDP does by setting the overrideDPPX property on the browsing context which then gets distributed through all child contexts as well.

For details see: https://searchfox.org/mozilla-central/rev/058ab60e5020d7c5c98cf82d298aa84626e0cd79/dom/chrome-webidl/BrowsingContext.webidl#163-176

As such using the behavior from CDP would be equivalent to RDM and that is what we are proposing here.

AutomatedTester commented 2 months ago

I wonder if we need to have anything to devicePixelRatio at all because CDP does have it but if you then send over other commands(interaction commands as example) they will not work as intended and the wrong elements will get the command leading to element click intercepted errors being thrown.

If we leave it in then I think we need to have meaningful tests added to show that interactions work as expected (which they don't currently)