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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b07ed53
report: use css variable for element screenshot
connorjclark Mar 4, 2021
84b83a1
Merge remote-tracking branch 'origin/master' into elem-css-var
connorjclark Mar 17, 2021
defa44c
Merge remote-tracking branch 'origin/master' into elem-css-var
connorjclark Mar 17, 2021
cbb9212
ok brrrr
connorjclark Mar 17, 2021
e825d4d
comment
connorjclark Mar 17, 2021
b78c835
Merge remote-tracking branch 'origin/master' into elem-css-var
connorjclark Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-core/gather/gatherers/full-page-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ class FullPageScreenshot extends Gatherer {
const data = 'data:image/jpeg;base64,' + result.data;

return {
data,
width,
height,
data,
};
}

Expand Down
18 changes: 8 additions & 10 deletions lighthouse-core/report/html/renderer/element-screenshot-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,15 @@ class ElementScreenshotRenderer {
}

/**
* Called externally and must be injected to the report in order to use this renderer.
* @param {DOM} dom
* Called by report renderer. Defines a css variable used by any element screenshots
* in the provided report element.
* Allows for multiple Lighthouse reports to be rendered on the page, each with their
* own full page screenshot.
* @param {HTMLElement} el
* @param {LH.Artifacts.FullPageScreenshot['screenshot']} screenshot
*/
static createBackgroundImageStyle(dom, screenshot) {
const styleEl = dom.createElement('style');
styleEl.id = 'full-page-screenshot-style';
styleEl.textContent = `
.lh-element-screenshot__image {
background-image: url(${screenshot.data})
}`;
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 •
edited
Loading

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.

}

/**
Expand Down Expand Up @@ -159,6 +156,7 @@ class ElementScreenshotRenderer {
if (!el) return;

const overlay = dom.createElement('div', 'lh-element-screenshot__overlay');
ElementScreenshotRenderer.installFullPageScreenshot(overlay, fullPageScreenshot.screenshot);
rootEl.append(overlay);

// The newly-added overlay has the dimensions we need.
Expand Down
9 changes: 5 additions & 4 deletions lighthouse-core/report/html/renderer/report-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,6 @@ class ReportRenderer {
const detailsRenderer = new DetailsRenderer(this._dom, {
fullPageScreenshot,
});
const fullPageScreenshotStyleEl = fullPageScreenshot &&
ElementScreenshotRenderer.createBackgroundImageStyle(
this._dom, fullPageScreenshot.screenshot);

const categoryRenderer = new CategoryRenderer(this._dom, detailsRenderer);
categoryRenderer.setTemplateContext(this._templateContext);
Expand Down Expand Up @@ -269,9 +266,13 @@ class ReportRenderer {
reportFragment.appendChild(reportContainer);
reportContainer.appendChild(headerContainer);
reportContainer.appendChild(reportSection);
fullPageScreenshotStyleEl && reportContainer.appendChild(fullPageScreenshotStyleEl);
reportSection.appendChild(this._renderReportFooter(report));

if (fullPageScreenshot) {
ElementScreenshotRenderer.installFullPageScreenshot(
reportContainer, fullPageScreenshot.screenshot);
}

return reportFragment;
}
}
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/report/html/report-styles.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.