Closed Bug 1544417 Opened 1 year ago Closed 6 months ago

Implement Emulation.setDeviceMetricsOverride's

Categories

(Remote Protocol :: Emulation, task, P1)

Tracking

(firefox74 fixed)

RESOLVED FIXED
Firefox 74
Tracking Status
firefox74 --- fixed

People

(Reporter: ochameau, Assigned: whimboo)

References

(Depends on 1 open bug, Blocks 9 open bugs, )

Details

(Whiteboard: [puppeteer-beta-mvp])

Attachments

(1 file, 2 obsolete files)

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.

Component: Agent → Emulation
Type: defect → enhancement
Priority: -- → P3
Priority: P3 → P2

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.

(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

(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?

(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.

Few tests impacted, ni? to list impacted tests.

Flags: needinfo?(jdescottes)
Priority: P2 → P3

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.

Flags: needinfo?(jdescottes)
Priority: P3 → P2
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Blocks: 1588432
Whiteboard: [puppeteer-alp
Whiteboard: [puppeteer-alp → [puppeteer-alpha]
Priority: P1 → P2

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.

Blocks: 1588622

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Summary: Implement Emulation.setDeviceMetricsOverride's width and height arguments → Implement Emulation.setDeviceMetricsOverride's

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.

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.

Status: ASSIGNED → NEW
Depends on: 1595697
Priority: P1 → P2
Assignee: hskupin → nobody
Attachment #9066354 - Attachment is obsolete: true
Attachment #9058254 - Attachment is obsolete: true
Priority: P2 → P3
Whiteboard: [puppeteer-alpha] → [puppeteer-alpha-reserve]
Blocks: 1602926

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");

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.

No longer blocks: 1588622
Priority: P3 → P2
Whiteboard: [puppeteer-alpha-reserve] → [puppeteer-beta-mvp]
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-mvp][puppeteer-high-priority]
Blocks: 1587845
Blocks: 1604723
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Depends on: 1605784
Depends on: 1596136
Attachment #9119075 - Attachment description: Bug 1544417 - [remote] Implement Emulation.setDeviceMetricsOverride. r=#remote → Bug 1544417 - [remote] WIP: Implement Emulation.setDeviceMetricsOverride. r=#remote

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?

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(emilio)
Priority: P1 → P2
See Also: → 1604756
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1

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.

Flags: needinfo?(emilio)

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

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
Blocks: 1608468
Blocks: 1608470

(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.

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1

(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.

Attachment #9119075 - Attachment description: Bug 1544417 - [remote] WIP: Implement Emulation.setDeviceMetricsOverride. r=#remote → Bug 1544417 - [remote] Implement Emulation.setDeviceMetricsOverride. r=#remote

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.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8d3c8a85b63
[remote] Implement Emulation.setDeviceMetricsOverride. r=remote-protocol-reviewers,maja_zf,ato
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
Type: enhancement → task
Blocks: 1610285
Whiteboard: [puppeteer-beta-mvp][puppeteer-high-priority] → [puppeteer-beta-mvp]
You need to log in before you can comment on or make changes to this bug.