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

Font-Display check should consider icon fonts #10127

Open
Stanzilla opened this issue Dec 18, 2019 · 10 comments
Open

Font-Display check should consider icon fonts #10127

Stanzilla opened this issue Dec 18, 2019 · 10 comments
Assignees

Comments

@Stanzilla
Copy link

Feature request summary
A special check to not recommend font-display: swap for icon fonts.

What is the motivation or use case for changing this?

There seems to be no clear recommendation on what is actually best practice for icon fonts, and even if there is, it's not swap but actually block. So I would suggest either adding a special rule to to detect icon fonts or maybe whitelist the most common ones so they don't trigger the lighthouse warning when swap is not set.

Relevant links:

How is this beneficial to Lighthouse?

Currently the Lighthouse recommendation is not actual the best practice and might cause people blindly following it to get unintended behaviour.

@patrickhulce
Copy link
Collaborator

Thanks for filing @Stanzilla!

so they don't trigger the lighthouse warning when swap is not set.

The Lighthouse audit is not looking for swap it's looking for any valid font-display value that's not the default. If you follow icon font best practices and use block, the audit will pass.

The Learn More docs are misleading on this point, so we'll update those 👍

@craze3
Copy link

craze3 commented Jan 15, 2020

Here is an example of it currently happening:

example

@patrickhulce
Copy link
Collaborator

@craze3 those font declarations in the stylesheets aren't using the icon font best practice block, that's a valid audit failure.

https://use.fontawesome.com/releases/v5.10.0-11/css/all.css font-display: auto not block
https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css missing font-display completely

@craze3
Copy link

craze3 commented Jan 15, 2020

Oo, thanks @patrickhulce. What's the quickest fix?

.fa,.fab,.fad,.fal,.far,.fas {
   font-display: block;
}

Think that will work ?

@patrickhulce
Copy link
Collaborator

Unfortunately it has to be declared with the font declaration, which if you're using the CDN version is controlled by them and there's no workaround.

It sounds like they plan to address this soon though, see FortAwesome/Font-Awesome#14387 (comment) :)

Also FWIW you don't lose anything on the performance score for this showing up in the audit, and for fonts that are going to change to block it's really just an 'ignore this URL' type of signal anyhow, so feel free to ignore that LH result in the meantime.

@tagliala
Copy link

tagliala commented Jan 15, 2020

Hi 👋👋

Geremia from Font Awesome here

Sorry for the late feedback, I've missed this.

Unfortunately it has to be declared with the font declaration, which if you're using the CDN version is controlled by them and there's no workaround.

...and unfortunately FA 4.7.0 is in EOL and we do not host it directly. Don't know if bootstrapcdn maintainers are interested in doing the fix and release an unofficial version of 4.7.0, anyone interested in having a fix on the old version maybe can ask.

In any case, since the fix cannot use the same version number for caching reasons, probably some plugins I can see in the screenshot must be updated on their turn

@connorjclark

This comment has been minimized.

@benschwarz
Copy link
Contributor

@Stanzilla, Fonts are not recommended for icons they commonly introduce both accessibility and performance issues. I can't think of a reason why Lighthouse should add/update an audit to allow for a practice that is generally not recommended.

WDYT?

Refs:

@Stanzilla
Copy link
Author

I think if it was about adding something, I'd agree. But since the functionality already exists and gets used, I think it should be updated. Just my 2ct.

@yisibl
Copy link

yisibl commented Mar 20, 2021

@PatOnTheBack A digression, is there any tool to test and compare the performance of svg and font icons?

andersk added a commit to andersk/webfonts-generator that referenced this issue Aug 5, 2021
It’s not helpful for the browser to substitute another font for the
icon font while it’s loading.

This suppresses a warning from the Lighthouse performance analyzer:
GoogleChrome/lighthouse#10127 (comment).

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants