report: flexible viewport for element screenshot overlay #11843
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
| if (reportEl.classList.contains(screenshotOverlayClass)) return; | ||
| reportEl.classList.add(screenshotOverlayClass); | ||
| const topbarEl = dom.find('.lh-topbar', dom.document()); | ||
| const containerEl = topbarEl.parentElement; |
There was a problem hiding this comment.
ah so this should be the .lh-report in the standalone report, but might be different in DevTools/PSI?
There was a problem hiding this comment.
nope. lh-report is inside lh-container.
standalone report:
CDT:
There was a problem hiding this comment.
for whatever reason we have a <main> in standalone. and that elem isn't there for devtools. (not sure about PSI, but probably one of those)
we could be normalizing this DOM structure a bit more but.. yah it's currently not.
| height: dom.document().documentElement.clientHeight * 0.75, | ||
| }; | ||
| if (dom.isDevTools()) { | ||
| maxLightboxSize.width = containerEl.clientWidth; |
There was a problem hiding this comment.
is there a reason we don't always do the Math.min of these? is the below correct?
In devtools:
- the container is the inner area available to our pane
- the document is the entire devtools area
In standalone:
- the container is the very tall lh-report element
- the document is the window
There was a problem hiding this comment.
is the below correct?
yes
is there a reason we don't always do the Math.min of these?
That would work. I find this easier to reason about though. Although using the min would possible benefit other embedders..is that your goal here?
| overlay.remove(); | ||
| }); | ||
|
|
||
| reportEl.appendChild(overlay); | ||
| containerEl.insertBefore(overlay, topbarEl.nextElementSibling); |
There was a problem hiding this comment.
there seems to be significance to the fact that the overlap comes immediately after the topbar. if so, comment? if not, stick with appendChild on the container?
There was a problem hiding this comment.
ok
if not, stick with appendChild on the container?
It doesn't render if placed after lh-container.
| height: 100vh; | ||
| z-index: 1; | ||
| z-index: 1001; |
There was a problem hiding this comment.
| z-index: 1001; | |
| z-index: 2000; /* .lh-topbar is 1000 */ |
| @@ -1549,12 +1549,11 @@ | |||
| outline: 2px solid var(--color-lime-400); | |||
| } | |||
| .lh-element-screenshot__overlay { | |||
| position: fixed; | |||
| position: sticky; | |||
There was a problem hiding this comment.
this does fix some weird layout business in devtools but it shifts the content down so its not longer behind the overlay
before / after
i think there's an alternate approach that handles this and also sorts out the bugs.
i'm noodling on it over here………
| if (reportEl.classList.contains(screenshotOverlayClass)) return; | ||
| reportEl.classList.add(screenshotOverlayClass); | ||
| const topbarEl = dom.find('.lh-topbar', dom.document()); | ||
| const containerEl = topbarEl.parentElement; |
There was a problem hiding this comment.
for whatever reason we have a <main> in standalone. and that elem isn't there for devtools. (not sure about PSI, but probably one of those)
we could be normalizing this DOM structure a bit more but.. yah it's currently not.
This resolves a bug where the overlay was not properly sized in CDT. Instead of using the document window is devtools to know how to center the overlay, for CDT we should use the container element size. Also moved the overlay element to be right after the topbar, and had to make it sticky.
To test you need to patch this CL to a checkout of devtools frontend, and run: