report: add full-page-screenshot to experimental config by connorjclark · Pull Request #10716 · GoogleChrome/lighthouse

connorjclark · 2020-05-06T23:20:04Z

Summary

New gatherer: full-page-screenshot changes the screen emulation to be as large as the content height and snaps a JPEG, backing off as necessary to meet size thresholds. After: if Lighthouse is in control of the emulation, we easily set it back to the emulated state. Otherwise, if emulation is outside of our control, we do our best to put it back. In practice this probably doesn't matter, since this is the afterPass of the very last gatherer, and we'd expect any programatic usage of Lighthouse to not reuse the page after the LH run.

New details: for the node detail type, an optional boundingRect represents the location of the node on the screen.

New audit: full-page-screenshot, simply exposes the FullPageScreenshot artifact.

New renderer: element-screenshot-renderer, for node details with a boundingRect, display a clipped image of the full page screenshot. On click, use the same renderer to paint the clipped image in a larger viewport in an overlay. The overlay is enabled in report-ui-features.

old

(before many refactors)

emulation

The previous PR wired emulation overrides through the driver + emulation files. Instead, I moved the presets out of emulation.js and into driver.js, meaning the overrides only go to Driver.beginEmulation. I think moving presets out of emulation.js makes the file better encapsulated.

on click

currently if you click on a node image you'll get a shadowbox overlay over the entire report. I see there is some code for instead showing the image in the corner of the page. I suppose we could also do a tooltip hover thing. We should sort out exactly what we want to do.

size

I added a script json-sizes so we can get a feel for how much larger this will make the LHR.

cat lhr.json | jq .audits | node lighthouse-core/scripts/json-size.js

theverge.com:

total                              100	897127
full-page-screenshot               38	337865
network-requests                   27	239008
screenshot-thumbnails              11	96481
final-screenshot                   4	37873
uses-long-cache-ttl                4	36034
tap-targets                        2	13667
network-server-latency             1	13084
network-rtt                        1	10443
third-party-summary                1	10294
uses-rel-preconnect                1	8864
bootup-time                        1	6958
unused-javascript                  1	6323
user-timings                       0	3702
...snip...

example.com:

total                              100	155237
screenshot-thumbnails              43	66273
final-screenshot                   10	15533
full-page-screenshot               9	13700
resource-summary                   1	1278
metrics                            1	1231
network-requests                   1	1210
mainthread-work-breakdown          1	1069
dom-size                           1	1006
html-has-lang                      1	969
critical-request-chains            1	946
bootup-time                        1	848
font-size                          0	739
...snip...

Cont: #8797

Note: do not use "land when green". Add Co-authored-by: Matt Zeunert <matt@mostlystatic.com> to commit body.

brendankenny · 2020-07-17T21:04:49Z

    Util.reportJson = report;

-    const detailsRenderer = new DetailsRenderer(this._dom);
+    const fullPageScreenshot = /** @type {LH.Artifacts.FullPageScreenshot | undefined} */ (


same thing with the audit details type and possibly avoiding the cast

how to avoid the cast?

how to avoid the cast?

caveating that I could be wrong because I haven't tried it :) I assume details will already either be LH.Audit.Details or undefined, so testing that it exists and has type full-page-screenshot should be enough to convince tsc.

alright how's that

for report-ui-features.js could also do something like

const fullPageScreenshot = report.audits['full-page-screenshot'] && report.audits['full-page-screenshot'].details; if (!fullPageScreenshot || fullPageScreenshot.type !== 'full-page-screenshot') return;

or some kind of code golfing, but it's also good enough if you're happy :)

cool, just clocks in at 100 characters

paulirish

spent some time in the renderer. in general it's dealing with the complexity quite well. :)

the var names make reading it a little tough so i have a few suggestions below that should help (and shouldn't be too contentious)

i'm also gonna do a followup PR with a slightly more contentious naming tweak

paulirish · 2020-07-17T21:40:58Z

+   * @param {LH.Artifacts.Rect} elementRectInScreenshotCoords
+   * @param {Size} elementPreviewSizeInScreenshotCoords
+   */
+  static renderClipPath(dom, maskEl, positionClip,


I believe we can simplify this, but I'll attempt it in a followup.

paulirish · 2020-07-17T22:03:23Z

Sign in to view

+    markerEl.style.top = positions.clip.top * zoomFactor + 'px';
+
+    const maskEl = dom.find('.lh-element-screenshot__mask', containerEl);
+    maskEl.style.width = elementPreviewSizeInDisplayCoords.width + 'px';


on my 2dpr screen i see a 1px edge on the right where the mask doesn't extend. not sure why, though.

not a blocker for shipping, obv.

paulirish · 2020-07-17T22:25:53Z

+   * @param {Size} elementPreviewSizeInScreenshotCoords
+   * @param {Size} screenshotSize
+   */
+  static getScreenshotPositions(elementRectInScreenshotCoords,


since this method is clearly working in screenshot coordinate scale, can we drop InScreenshotCoords from all the var names within?

I would like to push back a little on the idea that anything anywhere here is clearly working in any particular scale :)

it's definitely clearer what's happening within this function but in absence of comments getScreenshotPositions could easily be a name for a function that returns somethingScreenshotCords based on somethingDisplayCoords.

in my experience variable names are less easily ignored when reading a function to figure out what it's doing than function overview. I'm ok with removing the suffix, just didn't want it to die without a fight :)

aiight well if you're already feelin this, lemme pitch you on what I was going to put in a separate PR. basically abbreviating all the suffixes:

elementRectInScreenshotCoords => elementRect_SC

maxRenderSizeInDisplayCoords => maxRenderSize_DC

On my first pass looking at this renderer code i was really confused because the var names suffix was "Coords" but their types were Rect/Size. But then realized the different coords spaces and i'm pleased the names indicate this signal. But still, it just takes up so much visual space that I think it current hampers readability.

So yeah. suffix abbreviation. Connor's on board. so if you're into it, we could just do this.

edit: (Oh last bit... I already wrote a lil description for the top.. and we'd add in the abbv's in thar:

/** * @overview These functions define {Rect}s and {Size}s using two different coordinate spaces: * 1. Screenshot coords (SC): where 0,0 is the top left of the screenshot image * 2. Display coords (DC): that match the CSS pixel coordinate space of the LH report's page. */

)

this plus the fileoverview explaining the two coordinate spaces (and what the abbreviation means) SGTM 👍

Just no underscores, please :)

So yeah. suffix abbreviation. Connor's on board. so if you're into it, we could just do this.

plz do this in followup pr

…er.js Co-authored-by: Paul Irish <paulirish@google.com>

Co-authored-by: Paul Irish <paulirish@google.com>

…lscreens

mattzeunert added 30 commits May 14, 2019 16:51

WIP element screenshots

1b3c178

Store width/height with FPS

625a2b9

Use correct width for FPS

89d6ac2

Add FPS to sample json

af8c495

Update accessiblity artifacts

f0708c5

Update sample json

1d23453

Better handle desktop screenshots

2799517

Store fps audit result on details renderer

d473ca1

Positioning fixes

2a7bc86

Don't change unrelated emulation settings in FPS gatherer

e0b3edc

Show above table item

a854113

Zoom out

151cbcd

Tweaks

e3bf3dc

Always show in bottom right

0a81430

asfd

3e6c22f

click to toggle visible

cb6e4b1

nothing

761ad13

Remove node textcontent

4657913

Merge branch 'master' into element-screenshots

8cb7371

Fix merge

a85c6a9

show screenshot inline

362abfd

Merge branch 'master' into element-screenshots

2e3f280

Capture full-page screenshot

2ab034b

Don't inline elem screenshot again and again

6bbd1c5

Show modal on click

80bab17

Don't zoom out in modal so much

cd23fa5

Remove * 2

f53b83c

Link jpeg quality exploration

4d03200

Comment

c9f1593

Remove timing/timestamp from screenshots

012b81e

connorjclark added 10 commits July 14, 2020 13:47

pr

9bca476

dataset

90aaafc

Merge remote-tracking branch 'origin/master' into elscreens

5e495c9

color

c9850e9

type

9cdb115

travis flake

cfdffdd

set max screenshot height based on dpr

737a4b0

fix test

a5ea66f

travis flake

9641d0d

Merge remote-tracking branch 'origin/master' into elscreens

06d3e1b

connorjclark mentioned this pull request Jul 17, 2020

Make full-page-screenshot resistant to layout shifts #11118

Closed

paulirish mentioned this pull request Jul 17, 2020

full-page-screenshot: handle elements beyond MAX_SCREENSHOT_HEIGHT #11121

Closed

brendankenny reviewed Jul 17, 2020

View reviewed changes

connorjclark added 2 commits July 17, 2020 16:49

pr

5e689f3

pr

0456bfe

connorjclark mentioned this pull request Jul 17, 2020

Ensure running lighthouse with external emulation should work with full-page-screenshot #11122

Open

connorjclark added 3 commits July 17, 2020 17:27

comments

4b2d495

comment

9f0746e

style

3d8d697

paulirish approved these changes Jul 17, 2020

View reviewed changes

connorjclark and others added 6 commits July 17, 2020 22:17

Update lighthouse-core/report/html/renderer/element-screenshot-render…

97d9445

…er.js Co-authored-by: Paul Irish <paulirish@google.com>

Apply suggestions from code review

9fa93f3

Co-authored-by: Paul Irish <paulirish@google.com>

somerename

03c6a6b

Merge branch 'elscreens' of github.com:GoogleChrome/lighthouse into e…

2a4cb5b

…lscreens

lint

e39c5dc

fileoverview

ee3c365

paulirish mentioned this pull request Jul 22, 2020

report: tweak naming in element-screenshot renderer #11152

Merged

connorjclark mentioned this pull request Sep 23, 2020

core(full-page-screenshot): add full page screenshot to artifacts #9064

Closed

brendankenny mentioned this pull request Jan 5, 2021

core: normalize creation of NodeValue #11877

Merged

connorjclark mentioned this pull request Mar 10, 2021

☂️ Element screenshots #12230

Closed

16 tasks

Conversation

connorjclark commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

old

emulation

on click

size

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

connorjclark commented May 6, 2020 •

edited

Loading

brendankenny Jul 17, 2020 •

edited

Loading

patrickhulce Jul 18, 2020 •

edited

Loading

paulirish Jul 18, 2020 •

edited

Loading