Typography: Stabilize typography block supports within block processing #63401
There are no files selected for viewing
| 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 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| <?php | ||
| /** | ||
| * Temporary compatibility shims for block APIs present in Gutenberg. | ||
| * | ||
| * @package gutenberg | ||
| */ | ||
|
|
||
| /** | ||
| * 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 ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
| TYPOGRAPHY_SUPPORTS_EXPERIMENTAL_TO_STABLE, | ||
| } from '../api/constants'; | ||
|
|
||
| /** @typedef {import('../api/registration').WPBlockType} WPBlockType */ | ||
|
|
||
|
|
@@ -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. | ||
|
|
@@ -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( | ||
|
There was a problem hiding this comment. What about filters that update There was a problem hiding this comment. 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
There was a problem hiding this comment. 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. There was a problem hiding this comment. Thanks for the discussion here!
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:
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. There was a problem hiding this comment. 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: That allows to always use in core the new names but correctly match properties through legacy name when necessary. There was a problem hiding this comment. One final aside.
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. There was a problem hiding this comment. Removing features is definitely a common scenario. It might be good to know whether folks doing that are not impacted that much. There was a problem hiding this comment. 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 I'll have a play with it this week. There was a problem hiding this comment.
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
);There was a problem hiding this comment.
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 ); | ||
|
There was a problem hiding this comment. This util is useful as it needs to run a few times. I see it only does any processing when they |
||
|
|
||
| if ( | ||
| settings.description && | ||
| typeof settings.description !== 'string' | ||
|
|
@@ -119,29 +148,38 @@ export const processBlockType = | |
| } | ||
|
|
||
| if ( settings.deprecated ) { | ||
| 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
Comment on lines
+153
to
+155
There was a problem hiding this comment. To address the issue that @coder-karen mentioned, I wonder if we need to do the |
||
| 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 ) ) { | ||
|
|
||
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.
The
stabilizeSupportsfunction in Gutenberg expectssupportsto be part of a larger block configuration object that doesn’t have getter-only behavior. Ifsupportsis exported as a standalone object - for example with getter-only behavior -stabilizeSupportswon’t function as expected.For example, in Jetpack's case, four blocks were exporting
supportsin a way that led to console errors like "TypeError: Cannot set propertysupportsof # which has only a getter," causing editor load issues (both the site editor and post / page editors will not load at all, if the Jetpack blocks setting is enabled).We can fix this export approach on our end, but it'd only work for anyone with the version of Jetpack with the fix. I’m also concerned this could affect other plugins as well. Would it be possible to add a conditional check to help prevent similar issues with other plugins using exports like this (and previous versions of plugins)?
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 flagging this @coder-karen, this is just the kind of feedback we were hoping for by landing this change in the Gutenberg plugin, so we can account for any particular cases in plugins before this change makes it into a major WP release.
Can you give me an example of the kind of conditional check you have in mind? How might we mutate the value in a way that's compatible with what's happening in the plugin? Or, do you mean that we would skip any mutations altogether if we only have a getter?
From looking over the code, as far as I can tell, I think the culprit might be here: #63401 (comment) — where we're mutating
deprecation.supportsinstead of creating a new object, which we do slightly further down. So I'm wondering if a fix could be creating a new object with the updatedsupportskey instead of mutatingdeprecationdirectly.FYI: @aaronrobertshaw for visibility. (I'll look into a fix, but just a heads-up since we've been chatting about the block supports stabilization lately)
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.
Happy to test later. It would be good to have a local test condition that reproduces the recursion.
Is this the relevant Jetpack PR?
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.
Indeed! Yes, let's see if we can fix while also providing a test to cover this case.
That seems to be the one as far as I can tell. Hopefully if we fix this in GB that PR won't be required 🤞
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 put up a potential fix over in #66849
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.
Your solution looks ideal here, and works well for Jetpack, thank you!