WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 11 hours ago

Last modified 11 hours ago

#45731 closed defect (bug) (fixed)

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

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

Description

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 https://github.com/WordPress/twentynineteen/issues/719 ). 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: https://github.com/WordPress/twentynineteen/pull/728

Attachments (2)

45731.patch (150.5 KB) - added by kjellr 4 weeks ago.
Props: kjellr, mako09, allancole
45731.2.patch (251.5 KB) - added by kjellr 40 hours ago.

Download all attachments as: .zip

Change History (10)

@kjellr
4 weeks ago

Props: kjellr, mako09, allancole

#1 @pbiron
4 weeks 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
3 weeks ago

  • Milestone changed from Awaiting Review to 5.0.3

#3 @laurelfulford
3 weeks ago

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

#4 @laurelfulford
3 weeks 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
2 days 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:

https://codepen.io/laurelfulford/pen/oJOVZj

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).

@kjellr
40 hours ago

#6 @kjellr
40 hours 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: https://developer.mozilla.org/en-US/docs/Web/CSS/:lang

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
11 hours 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
11 hours ago

Thanks for the quick fix, @kjellr!

Note: See TracTickets for help on using tickets.