Typography: Stabilize typography block supports within block processing #63401
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +100 B (+0.01%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
try/stabilize-typography-supports-in-block-processing
branch
from
ae75b24 to
110e819
Compare
andrewserong
requested review from
spacedmonkey and
tellthemachines
as code owners
|
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. |
|
Hi folks! I've pinged some of you for review who I think might be interested in this PR — it is not at all urgent for review, as I'm hoping this will make it in for WP 6.7, so please don't feel like you need to take a look at it immediately 🙂 This is just one particular way that stabilization could occur for these block supports, I'm very happy for ideas if anyone has other ideas for how to handle it. Also, please note that the sync PR for core takes a slightly different approach over in WordPress/wordpress-develop#7069 as in core we don't need to rely on filters for the PHP implementation. |
andrewserong
changed the title
andrewserong
removed
the
[Status] In Progress
There was a problem hiding this comment.
I just ran through the test steps quickly - all working as intended. 🚀
I like the small footprint of this PR, and the fact that there's no disruption to existing block.json supports.
Also, it'll pad out the support docs too!
https://developer.wordpress.org/block-editor/reference-guides/block-api/block-supports/#typography
Would we need to update the block.json schema as well?
https://github.com/WordPress/gutenberg/blob/trunk/schemas/json/block.json#L578
|
Thanks for taking this for a spin!
Totally! The overall goal with this work is so that we can better promote these block supports for folks to use 👍
Yes, but I'd like to leave that to a separate PR to try to keep this change as small as it can logically be, and since the |
There was a problem hiding this comment.
Thanks for tackling this @andrewserong 👍
I haven't gotten to test deeply yet but I think there is a specific scenario we might need to consider when stabilizing block supports. It may be less of an issue with typography supports compared to those with less widespread adoption but an issue all the same.
For some blocks, third party extenders filter block type args to enable or disable different block supports. The approach in this PR currently uses the default priority for the filter. I think it might need to be forced to run last so any filters setting experimental flags can stabilized.
Try adding something like the following to the active theme's functions.php
function disable_some_stuff( $args ) {
if ( 'core/paragraph' === $args['name'] ) {
_wp_array_set( $args, array( 'supports', 'typography', '__experimentalFontFamily' ), false );
}
return $args;
}
add_filter( 'register_block_type_args', 'disable_some_stuff', 10 );You'll see that the font family support under the stabilized key is still enabled. If you set the example to something like null for debugging purposes, the experiemental key will still be within the final filtered typography supports.
lib/compat/wordpress-6.7/blocks.php
Outdated
| return $args; | ||
| } | ||
|
|
||
| add_filter( 'register_block_type_args', 'gutenberg_stabilize_experimental_block_supports', 10, 1 ); |
There was a problem hiding this comment.
This might need to be forced to run last so 3rd party filters modifying support flags have been applied first.
|
Oh, thank you for flagging @aaronrobertshaw, I hadn't thought of that! It sounds like setting a priority of something like If this needs to run last, then it sounds like it'll also have implications for the backport in WordPress/wordpress-develop#7069 — in that case, I imagine we'll want to move the code that stabilizes the support to after the |
I'm not sure what the best value is. I could easily see extenders adding like 999 thinking they want their filter to run last. Is
Yep 👍
I think in core the preference would be to not incur the overhead of a filter. That was the feedback received on some block style variations stuff and the theme json resolver. Private functions were created and called directly before/after the filter as needed. |
Ah, of course! Yes, I see we're using that in a couple of places, I'll use that here.
Good idea. Actually a private function is a good idea for the backport anyway as it'll neaten up that function a bit. I'll follow-up there either tomorrow or next week if I run out of time. |
|
Updated, thanks again for the snippet. I wound up testing locally with the post title block as that gives us an immediate indication on the site frontend at render time: function disable_post_title_font_family( $args ) {
if ( 'core/post-title' === $args['name'] ) {
_wp_array_set( $args, array( 'supports', 'typography', '__experimentalFontFamily' ), false );
}
return $args;
}
add_filter( 'register_block_type_args', 'disable_post_title_font_family', 20 );The above is working now with the update the to |
|
Thanks for the quick iteration here 🚀 The code changes look good so far. I'll give it a thorough test with fresh eyes tomorrow.
Bonus points to you for resharing the updated snippet. I was being lazy and dumping out the filtered data rather than locating an appropriate block to confirm with 😅 When I get to testing, do you have any objections to me updating the test instructions to include the filtering angle for others than might belatedly come to this? |
Not at all, feel free to update the description. Thanks! Edit: I've updated the description to include the snippet 👍 |
There was a problem hiding this comment.
This is looking good so far @andrewserong 👍
I only have one real question and a few minor nits I've left as inline comments but this PR is pretty close.
Is it possible that plugins could be checking block support types at all to enhance capabilities?
My understanding of the current migration approach is that the experimental flags are removed entirely from the end result. Would it be worth keeping them for a couple of releases? It might by time to communicate their removal to plugin authors.
I haven't gone looking in the WP Directory though so maybe its a non-issue 🤷
P.S. Sorry for the delay in getting back to review this one 🙏
… removed once 6.7 is the required minimum version
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
andrewserong
force-pushed
the
try/stabilize-typography-supports-in-block-processing
branch
from
bff7af0 to
f6ba048
Compare
Indeed! I'll be going on sabbatical in a couple of weeks, so I'd love to get this merged before then. |
|
Flaky tests detected in f6ba048. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11623502800
|
There was a problem hiding this comment.
✅ Stable typography properties work as expected in block JSON
✅ When disabling properties using the register_block_type_args filter, both __experiemental* and their corresponding stable properties work
✅ UI controls function as expected in post and site editors for stable and experimental properties
LGTM
As @gziolo said, it's early in the cycle so I think we can merge and monitor.
Great work!
|
Thanks for the review @ramonjd! 🙇 Out of an abundance of caution and since I'm wrapping up for the week shortly, I'll leave this open until Monday morning. If there are no objections, I'll intend to merge on Monday. Thanks again for all the feedback and discussion here, everyone! |
andrewserong
deleted the
try/stabilize-typography-supports-in-block-processing
branch
| @@ -62,6 +66,24 @@ function mergeBlockVariations( | |||
| return result; | |||
| } | |||
|
|
|||
| function stabilizeSupports( rawSupports ) { | |||
There was a problem hiding this comment.
The stabilizeSupports function in Gutenberg expects supports to be part of a larger block configuration object that doesn’t have getter-only behavior. If supports is exported as a standalone object - for example with getter-only behavior - stabilizeSupports won’t function as expected.
For example, in Jetpack's case, four blocks were exporting supports in a way that led to console errors like "TypeError: Cannot set property supports of # 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.
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.
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)?
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.supports instead 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 updated supports key instead of mutating deprecation directly.
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.
I'll look into a fix, but just a heads-up since we've been chatting about the block supports stabilization lately
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.
Happy to test later. It would be good to have a local test condition that reproduces the recursion.
Indeed! Yes, let's see if we can fix while also providing a test to cover this case.
Is this the relevant Jetpack PR?
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.
I've put up a potential fix over in #66849
There was a problem hiding this comment.
Your solution looks ideal here, and works well for Jetpack, thank you!
| deprecation.supports = stabilizeSupports( | ||
| deprecation.supports | ||
| ); |
There was a problem hiding this comment.
To address the issue that @coder-karen mentioned, I wonder if we need to do the { ...deprecation } spread before we update the supports key here, and if that would fix the issue for Jetpack 🤔
|
Quick note for added exposure. The current plan is to backport all the block support stabilization in a single core PR. More info can be found in #66918 (comment) |
…ng (WordPress#63401) * Try stabilizing typography block supports within block processing * Update test * Try PHP equivalent * Add backport changelog entry * Update so that writingMode is not stabilized * Add tests for the PHP implementation * Alphabetize * Move the PHP transformation to the compat directory so that it can be removed once 6.7 is the required minimum version * Update filter priority * Update phpunit/block-supports/typography-test.php Co-authored-by: Aaron Robertshaw <[email protected]> * Update phpunit/block-supports/typography-test.php Co-authored-by: Aaron Robertshaw <[email protected]> * Update phpunit/block-supports/typography-test.php Co-authored-by: Aaron Robertshaw <[email protected]> * Update phpunit/block-supports/typography-test.php Co-authored-by: Aaron Robertshaw <[email protected]> * Perform transformation a second time after applying filters * Move to 6.8 directory --------- Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: justintadlock <[email protected]> Co-authored-by: annezazu <[email protected]>
What?
Part of #63001, alternative to #63072.
Stabilize the following typography block supports by no longer requiring the
__experimentalprefix inblock.jsonand when registering block types:__experimentalFontFamily→fontFamily__experimentalFontStyle→fontStyle__experimentalFontWeight→fontWeight__experimentalLetterSpacing→letterSpacing__experimentalTextDecoration→textDecoration__experimentalTextTransform→textTransform__experimentalWritingMode→writingModeThe approach in this PR is to handle the transformation from the experimental properties at the point that blocks are processed. This happens during block registration. The idea proposed here is to do the transformation just before filters are applied so that the filters use the "real" block supports, not the experimental ones.
Note that this PR does not update any of the existing blocks to use the stable syntax. That can be worked on in follow-up PRs.
Why?
Stabilize the above typography block supports so that they can be used without the
__experimentalprefix. However, at the same time, support both the experimental and the non experimental syntaxes, so that blocks out in the wild do not have to be updated in order to continue to work with the typography support.How?
A previous attempt was tried in #63072, however the approach in this PR is simpler:
__experimentalprefix__experimentalas opting in to the "real" support in both JS and PHPTesting Instructions
Here is some block markup for a Title block and a Group block with a paragraph within it, with typography applied to both blocks:
Test markup
block.jsonfiles to use the non-experimental syntax for the typography supports. E.g. updatesupports.typography.__experimentalFontStyletosupports.typography.fontStyleand so on for the six typography supports covered in this PR.diff / patch to update Group and Post Title blocks to use stabilized typography supports
Finally, double check that plugins or themes can still filter out particular experimental typography block supports using the experimental syntax. For example, with the below snippet added to
functions.phpin TT4 theme, we can skip Font support on the Post Title block. With the snippet active, custom font family set on any Post Title block should not be output on the site frontend:Screenshots or screencast
Note: while working on this I noticed that the Appearance controls are not at the correct height in the sidebar UI. That is unrelated to this change, but I've opened a separate issue for it over in #63886
All of these typography supports should work just as they do on
trunk:Related links