Skip to content

Fix site editor broken when fontWeight is not defined or is an integer in theme.json or theme styles #64953

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.

Sign up for GitHub

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

Merged
ramonjd merged 6 commits into from
Sep 4, 2024
Merged

Fix site editor broken when fontWeight is not defined or is an integer in theme.json or theme styles #64953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dbe7731
Handle if nearest value input is not string for fontweight
Imran92 Aug 30, 2024
b9dd96c
handle string and undefined fontweights when getting in site editor
Imran92 Aug 30, 2024
ca110e5
Add additional check for fontStyle keys
Imran92 Sep 4, 2024
47a9f00
Test getting styles and fonts for numeric and undefined fontweight
Imran92 Sep 4, 2024
3e7955c
Test numeric font style getting in typography utils
Imran92 Sep 4, 2024
c3f423d
Add test for nearest value when fontWeight undefined
Imran92 Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions packages/block-editor/src/components/global-styles/test/typography-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,61 @@ describe( 'typography utils', () => {
nearestFontWeight: '400',
},
},
{
message:
'should return nearest fontStyle and fontWeight for normal/400 when fontFamilyFaces contain numerical fontWeight value',
fontFamilyFaces: [
{
fontFamily: 'IBM Plex Mono',
fontStyle: 'normal',
fontWeight: 400,
src: [
'file:./assets/fonts/ibm-plex-mono/IBMPlexMono-Regular.woff2',
],
},
{
fontFamily: 'IBM Plex Mono',
fontStyle: 'italic',
fontWeight: '400',
src: [
'file:./assets/fonts/ibm-plex-mono/IBMPlexMono-Italic.woff2',
],
},
{
fontFamily: 'IBM Plex Mono',
fontStyle: 'normal',
fontWeight: '700',
src: [
'file:./assets/fonts/ibm-plex-mono/IBMPlexMono-Bold.woff2',
],
},
],
fontStyle: 'normal',
fontWeight: '400',
expected: {
nearestFontStyle: 'normal',
nearestFontWeight: '400',
},
},
{
message:
'should return nearest fontStyle and fontWeight for normal/400 when fontFamilyFaces contain undefined fontWeight value',
fontFamilyFaces: [
{
fontFamily: 'IBM Plex Mono',
fontStyle: 'normal',
src: [
'file:./assets/fonts/ibm-plex-mono/IBMPlexMono-Regular.woff2',
],
},
],
fontStyle: 'normal',
fontWeight: '400',
expected: {
nearestFontStyle: 'normal',
nearestFontWeight: '700',
},
},
].forEach(
( {
message,
Expand Down
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/global-styles/typography-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ export function findNearestFontWeight(
availableFontWeights,
newFontWeightValue
) {
newFontWeightValue =
'number' === typeof newFontWeightValue
? newFontWeightValue.toString()
: newFontWeightValue;
if ( ! newFontWeightValue || typeof newFontWeightValue !== 'string' ) {
return '';
}
Expand Down Expand Up @@ -260,7 +264,7 @@ export function findNearestStyleAndWeight(
( { value: fs } ) => fs === fontStyle
);
const hasFontWeight = fontWeights?.some(
( { value: fw } ) => fw === fontWeight
( { value: fw } ) => fw?.toString() === fontWeight?.toString()
);

if ( ! hasFontStyle ) {
Expand Down
16 changes: 12 additions & 4 deletions packages/block-editor/src/utils/get-font-styles-and-weights.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ export function getFontStylesAndWeights( fontFamilyFaces ) {

fontFamilyFaces?.forEach( ( face ) => {
// Check for variable font by looking for a space in the font weight value. e.g. "100 900"
if ( /\s/.test( face.fontWeight.trim() ) ) {
if (
'string' === typeof face.fontWeight &&
/\s/.test( face.fontWeight.trim() )

@ramonjd ramonjd Sep 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're guarding against extra whitespace for space-separated values, I'm wondering if we should trim all face.fontWeight string values, e.g.,

		face.fontWeight =
			'string' === typeof face.fontWeight
				? face.fontWeight.trim()
				: face.fontWeight;
		if ( /\s/.test( face.fontWeight ) ) {

@Imran92 Imran92 Sep 2, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for checking this and the suggestions @ramonjd ! I thought about writing tests for it right after adding the solution but then decided to put in the effort after we agreed on the solution's approach. As I'm very new to this area, there could be other ways to solve it which could be way better :p So wanted to make sure I'm doing it right.

If we're guarding against extra whitespace for space-separated values, I'm wondering if we should trim all face.fontWeight

This is a very nice idea. My only concern about this is that we'll probably be doing the regex test even on numeric values in the right next line.

Also, the regex test will return true if the value of fontweight variable is an object. It should probably never be an object, but this line of here makes me wonder if it sometimes is an object for some reason.

@ramonjd ramonjd Sep 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, true. Thanks for the explanation 👍🏻

I thought about writing tests for it right after adding the solution but then decided to put in the effort after we agreed on the solution's approach.

Maybe tests will reveal some optimizations too. For now, it LGTM to me since it fixes a pretty major bug 🚀

) {
isVariableFont = true;

// Find font weight start and end values.
Expand All @@ -105,11 +108,15 @@ export function getFontStylesAndWeights( fontFamilyFaces ) {
}

// Format font style and weight values.
const fontWeight = formatFontWeight( face.fontWeight );
const fontWeight = formatFontWeight(
'number' === typeof face.fontWeight
? face.fontWeight.toString()

@ramonjd ramonjd Sep 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we coerce all values to string at the top?

I'm thinking, if we make sure all items in the fontWeights array are string here, at the source, the toString() coercions above in packages/block-editor/src/components/global-styles/typography-utils.js might not be required.

@Imran92 Imran92 Sep 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very good idea @ramonjd ! I actually tried to implement it. But later I stopped, because I was afraid of adding an regression because we're modifying the value fetched from the JSON. So for now, I'm just avoiding doing it in this PR. But WDYT about creating an issue where we properly test all implications of and modify the values at the top?

Thank you!

@ramonjd ramonjd Sep 4, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough - probably a premature optimization. Good to have the bug fixed first 😄

: face.fontWeight
);
const fontStyle = formatFontStyle( face.fontStyle );

// Create font style and font weight lists without duplicates.
if ( fontStyle ) {
if ( fontStyle && Object.keys( fontStyle ).length ) {
if (
! fontStyles.some(
( style ) => style.value === fontStyle.value
Expand All @@ -118,7 +125,8 @@ export function getFontStylesAndWeights( fontFamilyFaces ) {
fontStyles.push( fontStyle );
}
}
if ( fontWeight ) {

if ( fontWeight && Object.keys( fontWeight ).length ) {
if (
! fontWeights.some(
( weight ) => weight.value === fontWeight.value
Expand Down
148 changes: 148 additions & 0 deletions packages/block-editor/src/utils/test/get-font-styles-and-weights.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,17 @@ describe( 'getFontStylesAndWeights', () => {
fontWeight: '400',
src: 'http://www.wordpress.org/wp-content/uploads/fonts/N0b52SlTPu5rIkWIZjVKKtYtfxYqZ4RJBFzFfYUjkSDdlqZgy7LYxnL31AHfAAy5.woff2',
},
{
fontFamily: 'Piazzolla',
fontStyle: 'normal',
fontWeight: 600,
src: 'http://www.wordpress.org/wp-content/uploads/fonts/N0b52SlTPu5rIkWIZjVKKtYtfxYqZ4RJBFzFfYUjkSDdlqZgy7JxwXL31AHfAAy5.woff2',
},
{
fontFamily: 'Piazzolla',
fontStyle: 'normal',
src: 'http://www.wordpress.org/wp-content/uploads/fonts/N0b52SlTPu5rIkWIZjVKKtYtfxYqZ4RJBFzFfYUjkSDdlqZgy7JxwXL31AHfAAy5.woff2',
},
{
fontFamily: 'Piazzolla',
fontStyle: 'normal',
Expand Down Expand Up @@ -378,6 +389,10 @@ describe( 'getFontStylesAndWeights', () => {
name: 'Regular',
value: '400',
},
{
name: 'Semi Bold',
value: '600',
},
{
name: 'Black',
value: '900',
Expand All @@ -396,6 +411,14 @@ describe( 'getFontStylesAndWeights', () => {
fontWeight: '400',
},
},
{
key: 'normal-600',
name: 'Semi Bold',
style: {
fontStyle: 'normal',
fontWeight: '600',
},
},
{
key: 'normal-900',
name: 'Black',
Expand All @@ -420,6 +443,14 @@ describe( 'getFontStylesAndWeights', () => {
fontWeight: '400',
},
},
{
key: 'italic-600',
name: 'Semi Bold Italic',
style: {
fontStyle: 'italic',
fontWeight: '600',
},
},
{
key: 'italic-900',
name: 'Black Italic',
Expand Down Expand Up @@ -510,4 +541,121 @@ describe( 'getFontStylesAndWeights', () => {
isVariableFont: false,
} );
} );

it( 'should return available styles and weights for a font without fontWeight', () => {
const fontFamilyFaces = [
{
fontFamily: 'AR One Sans',
fontStyle: 'normal',
src: 'http://www.wordpress.org/wp-content/uploads/fonts/AROneSans-VariableFont_ARRRwght.ttf',
},
];
expect( getFontStylesAndWeights( fontFamilyFaces ) ).toEqual( {
fontStyles: [
{
name: 'Regular',
value: 'normal',
},
{
name: 'Italic',
value: 'italic',
},
],
fontWeights: [
{
name: 'Bold',
value: '700',
},
],
combinedStyleAndWeightOptions: [
{
key: 'normal-700',
name: 'Bold',
style: {
fontStyle: 'normal',
fontWeight: '700',
},
},
{
key: 'italic-700',
name: 'Bold Italic',
style: {
fontStyle: 'italic',
fontWeight: '700',
},
},
],
isSystemFont: false,
isVariableFont: false,
} );
} );

it( 'should return available styles and weights for a font with numeric fontWeight', () => {
const fontFamilyFaces = [
{
fontFamily: 'AR One Sans',
fontStyle: 'normal',
fontWeight: 400,
src: 'http://www.wordpress.org/wp-content/uploads/fonts/AROneSans-VariableFont_ARRRwght.ttf',
},
];
expect( getFontStylesAndWeights( fontFamilyFaces ) ).toEqual( {
fontStyles: [
{
name: 'Regular',
value: 'normal',
},
{
name: 'Italic',
value: 'italic',
},
],
fontWeights: [
{
name: 'Regular',
value: '400',
},
{
name: 'Bold',
value: '700',
},
],
combinedStyleAndWeightOptions: [
{
key: 'normal-400',
name: 'Regular',
style: {
fontStyle: 'normal',
fontWeight: '400',
},
},
{
key: 'normal-700',
name: 'Bold',
style: {
fontStyle: 'normal',
fontWeight: '700',
},
},
{
key: 'italic-400',
name: 'Regular Italic',
style: {
fontStyle: 'italic',
fontWeight: '400',
},
},
{
key: 'italic-700',
name: 'Bold Italic',
style: {
fontStyle: 'italic',
fontWeight: '700',
},
},
],
isSystemFont: false,
isVariableFont: false,
} );
} );
} );