proposal: report api

[paulirish]

proposing a unified API for the report renderer. (no more subclassing or other weird hooks)

original planning doc: http://go/akqfh 🔒

reviewers: scroll to the bottom of the diff, where the .d.ts is.

Basic idea:

• you create a Renderer instance and have two key methods. getContainerEl() and getTopBarEl()
• We split the report-ui-features along the topbar lines (as i hinted at in Refactor report dom, report ui features, etc. #12254 (comment) )
• there's options for report rendering. the options i added tackle all the weird stuff that PSI/DevTools/Viewer all do.
  • onDetailsItemRendered handles DevTools' lh-node linkifying and source-location resolving
• PSI just calls getContainerEl and if it decides to move the scoregauge and footer to different parts of the DOM, sure. But that's on them.

[patrickhulce]

wasn't clear if this API was meant to be long-lived or a temporary transition but I reviewed as if it were the final answer :)

thanks for compiling the use cases!

[paulirish]

wasn't clear if this API was meant to be long-lived or a temporary transition

meant to be long-lived yup!

[paulirish]

PTAL. It's all been revised

[connorjclark]

ah yea these are all components as found in #12803 . can we use just the component name for the key here?

{
    topbar: HTMLElement;
    main: HTMLElement;
    header: HTMLElement;
    category: HTMLElement[];
    footer: HTMLElement;
}

except lh-category is made via JS and isn't one of our "components" according to #12803 ...

instead of any of this, using a different name here would work for me. ReportPartials ? and then rename renderReportComponents to renderPartialReport or renderReportParts

[connorjclark]

stepping back a bit... why would a client not just be able to call renderFullReport and pluck out what they need?

[patrickhulce]

renderReportPartials SGTM

why would a client not just be able to call renderFullReport and pluck out what they need?

Because the contract is that the element that is returned is a live, LH-owned element that will update itself and if you break it up, all bets are off. If don't want to take over ownership of all interactivity in the report, but need to rearrange some layout, renderReportComponents allows you to place individual live elements wherever you wish.

Given our current embedding system, I think the difference is relatively minor, but that won't be true for flow reports.

[paulirish]

+1 on partials. great call. these are not 1:1 with our new "components" concept so i love using a different term.

[brendankenny]

Because the contract is that the element that is returned is a live, LH-owned element that will update itself and if you break it up, all bets are off. If don't want to take over ownership of all interactivity in the report, but need to rearrange some layout, renderReportComponents allows you to place individual live elements wherever you wish.

The makes total sense, but this also makes me question how much these components can actually be live and update themselves. A footer element I get, but recently we've trended toward more interactivity, and we've had difficulty keeping them encapsulated. Easy examples off the top of my head:

• coordinating scrolling in the categories with the indicator on the sticky header
• we have these individual components, but the screenshot lightbox still goes into overlayParentEl passed in the options and clicking on screenshots opens it? Or should renderReportComponents not get fullpage screenshots?
• (I'm sure there are other examples)

That's what I was getting at above with asking if renderFullReport is going to call this internally: are we going to have two versions of these components, independent mode and in-an-web-app mode? Or everyone uses independent mode and renderFullReport layers on JS between components on top of that (report-ui-features++)?

[patrickhulce]

makes me question how much these components can actually be live and update themselves

coordinating scrolling in the categories with the indicator on the sticky header

Is this a technical feasibility question? Or rhetorical/an objection to an API that returns separate elements that share some global state?

we have these individual components, but the screenshot lightbox still goes into overlayParentEl passed in the options and clicking on screenshots opens it

Is this a request for screenshotOverlay as a partial instead? If so, I like that :)

[brendankenny]

makes me question how much these components can actually be live and update themselves

coordinating scrolling in the categories with the indicator on the sticky header

Is this a technical feasibility question? Or rhetorical/an objection to an API that returns separate elements that share some global state?

Feasibility and/or asking for specifics :) How are we handling inter-component interactions with this API?

Is this a request for screenshotOverlay as a partial instead? If so, I like that :)

That might make the most sense, yeah. But again with how are the inter-component interactions going to be/should be set up?

[patrickhulce]

The specifics vary depending on what deps we can use, but if we're sticking with imperative onlistener changes approach for example...

function renderHeader(containerEl, rootState, setState, onRender) {
  containerEl.innerHTML = `<h1>Header y:<span>${rootState.y}</span></h1>`
  onRender(rootState => containerEl.querySelector('span').textContent = rootState.x)
  return containerEl
}

function renderCategories(containerEl, rootState, setState, onRender) {
  containerEl.innerHTML = `...`
  containerEl.onscroll = () => setState({y: containerEl.scrollTop})
  return containerEl
}

function renderPartials() {
  const rootState = {y: 0}
  const headerEl = document.createElement('div')
  const categoriesEl = document.createElement('div')
  
  const listeners = []
  const onRender = listener => listeners.push(listener)
  const setState = state => {
    for (const listener of listeners) listener(state)
  }

  return {
    header: renderHeader(headerEl, rootState, setState, onRender),
    categories: renderCategories(categoriesEl, rootState, setState, onRender),
  }
}

[brendankenny]

Right, so is the intention that renderReportPartials() would return elements bound to each other to replicate the interactions present in what's generated by renderReport()? That's what I'm asking. If so, can we write that down as part of this API? :)

I think that could absolutely suit us just fine, but this does increase testing surface area, e.g. if the overlay becomes a partial, with the current renderer the screenshot lightbox is removed on click. If someone doesn't use the overlay, every click on a thumbnail would just keep appending new lightboxes to the detached overlay element.

Obviously there can be some DIY/use at your own risk aspect to this, but the more automatic bindings there are, the greater the chance of this type of thing, and the more combinatoric testing we need to do (vs a system of user-controlled hooks between the components where we can get away with relatively more unit and less integration testing).

[connorjclark]

currently there's a some indirection with how "save as gist" code runs ... in base ReportUIFeatures a saveAsGist fn simply throws and error, and Viewer overrides it https://github.com/GoogleChrome/lighthouse/blob/2d796ea/lighthouse-viewer/app/src/viewer-ui-features.js#L54 but that still defers to a passed-in callback function. the impl is _onSaveJson https://github.com/GoogleChrome/lighthouse/blob/2d796ea/lighthouse-viewer/app/src/lighthouse-report-viewer.js#L286

ideally the report/renderer code would just know how to save as gist and clients can enable as they wish, but we wouldn't want to ship firebase and idb to support GithubApi ...

perhaps a better approach would be providing a way to make a custom action in the dropdown menu. then in the options here, instead of a specific "give us a saveGist callback and we will wire it to an otherwise stubbed out and hidden dropdown action", we allow clients to define any action in that menu they want.

[patrickhulce]

perhaps a better approach would be providing a way to make a custom action in the dropdown menu. then in the options here, instead of a specific "give us a saveGist callback and we will wire it to an otherwise stubbed out and hidden dropdown action", we allow clients to define any action in that menu they want.

I like this a lot, but I wonder if that opens us up to more flexibility (and complexity) than we need. The current implementation of dropdown actions calls e.preventDefault and manually handles each data-action. I could see an eventual reportToolsMenuItems: Array<{label: string, iconUrl: string, onClick: () => void, position?: number}> but without additional compelling use cases, I'm in favor of the onSaveGist approach here

[paulirish]

I agree that providing an API to define custom topbar menu actions would be ideal.. but i also agree with patrick that it's overkill for now. so i'd also like to keep this solution low key.

[patrickhulce]

I like this API! so happy to see it taking shape :)

[patrickhulce]

renderReportPartials SGTM

why would a client not just be able to call renderFullReport and pluck out what they need?

Because the contract is that the element that is returned is a live, LH-owned element that will update itself and if you break it up, all bets are off. If don't want to take over ownership of all interactivity in the report, but need to rearrange some layout, renderReportComponents allows you to place individual live elements wherever you wish.

Given our current embedding system, I think the difference is relatively minor, but that won't be true for flow reports.

[patrickhulce]

perhaps a better approach would be providing a way to make a custom action in the dropdown menu. then in the options here, instead of a specific "give us a saveGist callback and we will wire it to an otherwise stubbed out and hidden dropdown action", we allow clients to define any action in that menu they want.

I like this a lot, but I wonder if that opens us up to more flexibility (and complexity) than we need. The current implementation of dropdown actions calls e.preventDefault and manually handles each data-action. I could see an eventual reportToolsMenuItems: Array<{label: string, iconUrl: string, onClick: () => void, position?: number}> but without additional compelling use cases, I'm in favor of the onSaveGist approach here

[brendankenny]

excited about this, but a little unclear about how some of it matches up to implementation. Don't let my skepticism on some particulars sound too negative for the overall approach :)

[brendankenny]

are type and value audit details types?

[brendankenny]

Because the contract is that the element that is returned is a live, LH-owned element that will update itself and if you break it up, all bets are off. If don't want to take over ownership of all interactivity in the report, but need to rearrange some layout, renderReportComponents allows you to place individual live elements wherever you wish.

The makes total sense, but this also makes me question how much these components can actually be live and update themselves. A footer element I get, but recently we've trended toward more interactivity, and we've had difficulty keeping them encapsulated. Easy examples off the top of my head:

• coordinating scrolling in the categories with the indicator on the sticky header
• we have these individual components, but the screenshot lightbox still goes into overlayParentEl passed in the options and clicking on screenshots opens it? Or should renderReportComponents not get fullpage screenshots?
• (I'm sure there are other examples)

That's what I was getting at above with asking if renderFullReport is going to call this internally: are we going to have two versions of these components, independent mode and in-an-web-app mode? Or everyone uses independent mode and renderFullReport layers on JS between components on top of that (report-ui-features++)?

[brendankenny]

is the plan for this to internally call renderReportComponents() instead of building everything in its own flow?

[brendankenny]

who is this for?

[paulirish]

We can close this now as the proposal's design was improved here and most of it implemented.

---

A few items from it are not yet implemented… taking status…

    /** If defined, the 'Save as Gist' item in the topbar dropdown will be shown and when clicked, will run this function. */
    onSaveGist?: (lhr: LH.Result) => string;

onSaveGist was a hook for integration with the viewer. i don't know exactly how we're currently managing this conditional menu item, but this may still be worthwhile.

    /**
     * DOM element that will the overlay DOM should be a child of.
     * Between stacking contexts and z-index, the overlayParentEl should have a stacking/paint order high enough to cover all elements that the overlay should paint above.
     * Defaults to the containerEl, but will be set in PSI to avoid being under the sticky header.
     * @see https://philipwalton.com/articles/what-no-one-told-you-about-z-index/ */
    overlayParentEl?: HTMLElement;

overlayparent was necessary for managing the stacking of the overlay, probably relevant for both devtools and PSI. PSI seems dealt with already, but devtools is still using the old api and may need this.

    /** Callback running after a DOM element (like .lh-node or .lh-source-location) has been created */
    onDetailsItemRendered?: (
      type: LH.Audit.Details['type'],
      el: HTMLElement,
      value: LH.Audit.Details
    ) => void;

onDetailsItemRendered was for devtools' LighthouseReportRenderer.ts items for upgrading lh-node and lh-sourcelocation. Pretty sure we still want this and will need it to order to migrate the devtools report client to the new api.