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: add details-element polyfill for Edge #6465

Merged
merged 6 commits into from
Nov 5, 2018
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Nov 4, 2018

Split this out of #6459 as it's more complex then the other fixes. Hope that's cool, @wardpeet !

Edge doesn't support <details> but it's high priority for them.

Our clients:

DevTools Audits panel: Only Chrome support
Chrome extension: Only Chrome support
CLI -> HTML: Should support all browsers
Viewer: Should support all browsers
PSI: Should support all browsers

Where "All browsers" == current chrome, saf, ff, edge, but not IE11.

Polyfillin

This JS is essentially concatenated into the compiled-reportrenderer via the html-report-assets inclusion. It runs immediately and exits early if a feature test succeeds:

support = {
element: (function() {
var closedHeight, element, openedHeight, parent, ref;
element = document.createElement("details");
if (!("open" in element)) {
return false;
}
element.innerHTML = "<summary>a</summary>b";
element.setAttribute("style", "position: absolute; left: -9999px");
parent = (ref = document.body) != null ? ref : document.documentElement;
parent.appendChild(element);
closedHeight = element.offsetHeight;
element.open = true;
openedHeight = element.offsetHeight;
parent.removeChild(element);
return closedHeight !== openedHeight;
})(),
toggleEvent: (function() {
var element;
element = document.createElement("details");
return "ontoggle" in element;
})()
};
if (support.element && support.toggleEvent) {
return;
}

Shipping this polyfill this way fixes the CLI and Viewer. PSI is configured separately, but has a similar list of files for inclusion. DevTools is also unaffected, so that's cool. Note, this will add the polyfill superfluously to the extension bundle, but... eh.


@wardpeet note i removed these lines:

/* Edge doesn't recognize these, so we preemptively set to display:block */
details, summary {
display: block;
}
I think that's fine as the polyfill will add these styles, and I expect that to happen before we ever render a <details> but lmk wyt :)

@patrickhulce
Copy link
Collaborator

travis is still unhappy about it being require'd from node during collecting strings, maybe we should have an early exit if it's in node?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM other than test issue!

}

}).call(this);
(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what was this one? :)

Copy link
Collaborator

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

@paulirish you still need the details, summary part in the report.

/* Edge doesn't recognize these, so we preemptively set to display:block */
details, summary {
display: block;
}

If you don't. Edge doesn't threat it as block elements. The polyfill only adds the details > summary { display: block; } so you still need to add details { display: block; } to the lighthouse css. Unknown elements are threated as display: inline
image

with details { display: block }
image

Copyright © 2018 Javan Makhmali
*/

(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we ditch this empty function?

}

}).call(this);
(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here?

@paulirish
Copy link
Member Author

sg to all. updated.

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.

You should also try out the "save as html" in viewer (should work fine since it inlines the html-report-assets into the copy of the generator and uses that to save a new version) and just a regular report (saves outerHTML, so may be trickier with polyfilled stuff and the polyfill itself copied over?)

@@ -11,6 +11,8 @@ const REPORT_TEMPLATE = fs.readFileSync(__dirname + '/report-template.html', 'ut
const REPORT_JAVASCRIPT = [
fs.readFileSync(__dirname + '/renderer/util.js', 'utf8'),
fs.readFileSync(__dirname + '/renderer/dom.js', 'utf8'),
// COMPAT: Remove when Microsoft Edge supports <details>/<summary>
Copy link
Member

Choose a reason for hiding this comment

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

can you link the edge bug?

@@ -0,0 +1,295 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I know there's a thing with copying the report renderer files around, but is it possible to support this in the top-level third-party/? It should really be there (it makes it very clear its relationship to lighthouse)

Copy link
Member

Choose a reason for hiding this comment

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

also need a license file in parallel to it, and ideally a readme that says the source commit and lists the modifications made

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do these as a followup. It adds too much complexity to the roll right now.

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.

LGTM

(I think the extension error isn't a real one, but should double check it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants