Closed kraten closed 6 months ago
Running a bisect identifies #14418 to be the change that caused a regression.
git bisect start HEAD v9.6.6 --
git bisect run node ../spikes/lighthouse-bisect-fullpagescreenshot/index.js
...
LH:status Generating results... +0ms
This version is: Bad.
Actual dimensions: 360x6988.
Declared dimensions: 477x9259.
501133d70035dc1ea5b910efcd655abd2545d3c6 is the first bad commit
core(fps): use observed metrics for screenshot dimensions (#14418)
I spent some more time with it — I think our full page screenshot dimension calculations could be improved.
The reason why this specific site's audit is producing a difference between declared screenshot dimensions (that's in LHR) and actual dimensions (that of WebP image) is due to the page's horizontal overflow. Any page that has a horizontal overflow (a horizontal scrollbar for that matter) should be able to reproduce the inconsistency.
The root cause of this inconsistency comes from the way we calculate screenshot area. For a page with horizontal overflow, Chromium produces a window.innerWidth
that's larger than window.outerWidth
. Per spec, this seems correct behavior as window.innerWidth
is the width of the layout viewport, i.e., what is available to view. However, prior to taking a a screenshot, we resize the viewable viewport to a width of zero, that retains the width of the viewport to the emulated screen width. This forces the screenshot to clip horizontal overflow, which also seems to be the correct behavior as we like to take the screenshot of what the user is actually experiencing. However, this introduces the discrepancy in dimensions, with a WebP image dimension that's smaller than declared in LHR.
Similarly, the height calculation is a scaled approximation between device height, content height and viewport client height. I haven't fully understood the reasoning behind this calculation, but it seems to produce enough shortage from actual content height in most cases and results in a screenshot that's shorter than full height of the page. As a side effect of this, all element screenshot coordinate calculations are off for the page.
FWIW, I propose that we simplify this calculations to the following:
// To obtain a full page screenshot, we resize the emulated viewport to
// (1) a height equal to the maximum of visual-viewport height and document height.
// (2) a width equal to emulated visual-viewport width (we choose to clip overflow on x-axis).
// Finally, we cap the viewport to a maximum size allowance of WebP format.
const fullHeight = Math.max(deviceMetrics.height, metrics.cssContentSize.height);
const height = Math.min(fullHeight, MAX_WEBP_SIZE);
const width = Math.min(deviceMetrics.width, MAX_WEBP_SIZE);
Once we do that, we could avoid reading window.innerWidth
and rely on cssVisualViewport.client{width|height}
returned by Page.getLayoutMetrics
. The dimensions of what's captured and what's calculated as screenshot area should match at that point.
I'm testing this out in a branch and it seems to work, assuming our dpr
and other corner case tests are exhaustive.
Thoughts? Are there cases where I might have had an oversight?
I haven't fully digested everything about horizontal overflow, but it does appear that the placement of our element bounding boxes are off if there is horizontal overflow. Your branch seems to fix this issue at least so I think it's close to a solution.
Similarly, the height calculation is a scaled approximation between device height, content height and viewport client height. I haven't fully understood the reasoning behind this calculation, but it seems to produce enough shortage from actual content height in most cases and results in a screenshot that's shorter than full height of the page. As a side effect of this, all element screenshot coordinate calculations are off for the page.
It's important to point out that deviceMetrics.height
is not necessarily on the same scale as metrics.cssContentSize.height
. The fullHeight
calculation is incorrect for the fps-scaled
smoke test in your branch.
This is the reasoning behind the current fullHeight
calculation. We know how much we want to scale the window.innerHeight
by, but we can only control the window.outerHeight
.
FAQ
URL
https://wp-rocket.me
What happened?
I generated a JSON report for this URL: https://wp-rocket.me
From the JSON report, I extracted the webp image buffer and converted it to a file.
While I was checking the file attributes, I noticed the width and height mentioned in the report are different compared to what the actual size is.
Actual Size: 412 × 8795 pixels Size mentioned in report: 417 x 8902 pixels
What did you expect?
I expect the full-page screenshot image buffer to be the same size as mentioned in the report.
What have you tried?
I found this issue only for this particular URL. All other URLs I tried are working fine. Also, I'm getting the correct width and height with lighthouse 9.6.6.
How were you running Lighthouse?
CLI, node
Lighthouse Version
10.0.1
Chrome Version
109.0.5414.119 (Official Build) (arm64)
Node Version
16.18.1
OS
Mac
Relevant log output