1544417 - Implement Emulation.setDeviceMetricsOverride's

Status: RESOLVED FIXED

(Remote Protocol :: CDP, task, P1)

Description

[Alexandre Poirot [:ochameau]]

https://chromedevtools.github.io/devtools-protocol/tot/Emulation#method-setDeviceMetricsOverride
Emulation.setDeviceMetricsOverride:
Overrides the values of device screen dimensions (window.screen.width, window.screen.height, window.innerWidth, window.innerHeight, and "device-width"/"device-height"-related CSS media query results).

For now we exposed Emulation.setDeviceMetricsOverride but with an empty implementation:
https://searchfox.org/mozilla-central/rev/0376cbf447efa16922c550da3bfd783b916e35d3/remote/domains/content/Emulation.jsm#14

Wordpress test suites uses it to change the viewport size over here:
https://github.com/WordPress/gutenberg/blob/b786aad41b4670190b6cac4d99bab3abd3f85049/packages/e2e-test-utils/src/set-browser-viewport.js#L17-L18
and relies on setDeviceMetricsOverride to update the page width and height.
It only requires to implement setDeviceMetricsOverride's width and height arguments.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1544417 - Implement Emulation.setDeviceMetricsOverride's width and height arguments

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1544417 - setDeviceMetrics workaround for preview.test.js.

Comment 3

[Julian Descottes [:jdescottes]]

Chrome seems to have a different approach concerning the implementation. When resizing the viewport Chrome only resizes the inner viewport, not the window.

Some consequences of this difference:
1/ you can resize to very small dimensions or very big dimensions, regardless of the limitations of the hardware/OS. The implementation attached here will time out in such cases.
2/ different tabs can be resized independently. Which means among other things that calling browser.newPage on Firefox resets the viewport for previous pages.

For the second patch attached here, I can't get the mentioned test to run properly and consistently.

On a sidenote, I think very few tests are currently blocked by having the empty implementation, so I think I will wait until more crucial APIs are landed to work on this.

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Chrome seems to have a different approach concerning the implementation. When resizing the viewport Chrome only resizes the inner viewport, not the window.

Oh that explain why juggler resize the <xul:browser> element:
https://github.com/puppeteer/juggler/blob/master/testing/juggler/protocol/PageHandler.js#L54-L77
https://github.com/puppeteer/juggler/blob/master/testing/juggler/content/PageAgent.js#L31-L43

I found it so weird to do that, but I did not realized it was actually similar to what happens in chrome.
I thought it was a workaround to not have to handle the size of chrome elements.

For the second patch attached here, I can't get the mentioned test to run properly and consistently.

This test is intermittent. It still fails quite often with:

Preview
    ✕ should open a preview window for a new post (7926ms)

  ● Preview › should open a preview window for a new post

    INTERNAL ERROR: missing context with id = 12884901905

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Julian Descottes [:jdescottes] from comment #3)

1/ you can resize to very small dimensions or very big
dimensions, regardless of the limitations of the hardware/OS. The
implementation attached here will time out in such cases.

But this is ony in headless mode, right?

I believe we can support limitless resizing in headless mode because
all the painting code is mocked out, which means we’re not limited
by the hardware/OS. In normal windowed mode, I would think both
Chrome and we are limited by the surrounding system.

2/ different tabs can be resized independently. Which means among
other things that calling browser.newPage on Firefox resets the
viewport for previous pages.

Interesting find.

(In reply to Alexandre Poirot [:ochameau] from comment #4)

Oh that explain why juggler resize the <xul:browser> element:
https://github.com/puppeteer/juggler/blob/master/testing/juggler/protocol/PageHandler.js#L54-L77
https://github.com/puppeteer/juggler/blob/master/testing/juggler/content/PageAgent.js#L31-L43

I found it so weird to do that, but I did not realized it was actually similar to what happens in chrome.
I thought it was a workaround to not have to handle the size of chrome elements.

Would we be OK to do that?

I guess the primary mode of operation for Puppeteer is headless,
so the users wouldn’t necessarily notice the <xul:browser> extending
beyond the visible application window. But how would we reconcile
this in windowed mode?

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Andreas Tolfsen 「:ato」 from comment #5)

(In reply to Julian Descottes [:jdescottes] from comment #3)

1/ you can resize to very small dimensions or very big
dimensions, regardless of the limitations of the hardware/OS. The
implementation attached here will time out in such cases.

But this is ony in headless mode, right?

No, it's also the case in regular/non-headless mode.

Comment 7

[Julian Descottes [:jdescottes]]

Few tests impacted, ni? to list impacted tests.

Comment 8

[Julian Descottes [:jdescottes]]

Setting back to P2, most tests will timeout if we don't resize the window to the proper size at the beginning of the test.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This method is used in Puppeteer for capturing screenshots and in various other methods to emulate the viewport like page.setViewport. Given that this is used in Gutenberg e2e tests we have to keep it for the alpha release.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This method is also used for captureScreenshot with the fullPage argument. As this is part of the puppeteer examples, and puppeteer sets all the arguments of this method, we have to implement them all and not only width and height.

I'm taking this bug given that it blocks further work on bug 1588622.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here the two calls inside of Puppeteer's lib folder:

EmulationManager.js (emulateViewport()):

this._client.send('Emulation.setDeviceMetricsOverride', { mobile, width, height, deviceScaleFactor, screenOrientation });

Page.js (_screenshotTask()):

await this._client.send('Emulation.setDeviceMetricsOverride', { mobile: isMobile, width, height, deviceScaleFactor, screenOrientation });

That means that our basic implementation needs width, height, mobile, deviceScaleFactor, and screenOrientation.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bug 1595697 hasn't been reviewed yet, and I would need it landed first before I can start here because it lays the foundation of the Emulation domain on the parent process.

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

This is the next thing blocking test setup in gutenberg, after Page.getLayoutMetrics. The usage is described in Comment 0. Here is a minimal example of what gutenberg is doing.

const page = await browser.newPage();
await page.goto(url);
await page.setViewport({ width: 600, height: 700 });
await page
  .mainFrame()
  .waitForFunction("window.innerWidth === 600 && window.innerHeight === 700");

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

the Juggler implementation for setViewPort is:

async setViewport({deviceScaleFactor, isMobile, hasTouch}) {
    const docShell = this._frameTree.mainFrame().docShell();
    docShell.contentViewer.overrideDPPX = deviceScaleFactor || this._initialDPPX;
    docShell.deviceSizeIsPageSize = isMobile;
    this._scrollbarManager.setFloatingScrollbars(isMobile);
  }

It's not covering width, height, and orientation. But maybe it's something which could help us here, and which might be better than resizing the browser.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1544417 - [remote] Implement Emulation.setDeviceMetricsOverride. r=#remote

Depends on D58934

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I attached an early WIP which I don't actually have the time to continue on due the Fission work I have to get started. Whoever continues here please be aware of the ongoing discussion with Emilio on bug 1604756.

Emilio, would you mind to have a look at comment 14? Is that an implementation which would solve the problem you mentioned on bug 1604756 comment 7?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

No, that has the behavior described in that comment.

That being said, if per the documentation inner{Width,Height} changes, I'd expect viewport units and width / height media queries and such to change as well. So it is consistent at least.

I don't see how it'd override screen or device-{width,height} though.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I currently have to unassign myself from the bug due to Fission work.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

That being said, if per the documentation inner{Width,Height} changes, I'd expect viewport units and width / height media queries and such to change as well.

I filed bug 1608470 to investigate what's necessary for the 2nd part of your sentence.

I don't see how it'd override screen or device-{width,height} though.

Per definition of the CDP command setting the screen width/height is experimental, and as such we don't want to support that. We will be happy with the basics for now.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I was actually asked by the team to get the basics landed, which in this case will be the height, width, deviceScaleFactor. Everything else is whether mobile related, which we do not support anyway right now, or experimental API by CDP, which we don't focus on.

As such I will quickly finish it up before starting with Fission related work.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #19)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

That being said, if per the documentation inner{Width,Height} changes, I'd expect viewport units and width / height media queries and such to change as well.

I filed bug 1608470 to investigate what's necessary for the 2nd part of your sentence.

To be clear, that should be happening already, I'd think. For example, if you have something like:

<div style="background: green; width: 100vw; height: 100vh"></div>, I'd expect that to always occupy the whole viewport. I was just not sure whether that was intended or not.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For the last patch update I see the following when setting the emulation to 500x800:

Chrome:

  {
    layoutViewport: { pageX: 0, pageY: 0, clientWidth: 485, clientHeight: 800 },
    visualViewport: {
      offsetX: 0,
      offsetY: 0,
      pageX: 0,
      pageY: 0,
      clientWidth: 485,
      clientHeight: 800,
      scale: 1,
      zoom: 1
    },
    contentSize: { x: 0, y: 0, width: 485, height: 25852 }
  }

Firefox:

 {
    layoutViewport: { pageX: 0, pageY: 0, clientWidth: 485, clientHeight: 800 },
    contentSize: { x: 0, y: 0, width: 485, height: 26317 }
  },

It matches up perfectly. The discrepancy of 65px in the contentSize might be some different rendering. We will see once we can implement full page screenshots.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8d3c8a85b63
[remote] Implement Emulation.setDeviceMetricsOverride. r=remote-protocol-reviewers,maja_zf,ato

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/e8d3c8a85b63