report: refactor dom structure

[paulirish]

partial fix of #12254 ..

implementing my proposed DOM structure from here: #12254 (comment)

doing this paves the way to implement #12772

inlining for convenience:

.lh-vars /* .lh-root merges into that. */
    .lh-topbar
    .lh-main /* rename from lh-container, basically */
        .lh-header /* contains sticky-header and header-container (which both have 5 gauges, typically) */
        .lh-categories
        .lh-footer

edit: in fact, now as long as .lh-vars is attached to topbar and header and categories and footer, we're good.

notes:

• lh-root is removed as it basically merges into lh-vars.
  • i could see renaming lh-vars into lh-scope to better indicate its not just variable definitions but also this DOM subtree is intended to be LH report content only. I dont have strong feelings, but if we did it, it'd be in a followup
• report header elements were grouped together under a single lh-header container. (following the proposal).
  • theres still more cleanup that could happen, but I didn't want to muddy the diff too much.
• i kept the .lh-main container for now. i see a nice way to remove it in a followup, that'll also fix a few more things.
• runWarnings werent inside of a "header" element, but now are. visually didn't make a difference, but semantically its better.
• removed the mediaquery stuff for .lh-narrow as that's been unused for a long while.

[patrickhulce]

I'm happy with these name changes aside from the lh-vars callout, but I could live with that.

[patrickhulce]

I don't love the use of lh-vars for these purposes here :/

lh-scope doesn't exactly feel right either. IIRC, we used to put all the lh-vars on lh-root but then one environment didn't end up having an lh-root because it split everything out and we moved it onto lh-vars instead. Will that same problem happen again with lh-scope? Should we just keep both of these? That doesn't actually seem like the worst outcome.

[paulirish]

I just don't want to use 'root' where there are multiple of them. And yeah in the PSI case we have the multiple 'partials' and they all need these classes. I was thinking 'scope' felt good as you can have multiple. and then these L305-325 styles feel a little better on a "scope" than in a var-only thing.

lemme try finding a new home for these styles....

[paulirish]

okay i was able to place them more specifically: 98cf7d6

lg?

[connorjclark]

I know we deleted lh-container, but that seems like a good name for what is now root/vars.

[paulirish]

i just pushed one last change which removes the one place i had been applying actual styles with lh-vars. (this went against the spirit of the name, yeah)

so now we're back to lh-vars only being for providing the vars. :)

maybe its my recency bias, but when i see lh-container i feel like there should be only one, but.. its true its not as explicit as root.

[brendankenny]

i could see renaming lh-vars into lh-scope to better indicate its not just variable definitions but also this DOM subtree is intended to be LH report content only. I dont have strong feelings, but if we did it, it'd be in a followup

just naming wise, I'd put in a vote for lh-scope over lh-vars for this reason, but interested in the answer to @patrickhulce's question about the PSI frontend

[patrickhulce]

I'm still good with this.

Re: PSI point, I hear ya. I just don't have the same mental understanding of a container so multiple is fine :)

[paulirish]

fyi the removal of -webkit-font-smoothing: antialiased; makes the text a tad more bold on the platforms where that property does anything.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[connorjclark]

somethings to work out in devtools first, let's work on a CL at the same time and submit this PR together to reduce churn.

some things we noted on devtools:

(devtools; focus on lack of topbar bg color)

(standalone; there is not topbar bg color issue. here's the style rules)

The order these style rules are applied is different between standalone report and devtools. note the "constructed stylesheet" in devtools but not in standalone:

• in standalone, style.css is add in the head, and createTopbarComponent adds its style in main. CSS application order will ignore bg color from the first, overriden by the second.
• in devtools, style.css is injected at runtime (hence the constructed stylesheet). _renderTopBar does the same as in standalone. However, CSS application order dictates that constructed stylesheets are applied after the document styles. see this

[connorjclark]

first solution that comes to mind is to lean all-in on injecting our styles via JS. as in: convert report/assets/styles.css into a js call that injects the element onto the page. this would also help our embedding story by removing the need to add a separate CSS file.

this also means we can remove the "class name listing" we do in google3...and wouldn't actually need to prefix everything with lh- #12806 ... lol oh well

[patrickhulce]

first solution that comes to mind is to lean all-in on injecting our styles via JS.

I'm supportive of this long-term, we need to be able to rely on the consistency of CSS in multiple environments and varied injection methods prevent that from happening. Other short-term solutions to consider in the meantime...

• split the base reset line into the sets of properties that we actually want to apply (there's no reason for topbar to have this background color set
• increase specificity of these overrides :)

[patrickhulce]

@paulirish you marked this as waiting on reviewer, is there anything specific you need eyes on or just the devtools work @connorjclark raised?

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.