Implement Emulation.setDeviceMetricsOverride's
Categories
(Remote Protocol :: CDP, task, P1)
Tracking
(firefox74 fixed)
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: ochameau, Assigned: whimboo)
References
(Blocks 8 open bugs, )
Details
(Whiteboard: [puppeteer-beta-mvp])
Attachments
(1 file, 2 obsolete files)
(deleted),
text/x-phabricator-request
|
Details |
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.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 2•5 years ago
|
||
Comment 3•5 years ago
|
||
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.
Reporter | ||
Comment 4•5 years ago
|
||
(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•5 years ago
|
||
(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 callingbrowser.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-L43I 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•5 years ago
|
||
(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•5 years ago
|
||
Few tests impacted, ni? to list impacted tests.
Comment 8•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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 | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
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");
Assignee | ||
Comment 14•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
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?
Updated•5 years ago
|
Updated•5 years ago
|
Comment 17•5 years ago
|
||
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.
Assignee | ||
Comment 18•5 years ago
|
||
I currently have to unassign myself from the bug due to Fission work.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
(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 andwidth
/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
ordevice-{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.
Assignee | ||
Comment 20•5 years ago
|
||
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•5 years ago
|
||
(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 andwidth
/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.
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
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•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e8d3c8a85b63 [remote] Implement Emulation.setDeviceMetricsOverride. r=remote-protocol-reviewers,maja_zf,ato
Comment 24•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•4 years ago
|
Description
•