-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Font Face: Get name from "fontFamily" setting, not "name". #54615
Font Face: Get name from "fontFamily" setting, not "name". #54615
Conversation
If the "name" setting does not exist, then get it from the "fontFamily" setting.
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.4/fonts/font-face/class-wp-font-face-resolver.php |
To avoid reflection issues on different PHP versions and potential contributor confusion, merging the new tests into the existing test.
533ae9d
to
370caf0
Compare
If the name has ' symbols those are included. We should also strip out any ' or " |
👋 I think we should not default to using the name of a font family as the default value for the CSS font faces printed into the site. If the {
"fontFace": [
{
"fontFamily": "Cardo",
"fontStyle": "normal",
"fontWeight": "700",
"src": [
"file:./assets/fonts/cardo_normal_700.woff2"
]
}
],
"fontFamily": "Cardo",
"name": "Heading",
"slug": "heading"
} The font face css definition in this case is: @font-face {
font-family: Heading;
font-style: normal;
font-weight: 700;
font-display: fallback;
src: url("http://localhost/wp1/wp-content/themes/twentytwentyfour/assets/fonts/cardo_normal_700.woff2")
format("woff2");
} And the css preset is:
So, the font is not rendering as expected. |
I think the backup of using the first value of the family So following the previous example we could have something like the following: theme.json font family definition {
"fontFace": [
{
"fontFamily": "Cardo",
"fontStyle": "normal",
"fontWeight": "700",
"src": [
"file:./assets/fonts/cardo_normal_700.woff2"
]
}
],
"fontFamily": "Cardo, system-ui",
"name": "Heading",
"slug": "heading"
} Output css for font face @font-face {
font-family: Cardo;
font-style: normal;
font-weight: 700;
font-display: fallback;
src: url("http://localhost/wp1/wp-content/themes/twentytwentyfour/assets/fonts/cardo_normal_700.woff2")
format("woff2");
} CSS preset value: --wp--preset--font-family--heading: Cardo; |
That collection isn't required right? System fonts would not. However there will always be a "first item in the list" for fontFamily. I think it would be great to use that value for the |
@matiasbenedetto Hmm what if the If the So to protect the styles, I think you're right - it should use the required |
Yep, just to reiterate I would prefer to use the font face fontFamily property instead of the family |
@pbking Font Face is read-only of Theme JSON. It's not involved in what gets populated in the typography pickers. Testing it with only Core's You found a bug. Want to open a new issue for it? |
@matiasbenedetto What happens though if an extender has copied and pasted the family |
@pbking The System fonts though do not have |
Do not the "name" settting. Instead only use the required "fontFamily" setting.
It seems less likely, but in that case, we can maintain the coma-splitting safety. Apart from that, It would be nice to document each field properly so extenders know what is expected in each field. |
I think the comma-splitting is needed, just in case.
Do you mean each field in the |
Yes, but that's not part of the scope of this PR. Just a nice thing to have in the future. |
Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>
…he former is not being used Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>
Co-authored-by: Brian Alexander <824344+ironprogrammer@users.noreply.github.com>
Flaky tests detected in 014b4e3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6253339061
|
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.
Refreshed with trunk
and LGTM 👍🏻
Because `WP_Font_Face_Resolver` already exists in Core release code, this update cannot be tested in CI without migrating the code into a temporary `_Gutenberg` resolver, which will complicate syncing back to Core. Test will be added to a follow-up PR, to merge once this update is synced to Core. Note that this test does succeed when run locally against WP `trunk` (currently 6.4-alpha).
Iff the "name" setting does not exist (as it's not required by the schema or documentation), get the font-family name from the required "fontFamily" setting, i.e. within each `typography.fontFamilies`.
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: 5cde445 |
PR 5298 WordPress/wordpress-develop#5298 ports the Resolver changes and tests to Core. Once committed, the tests can be (re) added into the Gutenberg repo for the CI jobs running against Core's |
Instead of getting the name from the optional `'name'` field, the font-family name now comes from the required `'fontFamily'` field. This change fixes a back-compat (BC) break in how the font-family name is pulled from the incoming font data in the `WP_Font_Face_Resolver`. Why? WP Core does not require the `'name'` field in theme.json. For themes that do not declare it, that set of font variations is ignored, thus causing a BC break from how the stopgap code worked (see [53282]). However, `WP_Theme_JSON` schema does require the `fontFamily` field in each of the `typography.fontFamilies`. == Other details: Includes a parser to extract the first entry when a `fontFamily` field has a comma-separated list of font-families, e.g. `Inter, sans-serif`. References: * Merge from Gutenberg's PR WordPress/gutenberg#54615. Follow-up to [56500], [53282]. Props ironprogrammer, hellofromTonya, mmaattiiaass, pbking. Fixes #59165. git-svn-id: https://develop.svn.wordpress.org/trunk@56688 602fd350-edb4-49c9-b593-d223f7449a82
Instead of getting the name from the optional `'name'` field, the font-family name now comes from the required `'fontFamily'` field. This change fixes a back-compat (BC) break in how the font-family name is pulled from the incoming font data in the `WP_Font_Face_Resolver`. Why? WP Core does not require the `'name'` field in theme.json. For themes that do not declare it, that set of font variations is ignored, thus causing a BC break from how the stopgap code worked (see [53282]). However, `WP_Theme_JSON` schema does require the `fontFamily` field in each of the `typography.fontFamilies`. == Other details: Includes a parser to extract the first entry when a `fontFamily` field has a comma-separated list of font-families, e.g. `Inter, sans-serif`. References: * Merge from Gutenberg's PR WordPress/gutenberg#54615. Follow-up to [56500], [53282]. Props ironprogrammer, hellofromTonya, mmaattiiaass, pbking. Fixes #59165. Built from https://develop.svn.wordpress.org/trunk@56688 git-svn-id: https://core.svn.wordpress.org/trunk@56200 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Instead of getting the name from the optional `'name'` field, the font-family name now comes from the required `'fontFamily'` field. This change fixes a back-compat (BC) break in how the font-family name is pulled from the incoming font data in the `WP_Font_Face_Resolver`. Why? WP Core does not require the `'name'` field in theme.json. For themes that do not declare it, that set of font variations is ignored, thus causing a BC break from how the stopgap code worked (see [53282]). However, `WP_Theme_JSON` schema does require the `fontFamily` field in each of the `typography.fontFamilies`. == Other details: Includes a parser to extract the first entry when a `fontFamily` field has a comma-separated list of font-families, e.g. `Inter, sans-serif`. References: * Merge from Gutenberg's PR WordPress/gutenberg#54615. Follow-up to [56500], [53282]. Props ironprogrammer, hellofromTonya, mmaattiiaass, pbking. Fixes #59165. Built from https://develop.svn.wordpress.org/trunk@56688 git-svn-id: http://core.svn.wordpress.org/trunk@56200 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Instead of getting the name from the optional `'name'` field, the font-family name now comes from the required `'fontFamily'` field. This change fixes a back-compat (BC) break in how the font-family name is pulled from the incoming font data in the `WP_Font_Face_Resolver`. Why? WP Core does not require the `'name'` field in theme.json. For themes that do not declare it, that set of font variations is ignored, thus causing a BC break from how the stopgap code worked (see [53282]). However, `WP_Theme_JSON` schema does require the `fontFamily` field in each of the `typography.fontFamilies`. == Other details: Includes a parser to extract the first entry when a `fontFamily` field has a comma-separated list of font-families, e.g. `Inter, sans-serif`. References: * Merge from Gutenberg's PR WordPress/gutenberg#54615. Follow-up to [56500], [53282]. Props ironprogrammer, hellofromTonya, mmaattiiaass, pbking. Fixes #59165. git-svn-id: https://develop.svn.wordpress.org/trunk@56688 602fd350-edb4-49c9-b593-d223f7449a82
Instead of getting the name from the optional `'name'` field, the font-family name now comes from the required `'fontFamily'` field. This change fixes a back-compat (BC) break in how the font-family name is pulled from the incoming font data in the `WP_Font_Face_Resolver`. Why? WP Core does not require the `'name'` field in theme.json. For themes that do not declare it, that set of font variations is ignored, thus causing a BC break from how the stopgap code worked (see [53282]). However, `WP_Theme_JSON` schema does require the `fontFamily` field in each of the `typography.fontFamilies`. == Other details: Includes a parser to extract the first entry when a `fontFamily` field has a comma-separated list of font-families, e.g. `Inter, sans-serif`. References: * Merge from Gutenberg's PR WordPress/gutenberg#54615. Follow-up to [56500], [53282]. Props ironprogrammer, hellofromTonya, mmaattiiaass, pbking. Fixes #59165. git-svn-id: https://develop.svn.wordpress.org/trunk@56688 602fd350-edb4-49c9-b593-d223f7449a82
What?
Get the font-family name from the "fontFamily" field, instead of the"name" field.
Why?
To avoid a back-compatibility (BC) break for themes that do not have the
"name"
setting defined.WP Core also does not require the
"name"
setting. Using it exclusively is a BC break in Core.It's not clearly documented that the
"name"
setting is required. Thus, it's been optional.How?
Use the font-family's
fontFamily
field instead of itsname
field.As the
fontFamily
field could be a comma-separated list of font-families, it parses the list to extract the first entry, using the same approach in the Fonts API.Why
fontFamily
setting?WP_Theme_JSON
schema requires thefontFamily
setting in each of thetypography.fontFamilies
.Testing Instructions
trunk
. (Why?trunk
has Font Face in it, so the Font Face files in Gutenberg will not load into memory.)theme.json
file in your favorite editor and modify it as follows:"name": "DM Sans",
line.wp-fonts-local
style element.Expected:
@font-face
styles ❌@font-face
styles ✅