-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use available font weights and styles in FontAppearanceControl #61915
Conversation
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
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.
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.
- Select an appearance option that does exist for another font family
- Select the other font family where this option exists
- Ensure that the appearance option does not change and the styles are still successfully applied in the Editor
These steps work as described when I move from a Semi-Bold Font into the System Font, but not the other way around. It's also resetting the style when moving between two custom fonts that do include variations such as Semi Bold.
fonts.mov
I think this is happening because the weight comparison is missing some nuance in how the weights are defined in variable fonts:
fontFamilyFaces | useEffect |
---|---|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work better for you now, after ec2cf86?
Yes, that took care of that case :)
This is looking good to me 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <1157901+vcanales@users.noreply.github.com> * 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 <1157901+vcanales@users.noreply.github.com> Co-authored-by: Grant Kinney <hi@grant.mk> Co-authored-by: mikachan <mikachan@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: vcanales <vcanales@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: matiasbenedetto <mmaattiiaass@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: cbirdsong <cbirdsong@git.wordpress.org> Co-authored-by: hanneslsm <hanneslsm@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org> Co-authored-by: jffng <jffng@git.wordpress.org>
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
FontAppearanceControl
component. This can then be used instead of the staticFONT_WEIGHTS
andFONT_STYLES
constants 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