-
Notifications
You must be signed in to change notification settings - Fork 1
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
System Font Stacks: update to match GitHub #36
Conversation
See some analysis by Federico here: iFixit/react-commerce#626 (comment) As well as some additional references: - https://bitsofco.de/the-new-system-font-stack/ (2016 - very specific on the -apple-system/BlinkMacSystemFont distinction) - https://markdotto.com/2018/02/07/github-system-fonts/ (2018) - https://medium.com/towards-more-beautiful-web-typography/survey-system-font-stack-5f73a3b39776 (2020) - https://css-tricks.com/snippets/css/system-font-stack/ (2022) - https://make.wordpress.org/core/2016/07/07/native-fonts-in-4-6/ (2016) - https://qwtel.com/posts/software/the-monospaced-system-ui-css-font-stack/#fn:10 (2020)
Was
https://developer.mozilla.org/en-US/docs/Web/CSS/font-family#system-ui I still have nightmares of tracking down that Chrome issue with |
https://css-tricks.com/snippets/css/system-font-stack/ confirms:
|
Seems like there might be some internationalization issues with That's a little old, but GitHub and Bootstrap didn't add it back it seems 🤷 |
We're going to find all sorts of conflicting opinions. I'll spend some time today testing. IIRC have a Windows VM installed on one of my computers |
Ughhhh more Blink issues that were patched https://bugs.chromium.org/p/chromium/issues/detail?id=1018581 |
index.json
Outdated
"lato": "Lato, -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji'", | ||
"mono": "'Roboto Mono', Monaco, 'Lucida Console', 'Courier New', Courier, monospace", | ||
"monoSystem": "Monaco, 'Lucida Console', 'Courier New', Courier, monospace", | ||
"sansSystem": "-apple-system, 'Segoe UI', Helvetica, Arial, sans-serif", | ||
"monoSystem": "ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace", |
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.
@davidrans, see https://github.com/iFixit/ifixit/pull/37335, #21, and https://bugs.chromium.org/p/chromium/issues/detail?id=781344&q=BlinkMacSystemFont&can=1&sort=-modified
In short, BlinkMacSystemFont
causes a whole page reflow hurting LCP, FID and CLS of course only in chrome
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.
No good then, that's ~75% of traffic and all those metrics are important to rank, UX, etc
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'm trying to trace back what the problem with BlinkMacSystemFont
was but I'm struggling a bit because the issue is referencing a Dozuki Slack discussion I've no access to.
Moreover, by skimming through the chromium issue, I have the sensation that they are discussing something slightly different:
- They are profiling an animation of the
<text>
block inside the SVG - The performance hit seems to be related to the number of fonts specified, not to using
BlinkMacSystemFont
or not using it.
I've seen that there is a comment from you @jarstelfox but I'm not seeing any CLS by testing the react-commerce branch with the BlinkMacSystemFont
font family on chrome.
Also the other chromium issue referenced in #21 seems not to be reproducible anymore.
Can you please give me a little bit more of context?
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.
Can you please give me a little bit more of context?
💯
Part of the problem is that the issue we were seeing on the website doesn't seem to be well documented. I searched multiple repos, slack and emails to work back to the exact error.
From my understanding, we were seeing a large reflow during page loads and all of our digging led us to the chrome bug.
We were not sure this was the error:
Jarred Stelfox
19:40
Hey I found it. it’s BlinkMacSystemFont
removing that from the critical css fixes the CLS shifting
tailwindlabs/tailwindcss#1402
^ suggested from here
Ian Rohde
19:41
:whoa:
That’s part of the primitive stack. "sansSystem": "-apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif", I can make a new version for it later tonight. IIRC we pulled the stack from github, so I’m good with removing it, especially if it’s causing problems
Jarred Stelfox
19:48
https://www.google.com/search?q=BlinkMacSystemFont+chrome+bug&oq=BlinkMacSystemFont+[…]g&aqs=chrome..69i57j0i22i30.3051j0j7&sourceid=chrome&ie=UTF-8
A quick google search shows chrome hates that font family
https://bugs.chromium.org/p/chromium/issues/detail?id=1057654#c35
^ found the bug!
Actually, I think it’s: https://bugs.chromium.org/p/chromium/issues/detail?id=781344&q=BlinkMacSystemFont&can=1&sort=-modified
tl;dr: They had a series of rendering bugs that happened a year ago. Now they notice a CLS with it.
One other note: https://bugs.chromium.org/p/chromium/issues/detail?id=554590&q=BlinkMacSystemFont&can=2
It looks like chrome intends to deprecate the font family
I know that thread is not the most helpful, but it seems we were hitting some kind of Layout shifting that went away if we removed the font from the stack. What's tricky is that it's an apple font, so you have to be on a mac only to receive the shifting.
Related, but it looks like the chrome team has put time into fixing bugs with this font overall. Maybe it's safe to use now.
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.
BlinkMacSystemFont
may be safe to use now, but is it reliable long-term? My proposal here achieves the same functionality without relying on it.
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.
BlinkMacSystemFont
may be safe to use now, but is it reliable long-term? My proposal here achieves the same functionality without relying on it.
The linked font stack will make MacOS Chrome render either Helvetica or Arial before SF though
we were hitting some kind of Layout shifting that went away if we removed the font from the stack. What's tricky is that it's an apple font, so you have to be on a mac only to receive the shifting.
If you have a repro we can definitely test it out to check if all this is still an issue
@ianrohde I think you are looking for https://bugs.chromium.org/p/chromium/issues/detail?id=781344&q=BlinkMacSystemFont&can=1&sort=-modified
|
How about leveraging the font stack order to resolve:
|
Is there any way to preview these changes before making them? deploy_block 🦖 |
@erinemay the easiest way is to add this line to the body in browser dev tools:
|
un_deploy_block 👍 Am I correct in assuming there's more discussion happening here?
@davidrans, should we go ahead with some cross browser testing to try this out ⬆️? |
Correct 👍 |
I'm throwing on the external block label so it's filtered down on our QA Pulldasher list. @davidrans, let us know when you want a full cross browser test of the site. I'll be out next week, but @k0rvusk0r4x is ready to coordinate it as needed. |
Actually, I was wrong, Bootstrap did add |
Yet seems that after merge someone still experienced issues 😅. I've no windows machine available to test that out though |
Putting Seeing the edge cases that we are discussing though I cannot say for sure that this change is not bringing in other issues |
I tend to trust GitHub and Bootstrap to both have approaches that work for the vast majority of users. I'd favor copying the stacks of some other trustworthy site/library. It sounds like we're down to 2 or 3 possible options. Should we turn it over to QA and let them try to hammer each of those with different OS's, locales, browsers and see if they can find any issues? |
Agree 💯. Clearly if we do experience layout reflows or other issues I agree that we are forced to look for an alternative. |
Bootstrap:
GitHub:
Medium:
I vote we have QA do a thorough test of Bootstrap (i.e. If we don't find issues with either of them, then I don't really have a preference. Maybe a very slight lean towards |
@ianrohde @federicobadini are we good with choosing two of the options listed above (GitHub and Bootstrap, I'm thinking) and having QA take a pass? |
I also lean towards |
Okay, let's QA with GitHub's and Bootstrap's stack and see if we can find any issues with either across OS's/browsers/languages/quantum-portals @erinemay |
Adding final steps here. Add each of these stacks to the body of each page.
Github:
|
Halp! I'm having some trouble with this. I changed all the instances I could find (not just in package.json, which was possibly overkill and maybe my problem?), and after running diffdiff --git a/carpenter-frontend/package.json b/carpenter-frontend/package.json
index 4c2b3acc5e2..9d29ba1aa1e 100644
--- a/carpenter-frontend/package.json
+++ b/carpenter-frontend/package.json
@@ -63,7 +63,7 @@
"@chakra-ui/theme-tools": "~1.3.6",
"@chakra-ui/toast": "^1.5.7",
"@core-ds/icons": "^1.3.3",
- "@core-ds/primitives": "^2.4.5",
+ "@core-ds/primitives": "^2.5.1-rc.0",
"@emotion/css": "~11.10.0",
"@emotion/react": "^11.8.1",
"@emotion/styled": "^11.8.1",
diff --git a/carpenter-js/theme/package.json b/carpenter-js/theme/package.json
index 6a67cbb8a7d..94970c9c96b 100644
--- a/carpenter-js/theme/package.json
+++ b/carpenter-js/theme/package.json
@@ -12,7 +12,7 @@
"clean": "rimraf ./dist"
},
"dependencies": {
- "@core-ds/primitives": "~2.4.5"
+ "@core-ds/primitives": "~2.5.1-rc.0"
},
"devDependencies": {
"rimraf": "^3.0.2",
diff --git a/common/config/rush/pnpm-lock.yaml b/common/config/rush/pnpm-lock.yaml
index f71810c2de3..b03ef683182 100644
--- a/common/config/rush/pnpm-lock.yaml
+++ b/common/config/rush/pnpm-lock.yaml
@@ -23,7 +23,7 @@ importers:
'@chakra-ui/theme-tools': ~1.3.6
'@chakra-ui/toast': ^1.5.7
'@core-ds/icons': ^1.3.3
- '@core-ds/primitives': ^2.4.5
+ '@core-ds/primitives': ^2.5.1-rc.0
'@emotion/css': ~11.10.0
'@emotion/react': 11.9.0
'@emotion/styled': 11.8.1
@@ -225,7 +225,7 @@ importers:
'@chakra-ui/theme-tools': 1.3.6_@chakra-ui+system@1.11.2
'@chakra-ui/toast': 1.5.9_2lf2su2m7ujhsxzxd2z2brassq
'@core-ds/icons': 1.3.3_react@16.14.0
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
'@emotion/css': 11.10.0_@babel+core@7.17.9
'@emotion/react': 11.9.0_insruzqejwml3nexkj22dpur3a
'@emotion/styled': 11.8.1_3gn5s57izm5lhfjxgr4zkvfdti
@@ -461,11 +461,11 @@ importers:
../../carpenter-js/theme:
specifiers:
- '@core-ds/primitives': ~2.4.5
+ '@core-ds/primitives': ~2.5.1-rc.0
rimraf: ^3.0.2
typescript: ~4.6.2
dependencies:
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
devDependencies:
rimraf: 3.0.2
typescript: 4.6.3
@@ -579,7 +579,7 @@ importers:
'@babel/preset-env': ^7.14.4
'@babel/preset-typescript': ^7.13.0
'@core-ds/icons': ^1.3.0
- '@core-ds/primitives': ^2.4.5
+ '@core-ds/primitives': ^2.5.1-rc.0
'@fortawesome/fontawesome-svg-core': ~6.1.1
'@fortawesome/pro-regular-svg-icons': ~6.1.1
'@fortawesome/react-fontawesome': ~0.1.18
@@ -619,7 +619,7 @@ importers:
typescript: ^3.9.9
dependencies:
'@core-ds/icons': 1.3.3_react@16.14.0
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
'@fortawesome/fontawesome-svg-core': 6.1.1
'@fortawesome/pro-regular-svg-icons': 6.1.1
'@fortawesome/react-fontawesome': 0.1.18_3o7rhq5e2fd3auzno3wstxs4ey
@@ -673,7 +673,7 @@ importers:
'@chakra-ui/icons': ^1.0.15
'@chakra-ui/react': ^1.6.6
'@chakra-ui/system': ~2.1.1
- '@core-ds/primitives': ^1.5.0
+ '@core-ds/primitives': ^2.5.1-rc.0
'@emotion/react': 11.9.0
'@emotion/styled': 11.8.1
'@ifixit/react-components': ^0.3.0
@@ -703,7 +703,7 @@ importers:
'@chakra-ui/icons': 1.1.7_43khzrqpf4s2psydq72xkc5sca
'@chakra-ui/react': 1.8.8_xkgxhw3wemj3pp6vzlnnjihwla
'@chakra-ui/system': 2.1.1_bgqmsvm4hz6izcmpcwescmz73y
- '@core-ds/primitives': 1.5.0
+ '@core-ds/primitives': 2.5.1-rc.0
'@emotion/react': 11.9.0_426xlbpu4clsxae4qx32qwknde
'@emotion/styled': 11.8.1_neh35hrqppbvdgjxxnlc3q5icm
'@ifixit/react-components': 0.3.0_lzomchjozpjp3t5kmo7yr7pgeq
@@ -739,7 +739,7 @@ importers:
'@babel/runtime': ~7.16.7
'@chakra-ui/react': ~1.8.6
'@core-ds/icons': ^1.0.0
- '@core-ds/primitives': ^2.4.5
+ '@core-ds/primitives': ^2.5.1-rc.0
'@emotion/react': 11.9.0
'@emotion/styled': 11.8.1
'@fortawesome/fontawesome-svg-core': ~6.1.0
@@ -770,7 +770,7 @@ importers:
'@babel/runtime': 7.16.7
'@chakra-ui/react': 1.8.8_ivoplvjmqgmmcxll5nqusjf7b4
'@core-ds/icons': 1.3.3_react@16.14.0
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
'@emotion/react': 11.9.0_dyau445mh2pdagbkb2hgesg4xi
'@emotion/styled': 11.8.1_kbp7jgi2k3acql37g5mmrjdsjy
'@fortawesome/fontawesome-svg-core': 6.1.1
@@ -848,12 +848,12 @@ importers:
../../runtime_deps_install:
specifiers:
'@core-ds/icons': ^1.3.3
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
imagemarkup: iFixit/node-markup.git#f295d757306141d4658a08d2ebb2475887b52f08
react: 16.14.0
dependencies:
'@core-ds/icons': 1.3.3_react@16.14.0
- '@core-ds/primitives': 2.4.5
+ '@core-ds/primitives': 2.5.1-rc.0
imagemarkup: github.com/iFixit/node-markup/f295d757306141d4658a08d2ebb2475887b52f08
devDependencies:
react: 16.14.0
@@ -6309,12 +6309,8 @@ packages:
react: 16.14.0
dev: false
- /@core-ds/primitives/1.5.0:
- resolution: {integrity: sha512-wOd/FZaVW1laDbY6D63a80xCagmhiqpTgM0/bEJEOhGz39DCBDZUdUmxrWc2N/PfgEFf2CMu3JPiIy0S6GZPHQ==}
- dev: false
-
- /@core-ds/primitives/2.4.5:
- resolution: {integrity: sha512-0gUTtOrGUJdmCRRIQ8JDhNNZz6tf2A9HCouJxfT7aTh3dIIYTTEv5JlVjtVn6Yjtt2sEOMer/7A3vQcWdRQetQ==}
+ /@core-ds/primitives/2.5.1-rc.0:
+ resolution: {integrity: sha512-VuAqx+ow3Pb01r/ggvdBjSNkJ+NE2Cl+5smm9BPRCYvc1nVNiERvQso/TQRKy5MI14W/MgFbeI+WaGlHPzl8yw==}
dev: false
/@cspotcode/source-map-support/0.8.1:
diff --git a/common/config/rush/repo-state.json b/common/config/rush/repo-state.json
index 4762d7b456f..c9b3b3729a1 100644
--- a/common/config/rush/repo-state.json
+++ b/common/config/rush/repo-state.json
@@ -1,5 +1,5 @@
// DO NOT MODIFY THIS FILE MANUALLY BUT DO COMMIT IT. It is generated and used by Rush.
{
- "pnpmShrinkwrapHash": "db6cd466fb0b857a57106e0a1fd0d1f16b03c655",
+ "pnpmShrinkwrapHash": "3df2accb6d47b2213b760792118605d08a1866c2",
"preferredVersionsHash": "f21e10d7a60116c88059eb278beb50276307d038"
}
diff --git a/device-picker/package.json b/device-picker/package.json
index a9dba869548..1ebc3a24e3a 100644
--- a/device-picker/package.json
+++ b/device-picker/package.json
@@ -28,7 +28,7 @@
},
"dependencies": {
"@core-ds/icons": "^1.3.0",
- "@core-ds/primitives": "^2.4.5",
+ "@core-ds/primitives": "^2.5.1-rc.0",
"@fortawesome/fontawesome-svg-core": "~6.1.1",
"@fortawesome/pro-regular-svg-icons": "~6.1.1",
"@fortawesome/react-fontawesome": "~0.1.18",
diff --git a/nextjs-community/package.json b/nextjs-community/package.json
index f6246bc744f..e818ad6e54c 100644
--- a/nextjs-community/package.json
+++ b/nextjs-community/package.json
@@ -13,7 +13,7 @@
"@chakra-ui/icons": "^1.0.15",
"@chakra-ui/react": "^1.6.6",
"@chakra-ui/system": "~2.1.1",
- "@core-ds/primitives": "^1.5.0",
+ "@core-ds/primitives": "^2.5.1-rc.0",
"@emotion/react": "^11",
"@emotion/styled": "^11",
"@ifixit/react-components": "^0.3.0",
diff --git a/product-page-components/package.json b/product-page-components/package.json
index 660e3cdc310..564a7a02049 100644
--- a/product-page-components/package.json
+++ b/product-page-components/package.json
@@ -36,7 +36,7 @@
"@babel/runtime": "~7.16.7",
"@chakra-ui/react": "~1.8.6",
"@core-ds/icons": "^1.0.0",
- "@core-ds/primitives": "^2.4.5",
+ "@core-ds/primitives": "^2.5.1-rc.0",
"@emotion/react": "^11",
"@emotion/styled": "^11",
"@fortawesome/fontawesome-svg-core": "~6.1.0",
diff --git a/react-commerce b/react-commerce
--- a/react-commerce
+++ b/react-commerce
@@ -1 +1 @@
-Subproject commit 4cdb90991ba5479924aa7d9a7db0800f55b919b5
+Subproject commit 4cdb90991ba5479924aa7d9a7db0800f55b919b5-dirty
diff --git a/runtime_deps_install/package.json b/runtime_deps_install/package.json
index 7b2fbd40c1d..0f090801ced 100644
--- a/runtime_deps_install/package.json
+++ b/runtime_deps_install/package.json
@@ -7,7 +7,7 @@
},
"dependencies": {
"@core-ds/icons": "^1.3.3",
- "@core-ds/primitives": "2.4.5",
+ "@core-ds/primitives": "2.5.1-rc.0",
"imagemarkup": "iFixit/node-markup.git#f295d757306141d4658a08d2ebb2475887b52f08"
},
"devDependencies": {
|
@erinemay, the change should only be made in package.json files. It's possible you are running an older version of primitives. Try a search for Past that, there's little I can do to help without access to the repo. 😞 |
Thank you so much @ianrohde and @andyg0808! It worked! I'm seeing the new Lato family on cominor pages, identical to the Bootstrap stack. Depending on the browser, walking down the list and deleting fonts sets each next browser-appropriate font as the font on the page.
@davidrans, where do we use the monoSystem and sansSystem font families? |
We use them in this pull: https://github.com/iFixit/ifixit/pull/41227 |
Hmm, I'm still seeing Lato on that branch. It's the correctly updated version from here when I use |
I can almost guarantee that the lato primitive stack is still being used. If you want to switch iFixit to the system stack, it will take more code changes. I'm not sure if those changes were intended to be a part of this PR. |
Interestingly, GitHub just updated their font stack slightly: https://github.com/orgs/community/discussions/39226 |
"sansSystem": "-apple-system, 'Segoe UI', Helvetica, Arial, sans-serif", | ||
"lato": "Lato, -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Noto Sans', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji'", | ||
"mono": "'Roboto Mono', ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace", | ||
"monoSystem": "ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace", |
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.
"lato": "Lato, -apple-system, BlinkMacSystemFont, 'Segoe UI', 'Noto Sans', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji'", | ||
"mono": "'Roboto Mono', ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace", | ||
"monoSystem": "ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace", | ||
"sansSystem": "-apple-system, BlinkMacSystemFont, 'Segoe UI', 'Noto Sans', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji'", |
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.
CR ☕ |
b6c337e
to
61b4999
Compare
See some analysis by Federico here: iFixit/react-commerce#626 (comment)
As well as some additional references:
on the -apple-system/BlinkMacSystemFont distinction)
Merge checklist
npm version [patch | minor | major]
from the command line (if applicable).