Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report: css var for FPS, move overlay to container #12201

Merged
merged 6 commits into from
Mar 18, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

This will allow us to have multiple reports on a page, each with their own full page screenshot.

Tested in devtools too.

@connorjclark connorjclark requested a review from a team as a code owner March 4, 2021 23:12
@connorjclark connorjclark requested review from adamraine and removed request for a team March 4, 2021 23:12
@google-cla google-cla bot added the cla: yes label Mar 4, 2021
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice solution! Looks good cross browser, as well.

}`;
return styleEl;
static installFullPageScreenshot(el, screenshot) {
el.style.setProperty('--element-screenshot-url', `url(${screenshot.data})`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this go onto .lh-root like the overlay is appended to? Then they could share the variable rather than having to call installFullPageScreenshot twice (and have the 50k or whatever data url duplicated in the DOM)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having to call installFullPageScreenshot twice

I guess once on first render and then again every time the overlay is opened, actually

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was a reason we couldn't do this, but I've forgotten since writing this yesterday. I'll take another look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cl/360815959 for context. in PSI we want to install the overaly above lh-root at the document root (position:fixed has some implications)

so yeah we would prefer the flexibility. (even tho i'd ideally like it scoped at the lh-root level)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lh-root is tough because we don't know such an element at the time of rendering in render-report. that's why we use report-container atm.

could move the "installation" to report-ui-features, but then that means the element thumbnails would not show at all unless report-ui-features is invoked.

Copy link
Collaborator Author

@connorjclark connorjclark Mar 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make an issue for us to one day work out this gordian knot of a DOM we've created... #12254

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think this is as complicated as that, or at least lh-root isn't the cause of the complexity. I was only suggesting it because that's where the overlay is currently inserted.

lh-root is only programmatically used for the overlay currently, otherwise it's purely for CSS selectors. The one other programmatic use is for narrow devtools styling, and since .lh-narrow is exclusively for devtools it should be querying for .lh-devtools (not .lh-root) anyways.

This isn't an emergency but it would be nice to simplify things while it's still paged into everyone's mind instead of kicking it down the road as a P3 and letting lh-root continue to thread through things and create the very gordian knot you're referring to :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to move the overlay element to the report container element, so now we don't have to add a new CSS var for each time we make that. that's a bunch better now.

I still don't have a good answer for the whole lh-root shenangins. PSI doesnt even have lh-container, so paul and I are gonna work around that by having these "install" functions accept an element on where to install things.

@connorjclark connorjclark mentioned this pull request Mar 10, 2021
16 tasks
@connorjclark connorjclark changed the title report: use css variable for element screenshot report: use css variable for element screenshot, move overlay into container Mar 17, 2021
@connorjclark connorjclark changed the title report: use css variable for element screenshot, move overlay into container report: css variable for element screenshot, move overlay to container Mar 18, 2021
@connorjclark connorjclark changed the title report: css variable for element screenshot, move overlay to container report: css var for element screenshot, move overlay to container Mar 18, 2021
@connorjclark connorjclark changed the title report: css var for element screenshot, move overlay to container report: css var for FPS, move overlay to container Mar 18, 2021
@connorjclark connorjclark merged commit 2880956 into master Mar 18, 2021
@connorjclark connorjclark deleted the elem-css-var branch March 18, 2021 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants