Skip to content

report: correctly display CLS in budget table#11209

Merged
connorjclark merged 6 commits into
masterfrom
cls-budget
Aug 6, 2020
Merged

report: correctly display CLS in budget table#11209
connorjclark merged 6 commits into
masterfrom
cls-budget

Conversation

@Beytoven

@Beytoven Beytoven commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

Screen Shot 2020-08-03 at 2 11 44 PM

Fixes #11177

@connorjclark

Copy link
Copy Markdown
Collaborator

Nice! So to be clear, the issue was that the granularity was hiding the over-budget value for CLS?

@Beytoven

Beytoven commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

Nice! So to be clear, the issue was that the granularity was hiding the over-budget value for CLS?

That's part of it. The bigger issue was that the column was defined as 'ms' in the header and so any value in the column gets treated as such. This includes the number being rounded to the nearest 10 by default.

@brendankenny

Copy link
Copy Markdown
Contributor

There's a simpler way of doing this without special-casing budgets, and I apologize it's not documented better. I think this audit is meant to use it for the SpeedIndex budget based on this comment, but it doesn't appear to actually be doing it.

Table headings set the rendered types for the items in their column (e.g. measurement and overBudget set itemType: ms here), but each item can override that rendered type individually by using the object form of an item value instead of the primitive form. An example of this is renderCode(), which can be invoked for a table value:

So basically the CLS item needs to be something like {type: 'numeric', value, granularity: 0.01}. Unfortunately, this object form of numeric isn't something detailsRenderer knows how to render yet, so you'll need to

@connorjclark

Copy link
Copy Markdown
Collaborator

Won't this be a breaking change?

@brendankenny

Copy link
Copy Markdown
Contributor

It'll be a new type, so it'll fall back to the less good unknown details type in old renderers, but it's a new feature, not a breaking change. It would be the same for a BudgetValue, too.

@connorjclark

connorjclark commented Aug 3, 2020

Copy link
Copy Markdown
Collaborator

I meant for either the current change or just doing what you suggested: programmatic consumers of budgets likely aren't expecting overBudget to be an object. Does this meet our threshold of a breaking change?

@patrickhulce

Copy link
Copy Markdown
Collaborator

Does this meet our threshold of a breaking change?

Given that it's not adding a new details renderer type, I would say probably not requiring a major version rev, but it definitely could break some integrations (the way Lighthouse CI processes resource budgets would break if we did this there) so worth giving a heads up to our big programmatic consumers. If it were close enough to 7.0 I would probably say it's worth waiting :)

I think we once threw around the idea of creating a locked issue that interested consumers could subscribe to for notable changes like this?

@brendankenny

Copy link
Copy Markdown
Contributor

Given that it's not adding a new details renderer type, I would say probably not requiring a major version rev, but it definitely could break some integrations

We say details aren't generally part of our breaking changes, but there probably are exceptions for specific audits (budgets, metrics, etc) that people might reasonably rely on raw access staying stable.

OTOH, this is arguably a bug fix in line with the published details types, and people relying on the buggy behavior will have to adapt :)

Waiting if we think it's going to break someone or just relying on a strong notification of the fix both SGTM.

Comment thread lighthouse-core/audits/timing-budget.js Outdated
}
return budget.timings.map((timingBudget) => {

for (const timingBudget of budget.timings) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

<3

Comment thread lighthouse-core/report/html/renderer/details-renderer.js Outdated
Comment thread lighthouse-core/audits/timing-budget.js Outdated
}).sort((a, b) => {
return (b.overBudget || 0) - (a.overBudget || 0);

if (metricName === 'cumulative-layout-shift') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's unfortunate how complex this has made this fn + the sort at the end.

wb ~L167, where tableItems is called, we filter the results and replace the CLS measurement value instead ?

@connorjclark connorjclark changed the title report: correctly display CLS values in budget table report: correctly display CLS in budget table Aug 6, 2020
@connorjclark connorjclark merged commit 5b9a37a into master Aug 6, 2020
@connorjclark connorjclark deleted the cls-budget branch August 6, 2020 19:49
radum added a commit to radum/lighthouse that referenced this pull request Aug 13, 2020
* upstream/master: (42 commits)
  docs: add Code of Conduct to project (GoogleChrome#11212)
  docs(readme): add related project: lighthouse-viewer (GoogleChrome#11250)
  core(font-size): remove deprecated DOM.getFlattenedDocument (GoogleChrome#11248)
  misc: fix typo in method name (GoogleChrome#11239)
  i18n: make double dollar validation less strict (GoogleChrome#10299)
  misc: rephrase comments to be more inclusive (GoogleChrome#11228)
  misc: tweak gcp scripts to work in google corp (GoogleChrome#11233)
  v6.2.0 (GoogleChrome#11232)
  report: correctly display CLS in budget table (GoogleChrome#11209)
  report: vertically center thumbnails (GoogleChrome#11220)
  i18n: import (GoogleChrome#11225)
  tests: istanbul ignore inpage function (GoogleChrome#11229)
  deps(snyk): update script to prune <0.0.0 and update snapshot (GoogleChrome#11223)
  core(stacks): timeout stack detection (GoogleChrome#11172)
  core(config): unsized-images to default (GoogleChrome#11217)
  core(image-elements): collect CSS sizing, ShadowRoot, & position (GoogleChrome#11188)
  core: add FormElements gatherer (GoogleChrome#11062)
  new_audit: report animations not run on compositor (GoogleChrome#11105)
  tests: update chromestatus expecatations (GoogleChrome#11221)
  deps: update dot-prop secondary dependency (GoogleChrome#11198)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLS with Performance Budgets (budget.json) is always 0 ms in Lighthouse's HTML Report

6 participants