Use available font weights and styles in FontAppearanceControl #61915
Conversation
mikachan
added
[Type] Bug
|
Size Change: +840 B (+0.05%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
packages/block-editor/src/components/font-appearance-control/index.js
Outdated
Show resolved
Hide resolved
|
When there is only one font style/weight option available, it looks a little odd having both a "Default" option and the single available option in the dropdown menu, for example: 
In this case, Default is always going to be the same as the single available option. Should we remove the Default option if there is only one font style/weight option? This also doesn't feel ideal as that means the Default option sometimes shows for other font faces, and then disappears when there is only one option. |
|
Thanks so much @vcanales 🙏
I thought I'd tested this and it was working, but I don't think I tried with a variable font that had a limited range of weights. I'm able to reproduce this and I've attempted a fix that compares the formatted font weights and styles from the new |
There was a problem hiding this comment.
I've tested this, trying to switch between different font families in as many ways I can think of: switching the font family to/from a system font, a web font with one font face, a web font with multiple font faces, and variable fonts both with and without italics.
Everything seems to be working as described, mainly:
- ✅ System fonts and variable fonts have all weights and styles available, so switching to them never changes the font appearance style. The same when the web font has a font face with the current appearance style available--it stays the same.
- ✅ Regular web fonts (not variable) show only the appearance styles for the font faces they have activated, so switching to a web font when the appearance style is not available resets the appearance to "Default"
That said, while testing and thinking about the problem more, the way this takes away flexibility gives me some pause at merging it.
First, font weight: showing only the weight options the font has available is generally a good improvement. Before this PR, changing the Appearance from "Thin" to "Medium" or from "Black" to "Bold" for a font that only has Regular and Bold faces available has no effect--the extra choices are just clutter.
However, for a font with only a Regular weight font face active, many browsers will create a bold style as well--it seems you can turn any font bold, and this PR takes away that option.
Additionally, with this PR, changing the font family will reset Appearance to "Default" if the font doesn't have a face that matches exactly. For example, if I want the Appearance to be Bold, but the font only has a Black weight available, the Appearance setting goes back to Default when I select the new font. I loose the Bold setting and have to manually change it every time I change the font if I want to preview a bold-like appearance.
Second, the italic setting: before this PR, it you select italic appearance for a font that doesn't have one, many browsers will automatically create an italic font style. So this change takes away the option to make any font italic from the block settings.
Another reason I find this change awkward is how the Global Styles settings can be different. For example, you can set the default Appearance for a block to be Bold in Global Styles settings. Then when the individual block setting Appearance is "Default", the font is Bold, even when that font doesn't have an Bold font face. But you can't find Bold in the list of Appearance options unless the font has the face installed. I'm worried this inconsistency will cause confusion.
Some potential changes to address these problems:
- Keep the "Bold" Appearance option available for all fonts, even when the font face isn't explicitly installed
- Keep an Italic Appearance option available for each font weight available, even when the font face isn't explicitly installed (e.g. if a font has only Black font weight installed, also show "Black Italic" as an option)
- Try to match the closest weight option available when switching fonts, rather than going back to "Default" (e.g. if the Appearance settings is Bold, and switching to a font that only has Black available, change the Appearance to Black rather than default)
What do you think?
|
Thanks for the extensive testing, @creativecoder! I'm glad to see the green ticks 😄
I believe when browsers apply bold and italic to a regular font, this is called "faux bold" and "faux italic", and sometimes the results are not ideal. For example, some characters will look weird/wrong and may be hard to read, and each browser will apply these styles slightly differently. However, having thought about this more after your comment, I think there are more reasons to include these faux options:
I've included them in d359d9e, let me know what you think!
I think this is a good idea! I've tried this out in ad00060. I've also gone ahead and handled variable font weights in this commit: 0ece8f5. I was working on this anyway as a follow-up and since it seems to be in a good state, I've included it here. Some new testing steps following these changes:
|
Yeah, I have mixed feelings about the faux settings. Ideally you would install the font faces you need, but having bold/italic appearance always available is handy if you just want one thing on the page to look a bit different without the overhead of another font face. I hope that in the future we can help users reconcile this more transparently in some way.
Nice. I noticed this as well, but my review was already getting quite long. Glad to see it added--will test! |
|
I've updated the logic in d7bba77 to only add faux bold if there isn't already a font weight available of 600 or above. In my testing, the addition of faux Bold does not display any differently for a font face that already has a Semi Bold weight or above. With this change faux Bold is only added if it will actually change the appearance. (Of course, it's still dependent on the browser supporting faux bold text). |
There was a problem hiding this comment.
Thanks for the quick updates @mikachan !
With the recent changes, I think this is ready to merge, as this PR now only removes font Appearance options that have no effect. If users are currently depending on faux bold or faux italic options, those will still be available.
- ✅ All fonts have a bold Appearance option, as well as italic options for each weight
- ✅ System fonts have all weights and styles available
- ✅ Regular web fonts (not variable) show only the Appearance styles for the font faces they have activated (plus added bold and italic options)
- ✅ Variable fonts only show Appearance options for the weight range included with the font (plus added italic options)
Ah nice, I didn't realise this about Semi Bold. Thank you!
Woop, thank you for reviewing again! And thank you to everyone who has helped out here! I'll go ahead and merge this 🎉 |
| if ( isSystemFont || ( hasFontStyle && hasFontWeight ) ) { | ||
| setFontAppearance( { | ||
| fontStyle, | ||
| fontWeight, | ||
| } ); | ||
| } | ||
|
|
||
| // Reset font appearance if the font family does not have the selected font style | ||
| if ( ! hasFontStyle ) { | ||
| resetFontAppearance(); | ||
| } |
There was a problem hiding this comment.
@mikachan I noticed here that if isSystemFont is true and if hasFontWeight has a value, then if first calls setFontAppearance to set the font weight, but will straight away call resetFontAppearance and unsets the value that was just set.
The background is that I've been working on some e2e tests (#61932) that programatically set fontWeight a bit like this:
var block = wp.blocks.createBlock('core/paragraph', {
content: 'Hello',
style: { typography: { fontWeight: '300' } } }
);
wp.data.dispatch('core/block-editor').insertBlock( block )But that recently stopped working, and I think it's due to the resetFontAppearance here. To reproduce, if you run that in the console the block that's inserted ends up with no appearance styles.
I'm not familiar with the code, but thought I'd mention it.
There was a problem hiding this comment.
Thanks for highlighting, @talldan! I'll take a look and open a follow-up PR. I'm guessing the hasFontStyle and hasFontWeight combined logic needs to be adjusted.
| @@ -252,6 +255,39 @@ export default function TypographyPanel( { | |||
| setFontAppearance( {} ); | |||
| }; | |||
|
|
|||
| // Check if previous font style and weight values are available in the new font family | |||
| useEffect( () => { | |||
There was a problem hiding this comment.
This effect runs when the component first mounts and will potentially create undo levels when setFontAppearance calls onChange which in turn calls setAttributes. Is that what we want?
I'm also running into a problem with some new code I'm writing where this effect is clobbering style because it's calling onChange on mount with an outdated value.
Is an effect necessary? Could the logic be in the callback that triggers when a new font family is selected?
There was a problem hiding this comment.
This effect runs when the component first mounts and will potentially create undo levels when setFontAppearance calls onChange which in turn calls setAttributes. Is that what we want?
Hmm, no, we want this to update when a new font family is set or when a new block is inserted with font appearance settings.
Is an effect necessary? Could the logic be in the callback that triggers when a new font family is selected?
Which callback do you mean here? Currently, setFontFamily isn't a callback, but maybe it should be one 🤔
I'll investigate and see if we can avoid the useEffect.
There was a problem hiding this comment.
@noisysocks I think I've got something that avoids running the effect when the component first mounts - using a useRef to store the fontFamily value and then checking against that before running the logic. I've added this to the follow-up PR in this commit: 18e1bfe. Please let me know if this works better with what you're working on!
There was a problem hiding this comment.
Thanks I'll give that PR a spin!
…ress#61915) * Pass active font faces to appearance control component * Add formatFontWeight function * Use only available font weights in FontAppearanceControl * Refactor weight and style array fallbacks * Rename normal to regular in styles list * Make font weight labels translatable * Handle system fonts * Add comment for font weight and style options * Check against fontFamily rather than name * Handle font style names and values similar to font weights * Use some() rather than findIndex() * Add getMergedFontFamiliesAndFontFamilyFaces function * Add tests for getMergedFontFamiliesAndFontFamilyFaces * Merge common case statements * Remove toUpperCase() on font style names * Attempt to fix variable fonts options * Fix formatFontStyle test * Trim any surrounding whitespace from fontweight string * Add tests for normal and bold font weights * Trim font weight before checking for spaces * Create getFontStylesAndWeights function * Move trim into if statement and improve comment * Allow all uncombined weights and styles to be returned from getFontStylesAndWeights * Make option key consistent with combined result * Move combined option logic into getFontStylesAndWeights() * Swap ABeeZee test for Piazzolla example Co-Authored-By: Vicente Canales <[email protected]> * Fix isSystemFont logic * Apply available font styles and weights when fontFamily changes * Swap isSystemFont logic around * Export isSystemFont and isVariableFont from getFontStylesAndWeights * Use getFontStylesAndWeights to compare font face values * Improve comments for getFontStylesAndWeights * Improve test wording * Add todo item * Include faux bold and italic as options * Handle variable font weights * Try to set the nearest available font weight * Only adds faux bold if a weight of 600 or above is not already available * Updates tests with new faux bold logic --------- Co-authored-by: Vicente Canales <[email protected]> Co-authored-by: Grant Kinney <[email protected]> Co-authored-by: mikachan <[email protected]> Co-authored-by: creativecoder <[email protected]> Co-authored-by: vcanales <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: matiasbenedetto <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: cbirdsong <[email protected]> Co-authored-by: hanneslsm <[email protected]> Co-authored-by: colorful-tones <[email protected]> Co-authored-by: jffng <[email protected]>
What?
Currently in Global Styles > Typography, the font Appearance dropdown includes a static list of options that is hard-coded in the component. Often many of these options aren't available to actually use, depending on the installed font families and the active font.
This PR swaps out this list of static options and replaces them with a list of available font weights/styles based on the currently active font.
Why?
Fixes #49090.
How?
Passes the list of font faces from the active font family to the
FontAppearanceControlcomponent. This can then be used instead of the staticFONT_WEIGHTSandFONT_STYLESconstants currently used to populate the Appearance dropdown menu.Testing Instructions
Please also test changing to a system font and a variable font (e.g. AR One Sans). System fonts should have all possible Appearance options, and variable fonts should have the range of weights included with the font shown in the Appearance dropdown.
Screenshots or screencast