Skip to content
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

Typography: Stabilize typography block supports within block processing #63401

Open
wants to merge 14 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions backport-changelog/6.7/7069.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
https://github.com/WordPress/wordpress-develop/pull/7069

* https://github.com/WordPress/gutenberg/pull/63401
24 changes: 12 additions & 12 deletions lib/block-supports/typography.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,16 @@ function gutenberg_register_typography_support( $block_type ) {
return;
}

$has_font_family_support = $typography_supports['__experimentalFontFamily'] ?? false;
$has_font_family_support = $typography_supports['fontFamily'] ?? false;
$has_font_size_support = $typography_supports['fontSize'] ?? false;
$has_font_style_support = $typography_supports['__experimentalFontStyle'] ?? false;
$has_font_weight_support = $typography_supports['__experimentalFontWeight'] ?? false;
$has_letter_spacing_support = $typography_supports['__experimentalLetterSpacing'] ?? false;
$has_font_style_support = $typography_supports['fontStyle'] ?? false;
$has_font_weight_support = $typography_supports['fontWeight'] ?? false;
$has_letter_spacing_support = $typography_supports['letterSpacing'] ?? false;
$has_line_height_support = $typography_supports['lineHeight'] ?? false;
$has_text_align_support = $typography_supports['textAlign'] ?? false;
$has_text_columns_support = $typography_supports['textColumns'] ?? false;
$has_text_decoration_support = $typography_supports['__experimentalTextDecoration'] ?? false;
$has_text_transform_support = $typography_supports['__experimentalTextTransform'] ?? false;
$has_text_decoration_support = $typography_supports['textDecoration'] ?? false;
$has_text_transform_support = $typography_supports['textTransform'] ?? false;
$has_writing_mode_support = $typography_supports['__experimentalWritingMode'] ?? false;

$has_typography_support = $has_font_family_support
Expand Down Expand Up @@ -91,16 +91,16 @@ function gutenberg_apply_typography_support( $block_type, $block_attributes ) {
return array();
}

$has_font_family_support = $typography_supports['__experimentalFontFamily'] ?? false;
$has_font_family_support = $typography_supports['fontFamily'] ?? false;
$has_font_size_support = $typography_supports['fontSize'] ?? false;
$has_font_style_support = $typography_supports['__experimentalFontStyle'] ?? false;
$has_font_weight_support = $typography_supports['__experimentalFontWeight'] ?? false;
$has_letter_spacing_support = $typography_supports['__experimentalLetterSpacing'] ?? false;
$has_font_style_support = $typography_supports['fontStyle'] ?? false;
$has_font_weight_support = $typography_supports['fontWeight'] ?? false;
$has_letter_spacing_support = $typography_supports['letterSpacing'] ?? false;
$has_line_height_support = $typography_supports['lineHeight'] ?? false;
$has_text_align_support = $typography_supports['textAlign'] ?? false;
$has_text_columns_support = $typography_supports['textColumns'] ?? false;
$has_text_decoration_support = $typography_supports['__experimentalTextDecoration'] ?? false;
$has_text_transform_support = $typography_supports['__experimentalTextTransform'] ?? false;
$has_text_decoration_support = $typography_supports['textDecoration'] ?? false;
$has_text_transform_support = $typography_supports['textTransform'] ?? false;
$has_writing_mode_support = $typography_supports['__experimentalWritingMode'] ?? false;

// Whether to skip individual block support features.
Expand Down
41 changes: 41 additions & 0 deletions lib/compat/wordpress-6.7/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,44 @@ function gutenberg_filter_block_type_metadata_settings_allow_variations_php_file
return $settings;
}
add_filter( 'block_type_metadata_settings', 'gutenberg_filter_block_type_metadata_settings_allow_variations_php_file', 10, 2 );

/**
* Filters the block type arguments during registration to stabilize experimental block supports.
*
* This is a temporary compatibility shim as the approach in core is for this to be handled
* within the WP_Block_Type class rather than requiring a filter.
*
* @param array $args Array of arguments for registering a block type.
* @return array Array of arguments for registering a block type.
*/
function gutenberg_stabilize_experimental_block_supports( $args ) {
if ( empty( $args['supports']['typography'] ) ) {
return $args;
}

$experimental_typography_supports_to_stable = array(
'__experimentalFontFamily' => 'fontFamily',
'__experimentalFontStyle' => 'fontStyle',
'__experimentalFontWeight' => 'fontWeight',
'__experimentalLetterSpacing' => 'letterSpacing',
'__experimentalTextDecoration' => 'textDecoration',
'__experimentalTextTransform' => 'textTransform',
);

$current_typography_supports = $args['supports']['typography'];
$stable_typography_supports = array();

foreach ( $current_typography_supports as $key => $value ) {
if ( array_key_exists( $key, $experimental_typography_supports_to_stable ) ) {
$stable_typography_supports[ $experimental_typography_supports_to_stable[ $key ] ] = $value;
} else {
$stable_typography_supports[ $key ] = $value;
}
}

$args['supports']['typography'] = $stable_typography_supports;

return $args;
}

add_filter( 'register_block_type_args', 'gutenberg_stabilize_experimental_block_supports', PHP_INT_MAX, 1 );
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/font-family.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { shouldSkipSerialization } from './utils';
import { TYPOGRAPHY_SUPPORT_KEY } from './typography';
import { unlock } from '../lock-unlock';

export const FONT_FAMILY_SUPPORT_KEY = 'typography.__experimentalFontFamily';
export const FONT_FAMILY_SUPPORT_KEY = 'typography.fontFamily';
const { kebabCase } = unlock( componentsPrivateApis );

/**
Expand Down
12 changes: 6 additions & 6 deletions packages/block-editor/src/hooks/supports.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ const ALIGN_WIDE_SUPPORT_KEY = 'alignWide';
const BORDER_SUPPORT_KEY = '__experimentalBorder';
const COLOR_SUPPORT_KEY = 'color';
const CUSTOM_CLASS_NAME_SUPPORT_KEY = 'customClassName';
const FONT_FAMILY_SUPPORT_KEY = 'typography.__experimentalFontFamily';
const FONT_FAMILY_SUPPORT_KEY = 'typography.fontFamily';
const FONT_SIZE_SUPPORT_KEY = 'typography.fontSize';
const LINE_HEIGHT_SUPPORT_KEY = 'typography.lineHeight';
/**
* Key within block settings' support array indicating support for font style.
*/
const FONT_STYLE_SUPPORT_KEY = 'typography.__experimentalFontStyle';
const FONT_STYLE_SUPPORT_KEY = 'typography.fontStyle';
/**
* Key within block settings' support array indicating support for font weight.
*/
const FONT_WEIGHT_SUPPORT_KEY = 'typography.__experimentalFontWeight';
const FONT_WEIGHT_SUPPORT_KEY = 'typography.fontWeight';
/**
* Key within block settings' supports array indicating support for text
* align e.g. settings found in `block.json`.
Expand All @@ -34,7 +34,7 @@ const TEXT_COLUMNS_SUPPORT_KEY = 'typography.textColumns';
* Key within block settings' supports array indicating support for text
* decorations e.g. settings found in `block.json`.
*/
const TEXT_DECORATION_SUPPORT_KEY = 'typography.__experimentalTextDecoration';
const TEXT_DECORATION_SUPPORT_KEY = 'typography.textDecoration';
/**
* Key within block settings' supports array indicating support for writing mode
* e.g. settings found in `block.json`.
Expand All @@ -44,13 +44,13 @@ const WRITING_MODE_SUPPORT_KEY = 'typography.__experimentalWritingMode';
* Key within block settings' supports array indicating support for text
* transforms e.g. settings found in `block.json`.
*/
const TEXT_TRANSFORM_SUPPORT_KEY = 'typography.__experimentalTextTransform';
const TEXT_TRANSFORM_SUPPORT_KEY = 'typography.textTransform';

/**
* Key within block settings' supports array indicating support for letter-spacing
* e.g. settings found in `block.json`.
*/
const LETTER_SPACING_SUPPORT_KEY = 'typography.__experimentalLetterSpacing';
const LETTER_SPACING_SUPPORT_KEY = 'typography.letterSpacing';
const LAYOUT_SUPPORT_KEY = 'layout';
const TYPOGRAPHY_SUPPORT_KEYS = [
LINE_HEIGHT_SUPPORT_KEY,
Expand Down
10 changes: 5 additions & 5 deletions packages/block-editor/src/hooks/typography.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ function omit( object, keys ) {
);
}

const LETTER_SPACING_SUPPORT_KEY = 'typography.__experimentalLetterSpacing';
const TEXT_TRANSFORM_SUPPORT_KEY = 'typography.__experimentalTextTransform';
const TEXT_DECORATION_SUPPORT_KEY = 'typography.__experimentalTextDecoration';
const LETTER_SPACING_SUPPORT_KEY = 'typography.letterSpacing';
const TEXT_TRANSFORM_SUPPORT_KEY = 'typography.textTransform';
const TEXT_DECORATION_SUPPORT_KEY = 'typography.textDecoration';
const TEXT_COLUMNS_SUPPORT_KEY = 'typography.textColumns';
const FONT_STYLE_SUPPORT_KEY = 'typography.__experimentalFontStyle';
const FONT_WEIGHT_SUPPORT_KEY = 'typography.__experimentalFontWeight';
const FONT_STYLE_SUPPORT_KEY = 'typography.fontStyle';
const FONT_WEIGHT_SUPPORT_KEY = 'typography.fontWeight';
const WRITING_MODE_SUPPORT_KEY = 'typography.__experimentalWritingMode';
export const TYPOGRAPHY_SUPPORT_KEY = 'typography';
export const TYPOGRAPHY_SUPPORT_KEYS = [
Expand Down
21 changes: 15 additions & 6 deletions packages/blocks/src/api/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = {
},
fontFamily: {
value: [ 'typography', 'fontFamily' ],
support: [ 'typography', '__experimentalFontFamily' ],
support: [ 'typography', 'fontFamily' ],
useEngine: true,
},
fontSize: {
Expand All @@ -193,12 +193,12 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = {
},
fontStyle: {
value: [ 'typography', 'fontStyle' ],
support: [ 'typography', '__experimentalFontStyle' ],
support: [ 'typography', 'fontStyle' ],
useEngine: true,
},
fontWeight: {
value: [ 'typography', 'fontWeight' ],
support: [ 'typography', '__experimentalFontWeight' ],
support: [ 'typography', 'fontWeight' ],
useEngine: true,
},
lineHeight: {
Expand Down Expand Up @@ -240,17 +240,17 @@ export const __EXPERIMENTAL_STYLE_PROPERTY = {
},
textDecoration: {
value: [ 'typography', 'textDecoration' ],
support: [ 'typography', '__experimentalTextDecoration' ],
support: [ 'typography', 'textDecoration' ],
useEngine: true,
},
textTransform: {
value: [ 'typography', 'textTransform' ],
support: [ 'typography', '__experimentalTextTransform' ],
support: [ 'typography', 'textTransform' ],
useEngine: true,
},
letterSpacing: {
value: [ 'typography', 'letterSpacing' ],
support: [ 'typography', '__experimentalLetterSpacing' ],
support: [ 'typography', 'letterSpacing' ],
useEngine: true,
},
writingMode: {
Expand Down Expand Up @@ -297,3 +297,12 @@ export const __EXPERIMENTAL_PATHS_WITH_OVERRIDE = {
'typography.fontSizes': true,
'spacing.spacingSizes': true,
};

export const TYPOGRAPHY_SUPPORTS_EXPERIMENTAL_TO_STABLE = {
__experimentalFontFamily: 'fontFamily',
__experimentalFontStyle: 'fontStyle',
__experimentalFontWeight: 'fontWeight',
__experimentalLetterSpacing: 'letterSpacing',
__experimentalTextDecoration: 'textDecoration',
__experimentalTextTransform: 'textTransform',
};
82 changes: 60 additions & 22 deletions packages/blocks/src/store/process-block-type.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ import warning from '@wordpress/warning';
* Internal dependencies
*/
import { isValidIcon, normalizeIconObject, omit } from '../api/utils';
import { BLOCK_ICON_DEFAULT, DEPRECATED_ENTRY_KEYS } from '../api/constants';
import {
BLOCK_ICON_DEFAULT,
DEPRECATED_ENTRY_KEYS,
TYPOGRAPHY_SUPPORTS_EXPERIMENTAL_TO_STABLE,
} from '../api/constants';

/** @typedef {import('../api/registration').WPBlockType} WPBlockType */

Expand Down Expand Up @@ -62,6 +66,24 @@ function mergeBlockVariations(
return result;
}

function stabilizeSupports( rawSupports ) {
if ( ! rawSupports ) {
return rawSupports;
}

const supports = { ...rawSupports };
if ( supports?.typography && typeof supports.typography === 'object' ) {
supports.typography = Object.fromEntries(
Object.entries( supports.typography ).map( ( [ key, value ] ) => [
TYPOGRAPHY_SUPPORTS_EXPERIMENTAL_TO_STABLE[ key ] || key,
value,
] )
);
}

return supports;
}

/**
* Takes the unprocessed block type settings, merges them with block type metadata
* and applies all the existing filters for the registered block type.
Expand Down Expand Up @@ -102,13 +124,20 @@ export const processBlockType =
),
};

// Stabilize any experimental supports before applying filters.
blockType.supports = stabilizeSupports( blockType.supports );

aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
const settings = applyFilters(
Copy link
Member

Choose a reason for hiding this comment

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

What about filters that update supports and inject their entries, which could be experimental? What about plugins that expect experimental support entries and make decisions based on them?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was raised earlier and I believe what prompted @andrewserong to ask for additional opinions on the best path forward.

In a nutshell, the scenario we flagged, requires the migration to occur after the JS filters have been applied. Andrew found however that the block support filters adding attributes might be cleaner, and a better example, if the migration occurred before.

To cover both bases we may need to either

  • migrate both, before, and after, filters
  • keep the block support filters checking both the experimental and stable flags, then migrate only once after filters are applied

Copy link
Member

@gziolo gziolo Jul 30, 2024

Choose a reason for hiding this comment

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

It’s also possible that 3rd party plugins run some checks based on the experimental block support syntax so the more I think about it the more it feels you should keep both for some time to let devs adjust or even forever. Best it would be to verify with some plugins that reference these experimental names. The approach shared by @youknowriad in #63401 (review) might be also a good first step if you want to exercise the current ecosystem.

Copy link
Contributor Author

@andrewserong andrewserong Aug 1, 2024

Choose a reason for hiding this comment

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

Thanks for the discussion here!

the more I think about it the more it feels you should keep both for some time to let devs adjust or even forever.

I think we definitely want to at least support the experimental syntax forever so that block plugins out in the wild continue to work without being updated.

In terms of support within the filters and in regards to keeping both, how might that look? I.e. would it be something like:

  • When __experimentalFontFamily is set, copy to fontFamily but leave __experimentalFontFamily intact
  • When fontFamily is set, copy to __experimentalFontFamily for back-compat?

If we do the latter as well as the former, after the filter is run, we might need to check if the value has changed (i.e. if the filter mutated one or the other values) and then resync them 🤔

I might not get a chance to update this PR this week, but I think a next step for me could be to add some tests that simulate possible use cases for plugins augmenting or inspecting the values. That might help illuminate possible cases for us to consider, or at the very least, help us limit the potential scope for affecting plugins out in the wild.

Copy link
Member

@gziolo gziolo Aug 1, 2024

Choose a reason for hiding this comment

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

Would it be technically feasible to convert the config objects in block supports that have experimental flags into proxy objects and handle the backward compatibility through augmented setters and getters? Let me share a similar case in PHP to better illustrate the idea:

https://github.com/WordPress/wordpress-develop/blob/74e03e3cbef2f2565028f446c76acb2dabf749bd/src/wp-includes/class-wp-block-type.php#L353L387

That allows to always use in core the new names but correctly match properties through legacy name when necessary.

Copy link
Member

Choose a reason for hiding this comment

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

One final aside.

👎 They won't be able to filter in JS on blocks.registerBlockType to opt blocks in to __experimental prefix supports that they don't already have, as the stabilization in JS happens prior to applying the filter.

I have no real data to support my assumption but based on feedback I got at WordCamps and from GitHub reports, the most common scenario is removing UI controls for these features. I don’t know how that translates into code here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing features is definitely a common scenario. It might be good to know whether folks doing that are not impacted that much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points, I think at the very least this PR should handle the basic ways that plugins might disable controls in both JS and PHP. I think this leans me toward applying the transformation twice in JS (once before running the filter, once after), so that we can capture it. Example test code:

function removeTypographySupports( settings, name ) {
	if ( name === 'core/paragraph' && settings.supports.typography ) {
		settings.supports.typography.__experimentalTextTransform = false;
	}
	return settings;
}

addFilter(
	'blocks.registerBlockType',
	'core/style/addAttribute',
	removeTypographySupports
);

In trunk that'll remove the Letter Case option from the Paragraph block, but not with this PR applied. Ideally it'd recognise if the value has changed after the above filter is run, and migrate it accordingly.

I'll have a play with it this week.

Copy link
Member

@ndiego ndiego Aug 6, 2024

Choose a reason for hiding this comment

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

I think this leans me toward applying the transformation twice in JS (once before running the filter, once after), so that we can capture it.

This would be fantastic, if possible. We have a lot of "Editor curation" content out there that shows folks how to disable/enable block supports with JS and PHP. Either way, I think the developer community will be quite excited about this stabilization effort.

Even if the transformation was not applied twice, you could do something like this to support 6.7 while maintaining backward compatibility, right?

function removeTypographySupports( settings, name ) {
	if ( name === 'core/paragraph' && settings.supports.typography ) {
		settings.supports.typography.__experimentalTextTransform = false;
		settings.supports.typography.textTransform = false;
	}
	return settings;
}

addFilter(
	'blocks.registerBlockType',
	'core/style/addAttribute',
	removeTypographySupports
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if the transformation was not applied twice, you could do something like this to support 6.7 while maintaining backward compatibility, right?

Yes, that example should work nicely for plugins that wish to be forward and backward compatible.

For now, I think I've gotten it working fairly well by applying the transformation a second time for both the block supports and deprecations in 6934b8a. I've added a couple of tests to cover the scenario we've described here.

The implementation still assumes that plugins aren't watching for current values before overriding things, but at least this gives us coverage for plugins that are attempting to switch things off.

I'll give this PR a bit more of a manual test, though, as it seems right to me now, but I'm not sure if I'm thinking is right 😄

'blocks.registerBlockType',
blockType,
name,
null
);

// Re-stabilize any experimental supports after applying filters.
// This ensures that any supports updated by filters are also stabilized.
blockType.supports = stabilizeSupports( blockType.supports );
Copy link
Member

Choose a reason for hiding this comment

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

This util is useful as it needs to run a few times. I see it only does any processing when they typography object is provided, so it should be fast enough.


if (
settings.description &&
typeof settings.description !== 'string'
Expand All @@ -119,29 +148,38 @@ export const processBlockType =
}

if ( settings.deprecated ) {
settings.deprecated = settings.deprecated.map( ( deprecation ) =>
Object.fromEntries(
Object.entries(
// Only keep valid deprecation keys.
applyFilters(
'blocks.registerBlockType',
// Merge deprecation keys with pre-filter settings
// so that filters that depend on specific keys being
// present don't fail.
{
// Omit deprecation keys here so that deprecations
// can opt out of specific keys like "supports".
...omit( blockType, DEPRECATED_ENTRY_KEYS ),
...deprecation,
},
blockType.name,
deprecation
)
).filter( ( [ key ] ) =>
settings.deprecated = settings.deprecated.map( ( deprecation ) => {
// Stabilize any experimental supports before applying filters.
deprecation.supports = stabilizeSupports(
deprecation.supports
);
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
const filteredDeprecation = // Only keep valid deprecation keys.
applyFilters(
'blocks.registerBlockType',
// Merge deprecation keys with pre-filter settings
// so that filters that depend on specific keys being
// present don't fail.
{
// Omit deprecation keys here so that deprecations
// can opt out of specific keys like "supports".
...omit( blockType, DEPRECATED_ENTRY_KEYS ),
...deprecation,
},
blockType.name,
deprecation
);
// Re-stabilize any experimental supports after applying filters.
// This ensures that any supports updated by filters are also stabilized.
filteredDeprecation.supports = stabilizeSupports(
filteredDeprecation.supports
);

return Object.fromEntries(
Object.entries( filteredDeprecation ).filter( ( [ key ] ) =>
DEPRECATED_ENTRY_KEYS.includes( key )
)
)
);
);
} );
}

if ( ! isPlainObject( settings ) ) {
Expand Down
12 changes: 6 additions & 6 deletions packages/blocks/src/store/test/private-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,12 @@ describe( 'private selectors', () => {
name: 'core/example-block',
supports: {
typography: {
__experimentalFontFamily: true,
__experimentalFontStyle: true,
__experimentalFontWeight: true,
__experimentalTextDecoration: true,
__experimentalTextTransform: true,
__experimentalLetterSpacing: true,
fontFamily: true,
fontStyle: true,
fontWeight: true,
textDecoration: true,
textTransform: true,
letterSpacing: true,
fontSize: true,
lineHeight: true,
},
Expand Down
Loading
Loading