Skip to content
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

Merged
merged 54 commits into from
Aug 25, 2020
Merged

new_audit: add preload-fonts audit #11255

merged 54 commits into from
Aug 25, 2020

Conversation

lemcardenas
Copy link
Contributor

@lemcardenas lemcardenas commented Aug 12, 2020

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 the perf smoke test suite.

Related Issues/PRs

#11227

@lemcardenas lemcardenas requested a review from a team as a code owner August 12, 2020 01:15
@lemcardenas lemcardenas requested review from paulirish and connorjclark and removed request for a team August 12, 2020 01:15
Copy link
Member

@paulirish paulirish left a 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.

Copy link
Member

@paulirish paulirish left a 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.

@lemcardenas lemcardenas changed the title new_audit: add jankless-font audit new_audit: add preload-fonts audit Aug 17, 2020
@vercel vercel bot temporarily deployed to Preview August 18, 2020 22:45 Inactive
@vercel vercel bot temporarily deployed to Preview August 25, 2020 16:39 Inactive

// Gets the URLs of fonts where font-display: optional.
const optionalFontURLs =
FontDisplay.findFontDisplayDeclarations(artifacts, PASSING_FONT_DISPLAY_REGEX).passingURLs;
Copy link
Member

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

image

90pctl is 12ms! fast.

so yeah ... i'm cool with this cost.

Copy link
Contributor Author

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 💯

@paulirish paulirish merged commit 764f9f3 into master Aug 25, 2020
@paulirish paulirish deleted the jankless-font-audit branch August 25, 2020 23:41
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.

suggest font-display optional if font is preloaded, to reduce layout shift
6 participants