Implement support for "contentSize" and "layoutViewport" of Page.getLayoutMetrics
Categories
(Remote Protocol :: CDP, enhancement, P1)
Tracking
(firefox73 fixed)
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [puppeteer-alpha-reserve])
Attachments
(2 files)
Assignee | ||
Comment 1•5 years ago
|
||
Putting this bug on hold for now. The Network domain related events seem to be more important to get started on first.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
This method is used for screenshots and various mouse input actions like hover, tap, and click as implemented on the ElementHandle
class. That class is being used in DOMWorld.js
to retrieve elements by a selector. As such it is necessary for the alpha release because Gutenberg e2e tests use a lot of calls to click()
.
Assignee | ||
Comment 3•5 years ago
|
||
Botond and/or Hiro, do we have some examples in the tree in how to get details about the layoutViewport
and visibleViewport
? Both are required to implement this CDP endpoint in Firefox. See https://chromedevtools.github.io/devtools-protocol/tot/Page#method-getLayoutMetrics. Thanks
Comment 4•5 years ago
|
||
So, based on the descriptions in that document:
LayoutViewport.pageX/Y
would bewindow.scrollX/Y
LayoutViewport.clientWidth/Height
would bedocument.documentElement.clientWidth/Height
, assuming "layout viewport" (which is a somewhat ambiguous term) is being interpreted as "initial containing block" here, rather than "fixed viewport". (It may be worth running this by David Bokan to double-check the intention).VisualViewport
fields other thanzoom
would be the corresponding fields ofwindow.visualViewport
, where that exists.- Where
window.visualViewport
does not exist, zooming is not supported, so:pageX/Y
andclientWidth/Height
are equal to the correspondingLayoutViewport
fieldsoffsetX/Y
are 0scale
is 1
VisualViewport.zoom
would bewindow.devicePixelRatio
Does that help?
Comment 5•5 years ago
|
||
(the link doesn't work with enabling contents blocking)
Assignee | ||
Comment 6•5 years ago
|
||
Thanks Botond! That's pretty helpful information. Especially for the visibleViewport
availability. As you say the Browser Zoom
feature in Firefox doesn't have an effect here? It's only about pinching in mobile apps?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
(the link doesn't work with enabling contents blocking)
Sometimes the page throws a 404. Go to a different page and back to Page
to get content.
Comment 7•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+2] from comment #6)
As you say the
Browser Zoom
feature in Firefox doesn't have an effect here? It's only about pinching in mobile apps?
The browser zoom will have an impact on window.devicePixelRatio
(and therefore on VisualViewport.zoom
).
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #7)
As you say the
Browser Zoom
feature in Firefox doesn't have an effect here? It's only about pinching in mobile apps?The browser zoom will have an impact on
window.devicePixelRatio
(and therefore onVisualViewport.zoom
).
So as it looks like the visual viewport support is not enabled by default in Firefox yet, and you will have to set the dom.visualviewport.enabled
preference to true
. Which means that with the CDP feature enabled we will have to also set this preference to make use of the visual viewport details.
But even with it enabled the value for scale
sticks with 1
even I zoom in our out in Firefox. Given your comment 4 I assume I can simply ignore this property?
Comment 9•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+2] from comment #8)
So as it looks like the visual viewport support is not enabled by default in Firefox yet,
It's currently only enabled on Android. It will be enabled on desktop at a later time, as part of the desktop zooming effort.
and you will have to set the
dom.visualviewport.enabled
preference totrue
. Which means that with the CDP feature enabled we will have to also set this preference to make use of the visual viewport details.
I wouldn't recommend changing the preference, as the implementation is not passing tests on desktop yet and may return unexpected values.
I would recommend testing for the existence of window.visualViewport
, and using the mentioned fallbacks if it doesn't exist.
But even with it enabled the value for
scale
sticks with1
even I zoom in our out in Firefox.
The names are a bit confusing, but based on the descriptions, scale
reflects pinch-zoom, and zoom
reflects browser zoom.
By "zoom in or out in Firefox", I assume you mean browser zoom on Firefox desktop, in which case I would expect scale
to remain at 1 and zoom
to change.
Given your comment 4 I assume I can simply ignore this property?
I'm not sure what exactly the "CDP feature" is, but if it's applicable to Android, you don't want to ignore scale
as that's relevant for pinch-zooming there.
Even if it's not applicable to Android, since we're planning to bring pinch-zooming to desktop, I would suggest not ignoring scale
but hooking it up with the fallback as suggested (but that's more negotiable).
Comment 10•5 years ago
|
||
LayoutViewport.clientWidth/Height would be document.documentElement.clientWidth/Height, assuming "layout viewport" (which is a somewhat ambiguous term) is being interpreted as "initial containing block" here, rather than "fixed viewport". (It may be worth running this by David Bokan to double-check the intention).
Not quite sure about "intent" but my read of [1] in Chromium is that we do actually use the "fixed viewport" here. i.e. if you have a 600px width device with <meta name="viewport" content="width=device-width,minimum-scale=2">
, the LayoutViewport.clientWidth will be 300 because the fixed viewport is based on minimum scale.
Comment 11•5 years ago
|
||
Thanks, David.
In that case, the computation for LayoutViewport.clientWidth
would be something like document.scrollingElement.scrollWidth - window.scrollMaxX
(and similarly for height). It can be changed to window.innerWidth/Height
after bug 1514429 is fixed.
Assignee | ||
Comment 12•5 years ago
|
||
Thank you all for these information! I'm hoping to get to this bug soon.
Comment 13•5 years ago
|
||
(In reply to Botond Ballo [:botond] [standards meeting Nov 4-9] from comment #11)
In that case, the computation for
LayoutViewport.clientWidth
would be something likedocument.scrollingElement.scrollWidth - window.scrollMaxX
(and similarly for height). It can be changed towindow.innerWidth/Height
after bug 1514429 is fixed.
I think that might miss some edge-cases related to the viewport meta tag. E.g. the fixed viewport can be restricted with minimum-scale=1
for example which might not be reflected by document.scrollingElement.scrollWidth - window.scrollMaxX
. Granted, I don't know how Gecko works in these cases but definitely something to check.
Assignee | ||
Updated•5 years ago
|
Comment 14•5 years ago
•
|
||
(In reply to David Bokan from comment #13)
(In reply to Botond Ballo [:botond] [standards meeting Nov 4-9] from comment #11)
In that case, the computation for
LayoutViewport.clientWidth
would be something likedocument.scrollingElement.scrollWidth - window.scrollMaxX
(and similarly for height). It can be changed towindow.innerWidth/Height
after bug 1514429 is fixed.I think that might miss some edge-cases related to the viewport meta tag. E.g. the fixed viewport can be restricted with
minimum-scale=1
for example which might not be reflected bydocument.scrollingElement.scrollWidth - window.scrollMaxX
. Granted, I don't know how Gecko works in these cases but definitely something to check.
So I tried the attached simple testcase, where the content size is twice the ICB size, so there would be room for the fixed viewport to be expanded, but the minimum-scale=1
restricts it.
In this scenario, I am seeing:
document.scrollingElement.scrollWidth
is twice my screen width (on the phone I'm testing with, 720px)window.scrollMaxX
is equal to my screen width (360px)- their difference is equal to my screen width (360px)
That seems like the desired value of the fixed viewport width. Not sure which part of the calculation you were expecting to trip up?
Assignee | ||
Comment 15•5 years ago
|
||
As it looks like there is still a bit of discussion going on. As such I will work on the device emulation support (bug 1588432) first. This will also help to prove that particular values can be compared with Chrome not only for Firefox, but for the full set of devices as supported by Puppeteer. The first thing to implement is bug 1544417.
Comment 16•5 years ago
|
||
That seems like the desired value of the fixed viewport width. Not sure which part of the calculation you were expecting to trip up?
If you change to minimum-scale=0.75
does it still work? If the scrollMaxX is now 240 then this works correctly (since the fixed viewport will how be 480px wide and the scrollWidth should remain 720px). Thanks for checking.
Comment 17•5 years ago
|
||
(In reply to David Bokan from comment #16)
If you change to
minimum-scale=0.75
does it still work? If the scrollMaxX is now 240 then this works correctly (since the fixed viewport will how be 480px wide and the scrollWidth should remain 720px). Thanks for checking.
Yep, still works.
The reason this works is that window.scrollMaxX
is the largest value that window.scrollX
can take on, and as per the discussion we had here, window.scrollX/Y
is limited to a scroll range determined by the fixed viewport.
Updated•5 years ago
|
After tinkering with Bug 1549465, this looks like the next method that is blocking gutenberg, since Puppeteer uses it for mouse clicks.
Assignee | ||
Comment 19•5 years ago
|
||
Maja, if that is blocking you feel free to pick it up. Otherwise I would start working on this bug after bug 1544417 has been fixed.
Puppeteer's page.click(someSelector)
breaks in ElementHandle._clickablePoint, and in particular it relies on the layoutViewport
value from the Page.getLayoutMetrics command.
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #11)
In that case, the computation for
LayoutViewport.clientWidth
would be something likedocument.scrollingElement.scrollWidth - window.scrollMaxX
(and similarly for height). It can be changed towindow.innerWidth/Height
after bug 1514429 is fixed.
Please note that the patch from bug 1514429 landed 8h ago. So I would suggest to wait and directly make use of window.innerWidth/Height
.
Comment 22•5 years ago
|
||
Setting priority back to 'P3' so the bug appears in the project's Bugzilla queries.
Assignee | ||
Comment 23•5 years ago
|
||
We are moving out the more complex "visibleViewport" part to bug 1602926 given that it is not immediately necessary. Means as first step I'm going to implement the contentSize
(for full screenshots) and layoutViewport
(Gutenberg tests) first.
Assignee | ||
Comment 24•5 years ago
|
||
So I did two tests with Chrome and Firefox. All with a maximized window. One test is for a page without scrollbars, and another one with vertical scrollbars.
No scrollbars: http://example.org
Chrome:
"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1680,"clientHeight":846}
"contentSize":{"x":0,"y":0,"width":1680,"height":846}
Firefox:
"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1680,"clientHeight":895}
"contentSize":{"x":0,"y":0,"width":1680,"height":895}
Our available height in content is a bit larger because in Firefox we do not show the notification bar that the browser is under remote control. So I assume that this are those 49px.
With vertical scrollbar: https://www.mozilla.org
Chrome:
"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1665,"clientHeight":846}
"contentSize":{"x":0,"y":0,"width":1665,"height":7114}
Firefox:
"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1680,"clientHeight":895}
"contentSize":{"x":0,"y":0,"width":1665,"height":6965}
Similar situation for the available height, but check the clientWidth
for the layoutViewport
. There is a difference of 15px, which is exactly the scrollbar width.
Chromes implementation seems to be located here:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc?type=cs&g=0&l=716
Given that it includes the scrollbar the scrollbar_inclusion
flag is true
for at least CDP. So just using window.innerWidth
and window.innerHeight
doesn't seem to be enough.
Assignee | ||
Comment 25•5 years ago
|
||
Hm, for my daily profile and when starting Firefox with a new profile outside of Puppeteer I get the expected 1680px. I should investigate.
Assignee | ||
Comment 26•5 years ago
|
||
Sorry, 1680px isn't actually expected. So Chrome captures the screenshot definitely without the scrollbars, and as such their width and height will have to be taken into account. When doing that I get:
{"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1665,"clientHeight":895},"contentSize":{"x":0,"y":0,"width":1665,"height":6965}}
It's exactly what it should be. So all fine.
Assignee | ||
Comment 27•5 years ago
•
|
||
Assignee | ||
Comment 28•5 years ago
|
||
Comment 29•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4a36df9e03e9 [remote] Implement "layoutViewport" and "contentSize" for Page.getLayoutMetrics. r=remote-protocol-reviewers,maja_zf
Comment 30•5 years ago
|
||
bugherder |
Comment 31•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [βοΈUTC+1] from comment #26)
Sorry, 1680px isn't actually expected. So Chrome captures the screenshot definitely without the scrollbars, and as such their width and height will have to be taken into account. When doing that I get:
{"layoutViewport":{"pageX":0,"pageY":0,"clientWidth":1665,"clientHeight":895},"contentSize":{"x":0,"y":0,"width":1665,"height":6965}}
Just so I understand this: if Chromium takes screenshots of the full document area as if the scrollbars are not present, would we want to follow Jugglerβs example with ScrollbarManager.js and hide them prior to taking the screenshot?
Assignee | ||
Comment 32•5 years ago
•
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #31)
Just so I understand this: if Chromium takes screenshots of the full document area as if the scrollbars are not present, would we want to follow Jugglerβs example with ScrollbarManager.js and hide them prior to taking the screenshot?
Would totally make sense also given that there is Emulation.setScrollbarsHidden(), which we haven't implemented yet. I filed bug 1604799 for it. Thanks for noticing that.
Updated•4 years ago
|
Description
•