Use `system-ui` instead of deprecated `BlinkMacSystemFont`

Thanks for the patch.

1) Checked the 9.1 dev code and found BlinkMacSystemFont in the following files, and didn't find any usage of "system-ui".

• core/themes/claro/css/base/elements.css
• core/themes/claro/css/base/variables.pcss.css
• core/themes/claro/css/theme/ckeditor-dialog.css
• vendor/phpunit/php-code-coverage/src/Report/Html/Renderer/Template/css/bootstrap.min.css
• vendor/symfony/debug/ExceptionHandler.php
• vendor/symfony/error-handler/Resources/assets/css/error.css

2) Patch applies cleanly to 9.0 and 9.1:

[mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3117225-2.patch 
patching file core/themes/claro/css/base/elements.css
patching file core/themes/claro/css/base/variables.pcss.css
patching file core/themes/claro/css/theme/ckeditor-dialog.css

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3117225-2.patch 
patching file core/themes/claro/css/base/elements.css
patching file core/themes/claro/css/base/variables.pcss.css
patching file core/themes/claro/css/theme/ckeditor-dialog.css

3) After applying the patch, BlinkMacSystemFont only showed up in the vendor files.

4) I'm not a frontend developer so did some googling and found a post from 2017 that says:

Chrome and Safari have recently shipped “system-ui” which is a generic font family that can be used in place of “-apple-system” and “BlinkMacSystemFont” in the following examples.

in https://css-tricks.com/snippets/css/system-font-stack/.

And found others who are making the switch:

https://github.com/primer/css/issues/838
https://github.com/sindresorhus/modern-normalize/issues/18

Based on this, marking this RTBC.

Thank you @Kristen Pol! That is an excellent documentation on your review. I added commit credit for you to make sure that you get credited once this gets committed.

I think we should still do some manual testing on browsers we currently support to make sure that there aren't any regressions.

Good point! What would you like manually tested? Just generally "click around"?

The patch was clean applied, and apparently the font looks the same, see:

I am using Chrome in your version 81.0.4044.92, if needed I can to do tests in others browser.

@martijn.cuppens thank you! I'm wondering if you have any insights on what might have addressed https://infinnie.github.io/blog/2017/systemui.html between now and https://github.com/twbs/bootstrap/pull/22377?

Maybe we should prioritize this issue since it seems like BlinkMacSystemFont is not working properly on Chrome 81. There's an issue for this in the Chromium bug tracker. There's also a blog post with some more screenshots of the regression.

The chrome bug mentioned in #12 impacts both BlinkMacSystemFont and system-ui, so committing this issue would not be a way of circumventing the bug so there's not much benefit in prioritizing this. I'm unsure if there's much benefit in a target followup, either as I manually confirmed the bug is fixed in Chrome 83 beta, which is scheduled for release on May 19 (8 days) which is potentially faster than it would be to reach an agreement on an alternative font approach that hasn't yet been vetted/discussed

IMO switching away from BlinkMacSystemFont seems like a fine approach (since it's an old approach) even if it doesn't address the bug noted in #12.

Based on:

1) Patch has been reviewed

2) Patch has been manually tested

3) People are moving away from BlinkMacSystemFont

I am marking this RTBC so the core maintainers can make the final call.

Although not marked as a bug, I worked on this as part of the Bug Smash Initiative.

I think we need some more manual testing to address #11. The root cause for the revert in Bootstrap has been explained here https://infinnie.github.io/blog/2017/systemui.html.

Oh! Sorry I missed that. Unfortunately I don't have Windows to test on.

No worries! I realized my comment was a bit vague in the first place. 😄

Microsoft provides free virtual machines for browser testing https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/ if you're interested in trying to test this in Windows environment.

I am looking for issues needing manual testing and found this. But there are no instructions in the IS on how to test this.

Reading the issue I see that there has been manual testing but 'more' needs to be done. The last comment says that this needs to be tested in a windows environment, Is that all that needs to be done?

I looked at the link for windows VMs, there are 6 VMs to choose from. Which one or ones to use? Of course, that assumes I could figure out how to install Drupal in windows.

I suggest an issue summary update with steps to reproduce and what environments testing needs to be done would help.

Sorry I can't help.

Patch Failed, Needs Re-roll

#23 Please review the patch

Verified and tested patch #23.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: admin/appearance
# Inspect element and check the font family on "Warning message
There was a problem checking available updates for Drupal."

Expected Results:
# Use `system-ui` instead of deprecated `BlinkMacSystemFont`

Actual Results:
# Currently `BlinkMacSystemFont` is appearing

Please find attached screenshots.
Can be a move to RTBC.

Re-testing again. Seems like irrelevant test failure

We need to do cross-browser testing to address #18.

Here are results of cross browser testing.
After patch:

Tagging for DrupalCamp Colorado DCCO2021 which is Sunday August 8.
https://2021.drupalcampcolorado.org/contrib-day

This patch can be re-rolled into a Merge Request to make maintainer review easier, thanks!

I've checked the patch in all supported browsers and it both looks visually as expected and uses the system-ui font family as expected.

Re #16 - Bootstrap has no added this back - see https://github.com/twbs/bootstrap/pull/30561 and other tools are making system-ui so it seems the world has moved on.

+++ b/core/themes/claro/css/base/elements.css
@@ -10,7 +10,7 @@
-  font-family: BlinkMacSystemFont, -apple-system, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;
+  font-family: system-ui, -apple-system, Segoe UI, Roboto, Ubuntu, Cantarell, Noto Sans, sans-serif, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;

+++ b/core/themes/claro/css/theme/ckeditor-dialog.css
@@ -38,7 +38,7 @@
+  font: 0.8125rem/1.538em system-ui, -apple-system, Segoe UI, Roboto, Ubuntu, Cantarell, Noto Sans, sans-serif, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;

There's loads of duplicate fonts here. It looks like our preprocessor has an understanding of system-ui and expands that to be:

system-ui, -apple-system, Segoe UI, Roboto, Ubuntu, Cantarell, Noto Sans, sans-serif

Anyhow - all the stuff after it - "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif is going to be ignored.

postcss-preset-env is responsible for #34: https://github.com/csstools/postcss-preset-env

We can disable this by setting system-ui-font-family to false in core/scripts/css/compile.js. Alternatively, we could just specify system-ui alone in our font family lists, and allow PostCSS to provide the fallback list.

If we switch to PostCSS then the changes are:

• -apple-system is added
• Oxygen Sans is removed
• Noto Sans is added
• Helvetica Neue is removed

Not sure what the implications of this change would be.

I think we need to keep -apple-system - given we support Firefox ESR, the current version of which is 91, and according to https://caniuse.com/font-family-system-ui only supports -apple-system and not system-ui.

Applied patch #23 working fine and giving expected result
for ref sharing screenshot....

Removing credit for providing redundant screenshots.

I've been thinking about this on and off for a while and I suggest that the best route forward is to embrace postcss-preset-env instead of providing our own system font list. We already use postcss-preset-env to polyfill other CSS features, and it means that as we move forward and upgrade this package we automatically take the list from an upstream source who are likely better at maintaining it than we are, rather than having to maintain it ourselves.

This patch simply sets --font-family: system-ui; in .pcss.css and lets PostCSS expand this to a full list of system fonts.

+1 to #39. It's better to delegate to a tool we're already using. It seems like the easiest and most maintainable approach.

Here's the source in postcss-preset-env for the curious: https://github.com/csstools/postcss-preset-env/blob/41aa2935f83844e6e2b3...

Copied here as well:

const systemUiFamily = [
	'system-ui',
	/* macOS 10.11-10.12 */ '-apple-system',
	/* Windows 6+ */ 'Segoe UI',
	/* Android 4+ */ 'Roboto',
	/* Ubuntu 10.10+ */ 'Ubuntu',
	/* Gnome 3+ */ 'Cantarell',
	/* KDE Plasma 5+ */ 'Noto Sans',
	/* fallback */ 'sans-serif'
];

Thanks for the new patch and the feedback. Tagging for issue summary update and this still needs manual testing.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Things have moved on, and in Drupal 10 now we no longer support IE11 or other old browsers, so we can just use system-ui: https://caniuse.com/font-family-system-ui

Testing on chrome and firefox I'm not noticing any issues clicking around claro.

Regarding #16 I don't have a windows to test on but do we still think there could be a windows issue after 3 years?

We still need to confirm that #11 isn't the case anymore. The key concern seems to be Windows machines on languages using different default fonts (i.e. Chinese). It was brought up in the issue linked on #33 and that's why it looks like Bootstrap decided to just append their font-stack with system-ui.

Since it's been 2 months moving to PNMI for #11 to be addressed.

Just following up if anyone has been able to verify #11, if no follow up could close out in 3 months.

wanted to bump this 1 last time.

I am still in favour of doing this as per #39 and #45.

#11 is 5 years old, referencing a blog post from 8 years ago; things have moved on since then.

As an example here's a more modern blog post saying the opposite to #11: https://www.highperformancewebfonts.com/read/ditch-BlinkMacSystemFont-an...