report: fix element screenshot position, lifecycle, styles by paulirish · Pull Request #11846 · GoogleChrome/lighthouse

paulirish · 2020-12-17T02:42:02Z

this is an alternative to #11843 and #11844

(and here's the diff against #11843, which may be useful: fps-tweak-viewport...fps-tweak-partytime )

okay a few things..

use fixed positioning to keep the overlay on top of the report content

if our overlay element was placed higher up in the devtools DOM, pos:abs would work. (That's what it used there for their equivalent "glassPane" component).

i don't see a good way to use pos:sticky without a nasty hack like margin-bottom: -100vh; to fix it pushing down the report content.

fixed works well. DOM-wise the position of the overlay doesn't matter since it rolls up to its containing block. It can remain a child of .lh-report so there's no diff, but I slightly prefer it's a child of lh-root so it has more visibility.

overlay positioning aside

There are options but it gets interesting when considering our report within another page/webapp. (eg DevTools).

does overlay go on top of topbar? I think yes, cuz otherwise you can toggle the topbar menu and it displays under the overlay
does overlay go on top of devtools toolbar? I wasn't sure but I checked the DevTools glasspane and it goes on top. Here's an example modal & glasspane in Network panel: rightclick table headers > response headers > manage header columns. (Also their glasspane is transparent, so I've done that too.) Anyway, this all convinced me that the overlay should sit on top of everything.

size the lightbox screenshot relative to the overlay

with the overlay the size we want, we can use its dimensions as input for the maxLightboxSize which goes into the amazing sizing algorithm. that thing is too sweet and handles landscape/portrait viewports so so well.

this means we add the overlay to the dom first and then measure it, but i think that's OK!

small style tweaks on lightbox overlay.

it had 20px margin that it wanted for the thumbnail but was being applied in the lightbox too:

also i added a 1px outline around it, just for visual contrast. (yay for outline not affecting dimensions)

and I lessened the overlay's gray bgcolor a tad. more opacity ~= less dark. and in devtools, it's transparent to match the glasspane.

(obv these are not critical and i can defer till later if desired)

fix lightbox click lifecycle

basically the same thing that #11845 was getting at. The core problem is that we use the .lh-element-screenshot class for both thumbnails and lightbox images.

I kept @connorjclark's "moved the listener to the rootEl, instead of the document". 👍

And after starting with adding an extra if elem.closest(am-i-in-the-lightbox) return; I realized the existing selector could be tweaked.

…oks nice

…shots

patrickhulce

I like this approach 👍

@connorjclark how do you feel about this alternative?

patrickhulce · 2020-12-17T03:01:01Z

-    if (reportEl.classList.contains(screenshotOverlayClass)) return;
-    reportEl.classList.add(screenshotOverlayClass);
+    const topbarEl = dom.find('.lh-topbar', dom.document());
+    const containerEl = topbarEl.parentElement;


want to add the explanation you had over VC for why it conceptually belongs next to the topbar? :)

since your comment I changed this.

and at the top i added this explanation

DOM-wise the position of the overlay doesn't matter since it rolls up to its containing block. It can remain a child of .lh-report so there's no diff, but I slightly prefer it's a child of lh-root so it has more visibility.

Sign in to view

+    containerEl.classList.add(screenshotOverlayClass);

-    dom.document().addEventListener('click', e => {
+    containerEl.addEventListener('click', e => {


Sign in to view

-      const overlay = dom.createElement('div');
-      overlay.classList.add('lh-element-screenshot__overlay');
+      const overlay = dom.createElement('div', 'lh-element-screenshot__overlay');
+      containerEl.insertBefore(overlay, topbarEl.nextElementSibling);


…er.js Co-authored-by: Patrick Hulce <phulce@google.com>

Technotips786

I can not understand it.

connorjclark

QAing now. flushing comments.

connorjclark · 2020-12-17T18:28:21Z

-    };
+    const rootEl = dom.find('.lh-root', dom.document());
+    if (!rootEl) {
+      return console.warn('No lh-root. Overlay install failed.'); // eslint-disable-line no-console


nit: these return <void> calls give me the heebee-jeebies. reminds me of the days of express apps and callback hell. consider return on a separate line?

it is also a pattern that I don't think we do anywhere else.

connorjclark · 2020-12-17T18:30:09Z

+      // Only activate the overlay for clicks on the screenshot *preview* of an element, not the full-size too.
+      const el = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));


Suggested change

// Only activate the overlay for clicks on the screenshot *preview* of an element, not the full-size too.

const el = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));

const thumbnailEl = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));

maybe says same thing but without a comment?

I'm to blame for the comment :) WFM 👍

one way this'll clean up is if we use .lh-element-screenshot--thumbnail and .lh-element-screenshot--lightbox everywhere. i can do this in a followup

connorjclark · 2020-12-17T18:34:08Z

  --image-preview-size: 48px;
  --metric-toggle-lines-fill: #7F7F7F;
  --metrics-toggle-background-color: var(--color-gray-200);
+  --modal-overlay-background: rgba(0, 0, 0, 0.3);


--screenshot-overlay-background? or are you anticipating future modals?

connorjclark · 2020-12-17T18:39:40Z


-    dom.document().addEventListener('click', e => {
+    // Add a single listener to the top container to handle all clicks within (event delegation)
+    rootEl.addEventListener('click', e => {


(not now, but for later)

should we listen to resize so we can re-make an open overlay? things get messy when you dock/undock, but also impacts just resizing the window of a standalone report

connorjclark · 2020-12-17T18:41:41Z

is this expected (not being vertically centered)?

Co-authored-by: Connor Clark <cjamcl@google.com> Co-authored-by: Patrick Hulce <phulce@google.com>

paulirish

is this expected (not being vertically centered)?

it is vertically centered. 😄

(random image edits to prove the same amount of pixels on bottom as top:

.. vertically centered assuming the "overlay positioning aside" bit from the description"

paulirish · 2020-12-17T20:13:58Z

+      // Only activate the overlay for clicks on the screenshot *preview* of an element, not the full-size too.
+      const el = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));


one way this'll clean up is if we use .lh-element-screenshot--thumbnail and .lh-element-screenshot--lightbox everywhere. i can do this in a followup

paulirish · 2020-12-17T21:13:29Z

@Technotips786 i'm looking for something halfway between the prodigy and orbital. any suggestions?

connorjclark and others added 8 commits December 16, 2020 16:23

report: flexible viewport for element screenshot overlay

835beb8

tweak

356fbad

use fixed positioning to keep the overlay on top of the report content

ec33ad4

size the lightbox screenshot relative to the overlay

278d559

tweak styles on lightbox overlay. it shouldnt have margin. outline lo…

3c0b9c5

…oks nice

fix lightbox click lifecycle

0db1258

target thumbnail screenshots instead of thumbnail AND lightbox screen…

880fee5

…shots

overlay bg color. variable and transparent in devtools

26a80f7

paulirish requested a review from a team as a code owner December 17, 2020 02:42

paulirish requested review from connorjclark and removed request for a team December 17, 2020 02:42

google-cla bot added the cla: yes label Dec 17, 2020

devtools-bot assigned connorjclark Dec 17, 2020

devtools-bot added the waiting4reviewer label Dec 17, 2020

paulirish added 2 commits December 16, 2020 18:59

ignore topbar and install the sticky overlay against lh-root

98cf9cb

rename a class i was confused by

35105c5

vercel bot deployed to Preview December 17, 2020 03:00 View deployment

lint

6e19404

vercel bot deployed to Preview December 17, 2020 03:08 View deployment

patrickhulce reviewed Dec 17, 2020

View reviewed changes

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

b921008

…er.js Co-authored-by: Patrick Hulce <phulce@google.com>

vercel bot deployed to Preview December 17, 2020 07:33 View deployment

add a comment like what patrick offered

626b4b0

vercel bot deployed to Preview December 17, 2020 07:34 View deployment

Technotips786 reviewed Dec 17, 2020

View reviewed changes

connorjclark reviewed Dec 17, 2020

View reviewed changes

connorjclark changed the title ~~report: element-screenshot lightbox fixes~~ report: element screenshot overlay fixes Dec 17, 2020

connorjclark changed the title ~~report: element screenshot overlay fixes~~ report: fix element screenshot overlay positioning, lighten background Dec 17, 2020

Apply suggestions from code review

6f0e213

Co-authored-by: Connor Clark <cjamcl@google.com> Co-authored-by: Patrick Hulce <phulce@google.com>

vercel bot deployed to Preview December 17, 2020 20:15 View deployment

feedback

8da1815

vercel bot deployed to Preview December 17, 2020 20:16 View deployment

brendankenny reviewed Dec 17, 2020

View reviewed changes

Comment thread lighthouse-core/report/html/renderer/element-screenshot-renderer.js Outdated

Comment thread lighthouse-core/report/html/renderer/element-screenshot-renderer.js Outdated

no trailing spaces

0d70259

vercel bot deployed to Preview December 17, 2020 20:30 View deployment

paulirish commented Dec 17, 2020

View reviewed changes

connorjclark approved these changes Dec 17, 2020

View reviewed changes

paulirish changed the title ~~report: fix element screenshot overlay positioning, lighten background~~ report: fix element screenshot position, lifecycle, styles Dec 17, 2020

paulirish merged commit 966a206 into master Dec 17, 2020

paulirish deleted the fps-tweak-partytime branch December 17, 2020 21:12

connorjclark mentioned this pull request Mar 10, 2021

☂️ Element screenshots #12230

Closed

16 tasks

		// Only activate the overlay for clicks on the screenshot preview of an element, not the full-size too.
		const el = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));

	// Only activate the overlay for clicks on the screenshot preview of an element, not the full-size too.
	const el = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));
	const thumbnailEl = /** @type {?HTMLElement} */ (target.closest('.lh-node > .lh-element-screenshot'));

Conversation

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

Uh oh!

use fixed positioning to keep the overlay on top of the report content

overlay positioning aside

size the lightbox screenshot relative to the overlay

small style tweaks on lightbox overlay.

fix lightbox click lifecycle

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Dec 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!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Technotips786 left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

connorjclark commented Dec 17, 2020

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!

paulirish commented Dec 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

paulirish commented Dec 17, 2020 •

edited

Loading

paulirish Dec 17, 2020 •

edited

Loading