-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
new_audit: add preload-fonts audit #11255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned. i went deep on this over here: #11227 (comment)
i think the audit as currently configured doesn't as much impact we wanted. Because 1) few people are using optional
2) preload
is valuable to all webfonts 3) the documented optimizations don't have much to do with preload. :/
So we'll have to get consensus on the change there first. Luckily, I think we'll retain most of this PR anyhow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we'll have to get consensus on the change there first. Luckily, I think we'll retain most of this PR anyhow.
Okay! we can move forward again. We can land this PR with the given scope. It's currently quite narrow, but in order to broaden it, we need a few questions answered first. After that, we may tweak this audit.
All this investigation and discussion also means we'll probably tweak the naming/framing of the audit. I've left those suggestions inline below.
lighthouse-core/test/report/html/renderer/category-renderer-test.js
Outdated
Show resolved
Hide resolved
|
||
// Gets the URLs of fonts where font-display: optional. | ||
const optionalFontURLs = | ||
FontDisplay.findFontDisplayDeclarations(artifacts, PASSING_FONT_DISPLAY_REGEX).passingURLs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had a concern that findFontDisplayDeclarations
is rather expensive. and was thinking we could skip it if we saw no font network requests.
but.. i looked at some timings on LR for lh:audit:font-display
(which also does that work) and it seems plenty quick
90pctl is 12ms! fast.
so yeah ... i'm cool with this cost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hooray for preventative perf checks 💯
Summary
This PR addresses #11227 and adds a new audit that validates if all fonts that use
font-display: optional
were preloaded. Also adds a smoke test for this audit in theperf
smoke test suite.Related Issues/PRs
#11227