Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#45731 closed defect (bug) (fixed)

Twenty Nineteen: Implement a less aggressive approach for alternate approach for non-latin font fallbacks.

Reported by: kjellr's profile kjellr Owned by: laurelfulford's profile laurelfulford
Milestone: 5.1 Priority: normal
Severity: normal Version: 5.0.2
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: Cc:


For non-latin languages, Twenty Nineteen currently uses a lightweight but aggressive way of implementing font fallbacks. This roughly illustrates the method used:

html[lang="$language"] $global_wrapper * {
	font-family: $font_fallbacks !important;

Using !important here is not ideal, and has already caused broken icons in the classic editor block (as reported in ). It's bound to cause more problems down the line, and should be avoided.

The attached patch uses a different approach, and avoids using !important everywhere.

  • In our sass files, it replaces all of our font-family declarations with a mixin that bundles font fallbacks by default.
  • This mixin uses the :lang() pseudoclass along with a Sass @each directive to generate fallback rules each time a font family is defined. To save space and avoid repetition, these fallback rules are grouped together using a sass placeholder.

This patch was previously tested + approved on Twenty Nineteen's GitHub, but it's a big patch and should definitely be re-tested thoroughly here too.

Original thread:

Attachments (3)

45731.patch (150.5 KB) - added by kjellr 5 years ago.
Props: kjellr, mako09, allancole
45731.2.patch (251.5 KB) - added by kjellr 5 years ago.
45731-Devanagari.patch (752 bytes) - added by mako09 5 years ago.

Download all attachments as: .zip

Change History (18)

5 years ago

Props: kjellr, mako09, allancole

#1 @pbiron
5 years ago

Completely agree!

I just came across this problem when working on a plugin I'm writing. The plugin includes an icon font (very similar to dashicons) and CSS rules for setting that font-family for certain markup it outputs (on the front-end).

I hadn't noticed it until this morning, but Twenty Nineteen 1.1 and 1.0 (as officially released, but not any of the pre-release versions I had tested my plugin with) breaks the display of my plugin's markup because of the aggressive font fallbacks overriding my rules for which markup should use my icon font.

[BTW, I suspect that any plugin which uses dashicons to style things on the front-end would also suffer the same problems].

I have no opinion on the patch, per se, but the problem is REAL and needs to be fixed.

#2 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.0.3

#3 @laurelfulford
5 years ago

  • Keywords needs-testing added
  • Owner set to laurelfulford
  • Status changed from new to assigned

#4 @laurelfulford
5 years ago

  • Milestone changed from 5.0.3 to 5.1

I've started testing this, and can confirm it fixes the underlying issue (the theme styles overriding fonts for non-dashicon icons in the editor).

Given the size of the patch, and the nearness of the next release, I'm going to punt this to 5.1 to make sure I have time to thoroughly test the patch otherwise. I want to make sure I don't miss anything!

#5 @laurelfulford
5 years ago

Thanks for this patch, @kjellr!

As mentioned, this updated approach seems to fix the dashicons issue (yay!). However, I'm noticing some issues with the selectors that have multiple languages.

The ones applied to one language (:lang(zh-TW)) switch to the correct font when I switch the language of the site under Settings. But the ones with multiples (:lang(ar,ary,azb,ckb,fa-IR,haz,ps)) don't seem to apply the styles for me.

I could recreate this outside of the theme -- here's a really basic Codepen that shows the issue:

If you remove the azb from the last selector in the CSS, the ar paragraph will apply the green colour.

I'm not super familiar with :lang(), but I couldn't find any examples online where it's being used with more than one language -- I think this may need to be reworked to separate out the two grouped languages (ar,ary,azb,ckb,fa-IR,haz,ps and be,bg-BG,kk,mk-MK,mn,ru-RU,sah,sr-RS,tt-RU,uk).

5 years ago

#6 @kjellr
5 years ago

Thanks for testing, @laurelfulford. Looks like using comma separated values there is a CSS Level 4 spec, so its support is likely not across-the-board yet:

I'm attaching a new patch (45731.2.patch) that breaks those out. Tested on my end and it seems to be working fine. Mind giving it another try?

Thank you!

#7 @laurelfulford
5 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44650:

Twenty Nineteen: Use a less aggressive approach for non-latin font fallbacks.

The theme's original approach to its non-latin font fallbacks required !important, which overrode fonts that shouldn't have been changed, like the icon fonts used for editor buttons.

Props kjellr, mako09, allancole.
Fixes #45731.

#8 @laurelfulford
5 years ago

Thanks for the quick fix, @kjellr!

#9 @mako09
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

One more language group is forgotten.

Last edited 5 years ago by mako09 (previous) (diff)

#10 @laurelfulford
5 years ago

Oh no -- thank you for catching that, @mako09!

#11 @laurelfulford
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44657:

Twenty Nineteen: Create separate :lang() selectors for Devanagari.

In the non-latin font fallback styles, languages using the Devanagari alphabet were initially committed in a comma-separated list in the :lang() selector, which isn't fully supported. This update separates those languages into their own :lang() selectors.

Props mako09.
Fixes #45731.

#12 @kjellr
5 years ago

Thank you, @laurelfulford and @mako09!

#13 follow-up: @SergeyBiryukov
5 years ago

In 44773:

Twenty Nineteen: Update style-rtl.css after [44657].

See #45731.

#14 in reply to: ↑ 13 ; follow-up: @pbiron
5 years ago

@SergeyBiryukov: Is r44773 ready for 5.1.1?

#15 in reply to: ↑ 14 @SergeyBiryukov
5 years ago

Replying to pbiron:

Is r44773 ready for 5.1.1?

It looks like the affected locales are not RTL, so it shouldn't make a difference for them. I just noticed this after running the build task while working on #46291. The changes to style-rtl.css were apparently missed in [44657].

We could backport [44773] to 5.1.1 for consistency, but I'm also fine with keeping it in 5.2 only.

Note: See TracTickets for help on using tickets.