WIP: Element screenshots by mattzeunert · Pull Request #8797 · GoogleChrome/lighthouse

mattzeunert · 2019-05-02T12:41:37Z

Summary

Very WIP 🏗

take full-page screenshot (created a separate PR for this)
capture element bounding boxes
render element screenshot in report

Todo:

figure out final UX
we should scroll down the page once before capturing element positions or the screenshot
- otherwise page content size may change on scroll, e.g. because of lazy loading images/ads
- example: footer promo slider at the bottom of the BBC homepage
- Ticket: How should we handle content changing when scrolling down the page? #8647
  otherwise page content size may change on scroll (e.g. on the BBC homepage)
sort out how chrome devtools instantiates detailsrenderer
- I think this may actually be resolved now, seems like Chrome no longer has custom code to initialize the details renderer
don't use clippath (see Patrick's note on browser support, also there's no reason we need it)
do we need to check if the element is not visible and then not capture the bounding box so that we don't show a screenshot that doesn't show the element (e.g. because it's overflowing a scroll container or there's an overlay on top of it)
handle elements that are outside the captured screenshot area (e.g. for pages > 16000px)

Known issues:

if resizing the window changes the layout then the element marker positions will be off (e.g. this happens for position fixed elements, or if the page uses vh)
elements in overflow: scroll containers may be hidden
elements can be obscured by full-screen overlays

Related Issues/PRs

fixes #6683
fixes #7327

mattzeunert · 2019-05-02T12:43:03Z

+    snippetEl.innerText = item.snippet;
+    element.appendChild(snippetEl);
+
+    const fullpageScreenshotUrl = __LIGHTHOUSE_JSON__.audits['full-page-screenshot'].details.data;


What's a good way to access this in the renderer?

I would say it should probably be passed into a DetailsRenderer as the context of the report, almost like setTemplateContext

Passing it into the DetailsRenderer constructor now.

patrickhulce

nice @mattzeunert do you think you could update the artifacts so we can play around with it in the now.sh deployment report on the PR? (right now report renderer just throws and page is empty)

patrickhulce · 2019-05-14T18:00:06Z

+    height = Math.min(maxScreenshotHeight, height);
+
+    await driver.sendCommand('Emulation.setDeviceMetricsOverride', {
+      // todo: figure out what this means and set appropriately


😆 I wonder how many devtools protocol settings we should add this comment to 🤔

patrickhulce · 2019-05-14T18:01:28Z

+    snippetEl.innerText = item.snippet;
+    element.appendChild(snippetEl);
+
+    const fullpageScreenshotUrl = __LIGHTHOUSE_JSON__.audits['full-page-screenshot'].details.data;


I would say it should probably be passed into a DetailsRenderer as the context of the report, almost like setTemplateContext

patrickhulce · 2019-05-14T18:06:09Z

+.lh-element-screenshot__content {
+  position: absolute;
+  right: 0px;
+  top: -190px;


this is based on the dynamic height we set it js? anyway to fix it or not have the hanging magic value? :)

It's so that the screenshot doesn't cover the table item you're hovering over and instead appears above it. Not really based on anything though.

Do you think showing the screenshot above the table item on hover is the way to go? And then on mobile we could expand the table item to include the screenshot when the user taps on it.

patrickhulce · 2019-05-14T18:10:55Z

+    if (item.boundingRect) {
+      /** @type {Element} */
+      let elementScreenshot;
+      element.addEventListener('mouseenter', () => {


not sure what our stance is on JS in report now

@paulirish 🤓 🔫 , CSS-only impl? ;)

I can't find the comment, but I think we decided that's ok on the snippet renderer PR.

Not sure how feasible this would be to do without JS, browsers might not like having a hundred 500kb base64 images on one page.

Oh that sounds right. I was remembering some I/O convo about going back and forth to make things easier on PSI, but we did already break that barrier.

patrickhulce · 2019-05-14T18:14:42Z

+    const clipId = 'clip-' + Math.floor(Math.random() * 100000000);
+    mask.style.width = previewWidth + 'px';
+    mask.style.height = previewHeight + 'px';
+    mask.style.clipPath = 'url(#' + clipId + ')';


clever! I think the chart is scarier than reality given this narrow usecase but still might be troublesome for us? https://caniuse.com/#search=clipPath

would you mind explaining in words what the approach is here? I think I'm getting it but not sure why the mask is necessary

There isn't anything that can't be done with divs positioned over the image, will change it.

mattzeunert · 2019-05-15T07:13:20Z

do you think you could update the artifacts so we can play around with it in the now.sh deployment report on the PR

Done now 👍

patrickhulce · 2019-05-15T15:03:27Z

Nice thanks for deploying it! The experience is A-MAZ-ING 😍

I think the only major question I have is around the usefulness of it being full-size. I think context is frequently enough here and probably what we actually want in many cases. Perhaps that makes the size issue a little more swallowable too and/or the compression issues less noticeable?

Few findings:

Small screen sizes it gets a little weird that it goes full screen and completely covers the node, maybe we scale it down a bit so we can shift it to the side?
This is the sort of thing I was worried about with textContent, maybe we actually want innerText or perhaps the 80 character limit is enough?
Sometimes the full-size preview isn't really what we want I think, we might actually prefer context for where it is on the page? Example where we have a ton of duplicate lighthouse images on the page so I still don't know which one this is talking about.
It wasn't able to get the screenshot for these two links here, not sure whats going on with that
Another small screen size failure case where the important part of the screenshot is outside the window.

mattzeunert · 2019-05-15T16:49:59Z

This is the sort of thing I was worried about with textContent, maybe we actually want innerText or perhaps the 80 character limit is enough?

Addressed in this PR.

It wasn't able to get the screenshot for these two links here, not sure whats going on with that

They have a visible size of 0x0px – do you think we should leave some kind of indication if there's no screenshot?

Sometimes the full-size preview isn't really what we want I think, we might actually prefer context for where it is on the page? Example where we have a ton of duplicate lighthouse images on the page so I still don't know which one this is talking about.

Maybe if the element is taller than 60px we try to show where on the page it is and zoom out by 2x or 4x, and for smaller ones we do something similar to what we have now?

Small screen sizes it gets a little weird that it goes full screen and completely covers the node, maybe we scale it down a bit so we can shift it to the side?

I changed the CSS so that the hover always appears above the table item. Let's see how the zoomed out stuff from above works and see if it can help with this too.

What do you mean by "shift it to the side"?

Another small screen size failure case where the important part of the screenshot is outside the window.

Will do this:

if there's a hover contain it within the window
if the page is narrower than say 800px users have to tap the line with the element

connorjclark · 2020-04-13T23:50:29Z

hey @mattzeunert we're wanting to land this (mostly for #10517) and willing to invest eng time to get it the rest of the way. mind if we take it from here, or would you be able to pick this back up?

I'll go through the PR tomorrow and leave my thoughts.

mattzeunert · 2020-04-14T08:12:02Z

Hey @connorjclark! Having someone on the team pick it up sounds good 👍

connorjclark · 2020-04-24T23:46:58Z

I forked and started work here: https://github.com/GoogleChrome/lighthouse/compare/elscreens

mattzeunert requested review from brendankenny, patrickhulce and paulirish as code owners May 2, 2019 12:41

vercel Bot deployed to staging May 2, 2019 12:41 View deployment

mattzeunert commented May 2, 2019

View reviewed changes

vercel Bot deployed to staging May 14, 2019 15:51 View deployment

WIP element screenshots

1b3c178

mattzeunert force-pushed the element-screenshots branch from 5480491 to 1b3c178 Compare May 14, 2019 15:52

vercel Bot deployed to staging May 14, 2019 15:52 View deployment

patrickhulce reviewed May 14, 2019

View reviewed changes

mattzeunert added 4 commits May 15, 2019 07:39

Store width/height with FPS

625a2b9

Use correct width for FPS

89d6ac2

Add FPS to sample json

af8c495

Update accessiblity artifacts

f0708c5

vercel Bot deployed to staging May 15, 2019 07:08 View deployment

Update sample json

1d23453

mattzeunert requested a review from exterkamp as a code owner May 15, 2019 07:11

vercel Bot deployed to staging May 15, 2019 07:11 View deployment

mattzeunert added 3 commits May 15, 2019 09:38

Better handle desktop screenshots

2799517

Store fps audit result on details renderer

d473ca1

Positioning fixes

2a7bc86

vercel Bot deployed to staging May 15, 2019 11:40 View deployment

Don't change unrelated emulation settings in FPS gatherer

e0b3edc

Show above table item

a854113

vercel Bot deployed to staging May 15, 2019 16:47 View deployment

Zoom out

151cbcd

vercel Bot deployed to staging May 16, 2019 15:47 View deployment

Capture full-page screenshot

2ab034b

mattzeunert mentioned this pull request May 26, 2019

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

Closed

mattzeunert added 2 commits May 27, 2019 10:44

Don't inline elem screenshot again and again

6bbd1c5

Show modal on click

80bab17

vercel Bot deployed to staging May 27, 2019 09:56 View deployment

Don't zoom out in modal so much

cd23fa5

vercel Bot deployed to staging May 27, 2019 10:01 View deployment

mattzeunert added 7 commits May 29, 2019 19:42

Remove * 2

f53b83c

Link jpeg quality exploration

4d03200

Comment

c9f1593

Remove timing/timestamp from screenshots

012b81e

Add fps gatherer tests

7125d42

Merge branch 'master' into element-screenshots

1ebc438

Merge branch 'full-page-screenshot' into element-screenshots

7ca970d

googlebot added the cla: yes label May 31, 2019

vercel Bot deployed to staging May 31, 2019 07:07 View deployment

paulirish self-assigned this Jun 11, 2019

paulirish added the chillin ❄ label Jul 23, 2019

paulirish assigned egsweeny and unassigned paulirish Jan 27, 2020

paulirish removed the chillin ❄ label Jan 27, 2020

paulirish assigned connorjclark and unassigned egsweeny Apr 13, 2020

paulirish mentioned this pull request Apr 13, 2020

Better fallback for NodeLabel #10421

Closed

connorjclark closed this Apr 24, 2020

connorjclark mentioned this pull request May 6, 2020

report: add full-page-screenshot to experimental config #10716

Merged

connorjclark mentioned this pull request Mar 10, 2021

☂️ Element screenshots #12230

Closed

16 tasks

Conversation

mattzeunert commented May 2, 2019 • edited by paulirish Loading Uh oh! There was an error while loading. Please reload this page.

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!

patrickhulce left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

mattzeunert commented May 15, 2019

Uh oh!

patrickhulce commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattzeunert commented May 15, 2019

Uh oh!

connorjclark commented Apr 13, 2020

Uh oh!

mattzeunert commented Apr 14, 2020

Uh oh!

connorjclark commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mattzeunert commented May 2, 2019 •

edited by paulirish

Loading

patrickhulce left a comment •

edited

Loading

patrickhulce commented May 15, 2019 •

edited

Loading