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

Try to use the system Segoe UI by default instead of downloading a web font #4206

Merged
merged 13 commits into from
Mar 11, 2018

Conversation

christiango
Copy link
Member

@christiango christiango commented Mar 7, 2018

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

Right now the default styles set for fabric text is "'Segoe UI Web (West European)', 'Segoe UI'". Since the web font is first, we always will download the web font. This is a waste of ~100K (even more on browsers that don't support woff2) if the user already has that font installed:

image

With this change, we will make the default font "'Segoe UI', 'Segoe UI Web (West European)'", which by default will try to use the system font, otherwise it will fall back to the webfont. With this change we won't download any web fonts by default for Chrome and FF.

Unfortunately, Edge and IE still download the fonts in this case. To make this work in Edge/IE, we have to change the definition of the Segoe UI Web (West European) font face to include local("Segoe UI").

Focus areas to test

Verified that things look correct in IE, Edge, FF, Chrome.

@christiango christiango requested review from dzearing and MLoughry March 7, 2018 20:00
@christiango christiango requested a review from phkuo as a code owner March 7, 2018 20:00
@christiango
Copy link
Member Author

@dzearing can you spot check this on your macbook for me? Especially if you don't have Segoe UI installed locally.

@dzearing
Copy link
Member

dzearing commented Mar 8, 2018

@mikewheaton @Jahnp What do you think about this? Can we adjust the font ramp here without creating visual artifacts? I think so, screener seems to as well.

@mikewheaton
Copy link
Contributor

We used to prefer the local version of Segoe UI in Fabric Core, but then removed it due to Chrome on Windows not applying the semilight weight correctly. See issue 960.

Here's a CodePen that you can try/modify: https://codepen.io/mikewheaton/pen/rdNVzO?editors=1100

For Chrome on Windows, semilight (font weight 300) text is rendered using the the light weight.
image

image

@@ -163,7 +163,7 @@ exports[`ActivityItem renders compact with an icon correctly 1`] = `
box-sizing: border-box;
color: #666666;
display: flex;
font-family: 'Segoe UI Web (West European)', 'Segoe UI', -apple-system, BlinkMacSystemFont, 'Roboto', 'Helvetica Neue', sans-serif;
font-family: 'Segoe UI', 'Segoe UI Web (West European)', 'Segoe UI', -apple-system, BlinkMacSystemFont, 'Roboto', 'Helvetica Neue', sans-serif;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for Segoe UI to be in the stack twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just to avoid having to update the fallback fonts in a one off case

@christiango
Copy link
Member Author

You are right @mikewheaton , here is an example from the the demo page with and without my changes. The interesting thing is, do we actually need to download 3 variants of the font if we are using font weight anyway? 100KB is more than all of fabric

image

image

@christiango
Copy link
Member Author

Yup, it seems like downloading just regular would break things. Would be great to avoid downloading all 3 fonts just to work around a chrome bug though, it is impacting Office Online's page load time
image

@christiango
Copy link
Member Author

@mikewheaton I have to do a bit more testing, but how does this codepen look: https://codepen.io/anon/pen/wmvGOb?editors=1100

It doesn't download any fonts on Edge and downloads just Semilight on Chrome

@mikewheaton
Copy link
Contributor

@christiango Nice! That looks to be working in Firefox, Chrome, IE11, and Edge on Windows. As long as we get the fallback web fonts working for macOS/iOS/Android/Linux this should do the trick. Once we have a working and tested solution I'll do the same in Fabric Core. :)

@christiango
Copy link
Member Author

Cool! I'll take a look later today on how to do thid in fabric and test on a Linux VM

@christiango
Copy link
Member Author

Hmm the solution from the codepen needs to set the font family to be different based on if the use is semilight, I don't think we do that today anywhere though.

@christiango
Copy link
Member Author

Still need to do some tests in Ubuntu, but this solution means that Chrome downloads Segoe UI Semilight. We have very few components using semilight, but I verified it works by messing around with checkboxes

return {
fontFamily: _getFontFamily(localeCode),
Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored to avoid repeated calls to the loop over the LanguageToFontMap keys

@christiango
Copy link
Member Author

Verified that this works on my Macbook that did not have Segoe UI installed. It downloaded all the fonts as expected

@christiango
Copy link
Member Author

@dzearing Given that this change reduces the download size by 100k in some cases, I'm guessing we are ok with a 75 byte increase in the bundle?

@dzearing
Copy link
Member

75??!! ok finnnne.

@dzearing dzearing merged commit a384191 into microsoft:master Mar 11, 2018
chrismohr pushed a commit to chrismohr/office-ui-fabric-react that referenced this pull request Apr 17, 2018
…b font (microsoft#4206)

* By default, use the system segoe-ui

* Fix it more consistently

* Rush change

* Snapshot updates

* Make it work for IE and Edge as well.

* Update snapshots

* Add some snapshots for the before state of createFontStyles

* Refactor microsoft#1

* Update default font styles

* Another refactor

* Remove unused import

* Update snapshot
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants