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

Fonts: consider to set font-display: block #131396

Closed
bpasero opened this issue Aug 23, 2021 · 8 comments
Closed

Fonts: consider to set font-display: block #131396

bpasero opened this issue Aug 23, 2021 · 8 comments
Assignees
Labels
candidate Issue identified as probable candidate for fixing in the next release feature-request Request for new features or functionality themes Color theme issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Aug 23, 2021

As far as I know, we use custom fonts in 3 areas:

  • our Codicon set
  • extension contributed file icons
  • extension contributed product icons

I often see the workbench appear similar to this because fonts were not loaded yet and the browser fallsback to a standard font that doesn't include the used characters, as such glyphs appear:

recording

With font-display: block the browser will not use any fallback and will simply refuse to show the font until loaded (for up to 3s I think):

recording (1)

More info on the available choices are in this great article:

image
(credits: https://sia.codes/posts/making-google-fonts-faster/)

The following locations in code are imho impacted:

I realise that this might be a controversal change, so maybe alternatives can be discussed here as well. One alternative would be to try to load the fonts in parallel to the workbench loading to prevent the flicker alltogether,

@isidorn
Copy link
Contributor

isidorn commented Aug 23, 2021

I prefer not showing anything than the current empty squares which look like rendering glitches and are disturbing.

@aeschli
Copy link
Contributor

aeschli commented Aug 23, 2021

Makes sense to me. I'll make the change.

@aeschli aeschli added this to the August 2021 milestone Aug 23, 2021
@aeschli aeschli added font-rendering Font rendering issues themes Color theme issues feature-request Request for new features or functionality verification-needed Verification of issue is requested and removed font-rendering Font rendering issues labels Aug 23, 2021
@aeschli
Copy link
Contributor

aeschli commented Aug 23, 2021

@bpasero Thanks for the research!
If you have any tricks to get to see the placeholder boxes as long as in your screencasts, please add that for the verifier.

@bpasero
Copy link
Member Author

bpasero commented Aug 23, 2021

Thanks!

Yeah I cheated in my demo: I added code to specifically slow down the loading of ttf and woff. However, it should be relatively easy to be reproducible e.g. in a incognito browser session (where cache is not hit) and/or over a slow network connection.

@bpasero bpasero added verified Verification succeeded verification-found Issue verification failed and removed verified Verification succeeded labels Aug 24, 2021
@bpasero bpasero reopened this Aug 28, 2021
@bpasero
Copy link
Member Author

bpasero commented Aug 28, 2021

I just noticed how file icons where still suffering from this and realised that font-display can only be set on @font-face and not anywhere else. As such, this rule:

cssRules.push(`.show-file-icons .file-icon::before, .show-file-icons .folder-icon::before, .show-file-icons .rootfolder-icon::before { font-family: '${fonts[0].id}'; font-size: ${fonts[0].size || '150%'}; font-display: block; }`);

Needs to move, I guess into here:

cssRules.push(`@font-face { src: ${src}; font-family: '${font.id}'; font-weight: ${font.weight}; font-style: ${font.style}; }`);

@aeschli
Copy link
Contributor

aeschli commented Aug 30, 2021

Oh, yes, my bad. I'll make a PR.

aeschli added a commit that referenced this issue Aug 30, 2021
aeschli added a commit that referenced this issue Aug 30, 2021
@bpasero bpasero removed the verification-found Issue verification failed label Aug 30, 2021
@miguelsolorio
Copy link
Contributor

@aeschli I'll update this in the main codicons repo as well https://github.com/microsoft/vscode-codicons/blob/main/src/template/styles.hbs

@rzhao271 rzhao271 added the candidate Issue identified as probable candidate for fixing in the next release label Aug 30, 2021
@rzhao271
Copy link
Contributor

rzhao271 commented Sep 1, 2021

I found the perfect opportunity to verify this issue: I had Electron build in the background.

@rzhao271 rzhao271 added the verified Verification succeeded label Sep 1, 2021
miguelsolorio pushed a commit to microsoft/vscode-codicons that referenced this issue Sep 1, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
candidate Issue identified as probable candidate for fixing in the next release feature-request Request for new features or functionality themes Color theme issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants
@bpasero @isidorn @aeschli @rzhao271 @sandy081 @miguelsolorio and others