misc(treemap): first pass

[connorjclark]

ref #11254

https://lighthouse-h1odnuuh7.vercel.app/gh-pages/treemap/?debug

[patrickhulce]

nice!! 🎉

thinking of expanding on the awesome puppeteer test shell you added when split up? :D

[connorjclark]

There's not a lot of UI stuff to test against (that wouldn't simply be testing the treemap library). There should be once more of the UI is added.

I added some unit tests.

[paulirish]

i made a lil snippet script to evaluate how much skew the visualization introduces to the data.

console.clear();
root = $('.webtreemap-node--root');
rootArea = root.getBoundingClientRect().width * root.getBoundingClientRect().height;
totalSkew = 0;

hmm = $$('.webtreemap-node').map(elem=>{
    const caption = elem.querySelector('.webtreemap-caption').textContent;
    const area = elem.getBoundingClientRect().width * elem.getBoundingClientRect().height;
    const areaPct = area / rootArea * 100;
    const textPct = caption.match(/\((\d+)%/)[1];
    const skew = parseInt(textPct) - areaPct;

    totalSkew += Math.abs(skew);
    
    return {
        elem,
        caption,
        skew,
        textPct,
        areaPct,
    };
}
);

console.table(hmm);

console.log('total skew', totalSkew);

and adding border-box makes the skew worse, so we probably shouldn't roll with it now.

[paulirish]

one of the changes i made is to expose a webtreemap.layout method that'll relayout an existing treemap, instead of rebuilding the entire DOM again.

so i'd recommend doing that for a lil perf win.

(it was substantial with bigass trees)

This also means you wont have to updateColors on a resize.

[connorjclark]

done, but still need to update colors, newly added nodes (b/c of bigger viewport) need to be colored too.

[paulirish]

THEMS FIGHTING WORDS. 🥊

[paulirish]

we end up labeling correctly with KiB/MiB, so maybe just rename these to match.

[connorjclark]

lol I had to remind myself by reading myself https://hoten.cc/blog/kb-vs-kb/

k

[paulirish]

do these function effectively like tabs?

[connorjclark]

You'd have to elaborate what features constitute a tab. They are single selection, and selecting one will necessarily re-create the entire treemap DOM.

Should they be a radio input or something?

[paulirish]

just trying to understand how this works. (hard for me since theres only 1 mode currently) in short: yes they are like tabs that change the main content of the page.

radio button semantics are correct, as well... though I don't think we get a lot out of changing it to radios at this point :)

[connorjclark]

(can't figure out how to reply inline to paul's comments on naming refactors ... thx GH)

I've been meaning to refactor the naming structure for a while, been punting on that. I'm not sure why RootNodeContainer came into being. I collapsed all of that to just Node. root still remains but only for variable names to denote that the node contained is the top of the given tree. Is that more readable?

I'm hesitating on using name->src suggestion for the "root node" because I don't think that will hold for making a root Node to represent things like resource type breakdown.

I suppose the details of script-treemap-data could be changed to:

Array<{src: string, node: LH.Treemap.Node}>

where node is the "top node" as defined in makeRootNodeFromScriptWithSourceMap (that is, it's identifier isn't the script src, but instead is the top node conceptually from the map's sources file hierarchy, post any collapsing). This keeps the Node interface general, but would allow the treemap app to display the script src in a way other than as a layer in the node visualization (already possible, but requires assumptions/tree manipulation).

EDIT: I'm realizing this is basically what you've suggested, but I'm trying to emphasize that I want the treemap app render function to work for cases where a script source or file source is the wrong abstraction (like the resource sizes example I keep coming back to).

[paulirish]

re: naming and "root" nodes

Let's back up a bit.

We're generating all this data to visualize in a treemap. We create a tree of nodes. Due to the intended UX, we have this hierarchy:

Working from the inside…

• the depth-2 green node represents the effective sourceRoot of a sourcemap (after path separator splitting and collapsing). it's commonly webpack://. it doesn't exist if there's no sourcemap. its resourceBytes/size are always the same as its red parent.
• the depth-1 red node represents a network script. (inline scripts are combined into a single one of these)
• the depth-0 yellow node parents the entire "group" of these network scripts. this node is passed into webtreemap for rendering.

early on in excution, audits/script-treemap-data has no idea the yellow node even exists, so it's ~fine calling the red node a "root node". But over in treemap/main, we have both.

lighthouse/lighthouse-treemap/app/src/main.js

Lines 109 to 110 in 4521501

	const rootNodes = this.rootNodesByGroup[group];
	this.currentRootNode = this.wrapNodesInNewRootNode(rootNodes);

And that's not only confusing but violates how tree data structures are defined.

we still need some changes here.

I'm having a hard time parsing your comment above for the same reasons. "top node" vs "root node"... 🤔

.....

I'm trying to emphasize that I want the treemap app render function to work for cases where a script source or file source is the wrong abstraction

This makes sense to me and thanks for pointing it out. its useful to know your goals.

render() already seems satisfied by this requirement and show() doesn't seem so far off either. (more on that in another comment)

.........

I've been trying out some renames.. These feel distinctive and are flexible for the current and future usecases:

• currentRootNode aka D0 yellow node.
  • Suggesting currentTreemapRoot. It'll always be what we pass into treemap.render() so the "treemap" bit identifies well.
  • TBH i'd also be fine with using "group" in the name: ~currentGroupRoot. But I understand you have future plans to pass specific non-group nodes to render().
• topNode in script-treemap-data aka D2 green nodes.
  • how about sourceRootNode? these are totally specific to sourcemaps so let's name it so? admittedly contains "root" but by coincidence.
• rootNodes in script-treemap-data (currently shown as D1 red nodes)
  • all this data is specific to "scripts" so let's call them scriptNodes?
• rootNodes in main.js aka D1 red nodes.
  • Currently, each one maps to a "script" or "file" (due to inline script merging). I'd be fine with both. But the resourceType breakdown admittedly messes with this as each node will map to a resource type (Stylesheets, XHRs) and contain children which are ~"files".
  • Even renaming currentRootNode it'd still be massively confusing to keep root on these.
  • How about d1Nodes? Not compelling, but it's clear. primaryNodes or keyNodes are also decent. blockNodes was an early okayish idea.

---

I'm not sure why RootNodeContainer came into being. I collapsed all of that to just Node.

this didn't make sense to me either. nice cleanup. 👍

---

(A few other responses I tried to place in the code. Hopefully we don't have to fight the Github UX too much.)

[paulirish]

just spitballing:

it seems reasonable for render() to be passed the currentRootNode arg instead of reading it off the instance.

Also the semantic line between show() and render() isn't obvious and could definitely be shifted. (or the two merged)

i'm thinking ahead to where we want to show different modes.. I could see multiple separate invocations of render()/show().. perhaps it's being passed the array of "root nodes" instead and constructs this wrapper node itself.

[paulirish]

I suppose the details of script-treemap-data could be changed to: Array<{src: string, node: LH.Treemap.Node}> where node is the "top node" as defined in makeRootNodeFromScriptWithSourceMap (that is, it's identifier isn't the script src, but instead is the top node conceptually from the map's sources file hierarchy, post any collapsing).
This keeps the Node interface general, but would allow the treemap app to display the script src in a way other than as a layer in the node visualization (already possible, but requires assumptions/tree manipulation).

hmmm.. if i understand you right... isn't that what you just had with RootNodeContainer??

TBH I was confused as to why that object shape existed, but..

I agree with you that script-treemap-data currently enforces each script has a node. And I guess in the future there may be some circumstance where you don't want a treemap node for a script. And I see the value from a type perspective from staying more neutral..

However, with both resourcetypes & coverage usecases, we still want script-level nodes. Let's not generalize it too early. I'm +1 for not doing this and keeping script-treemap-data returning Array<LH.Treemap.Node>. This can always change in the future.

[paulirish]

aye... i've been adding the undocumented beautify: true in the options which is almost equivalent.

[paulirish]

lgtm