Typography: Stabilize typography block supports within block processing #63401
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +3.12 kB (+0.18%) Total Size: 1.76 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
lib/block-supports/typography.php
Outdated
| @@ -621,3 +621,41 @@ function gutenberg_get_typography_font_size_value( $preset, $settings = array() | |||
| remove_filter( 'render_block', 'wp_render_typography_support' ); | |||
| } | |||
| add_filter( 'render_block', 'gutenberg_render_typography_support', 10, 2 ); | |||
|
|
|||
| /** | |||
| * Filters the block type arguments to stabilize experimental block supports. | |||
There was a problem hiding this comment.
Do you reckon this migration will be tied to 6.7, or a general deprecation?
I guess what I'm really asking is, should the filter be in a compat folder?
There was a problem hiding this comment.
Oh, yes, a compat folder is a better idea since the idea is to remove this once 6.7+ is the required WP version for Gutenberg 👍
I'll move that around tomorrow. Thanks!
There was a problem hiding this comment.
Done in 71a0a96 👍
|
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 |
… removed once 6.7 is the required minimum version
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 🙏
| // Stabilize any experimental supports before applying filters. | ||
| blockType.supports = stabilizeSupports( blockType.supports ); | ||
|
|
There was a problem hiding this comment.
Plugins and themes could filter the supports using experimental flags. Same as with the PHP filter. So maybe this should be moved to after the application of filters similar to using PHP_INT_MAX as the priority.
There was a problem hiding this comment.
Oh, good question. Unfortunately in this case I think we might need to apply it before the JS filters, because we have logic such as in addAttributes for font family that's registered against blocks.registerBlockType that will check against the stable name for the typography support.
I'll give this a bit more thought!
There was a problem hiding this comment.
I guess it could be done in both places. Hard to judge if it is worth the cost of doing so. Perhaps outreach could help get a feel for the possible impact of leaving it as only before the filters?
| deprecation.supports = stabilizeSupports( | ||
| deprecation.supports | ||
| ); |
There was a problem hiding this comment.
Nice. I was wondering if this PR would also be covering deprecations.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Thanks for reviewing and thinking this feature through @aaronrobertshaw, I very much appreciate the discussions thus far! 🙇
Good question. The approach in this PR right now is to migrate from the Overall, I'm leaning a little toward the approach in this PR rather than having to keep the experimental versions of the support around. I explored that in the earlier PR #63072 which didn't feel as elegant as the approach here, but it did technically work. Also, as discussed in our thread (#63401 (comment)) we could also look at running the migration twice in JS (once before running the filter, and once after), though that might be redundant. At this stage, since I've gone down a particular rabbithole with this feature, I'll just ping @WordPress/gutenberg-core for visibility in case anyone else has further thoughts or ideas about how we can best handle stabilizing block supports. We'll need to re-use this again in the future when we stabilize other supports (e.g. border), so hopefully this PR will be a helpful exercise not just for Typography 🙂 |
There was a problem hiding this comment.
I think this is one of those PRs where it's very hard to judge the impact without actually shipping.
From my testing, reading of the code and the discussion. It seems a sane approach but we should highlight this change in the release post and keeping an eye on potential issues being raised (also during the WP beta period)
| @@ -102,6 +124,9 @@ export const processBlockType = | |||
| ), | |||
| }; | |||
|
|
|||
| // Stabilize any experimental supports before applying filters. | |||
| blockType.supports = stabilizeSupports( blockType.supports ); | |||
|
|
|||
| const settings = applyFilters( | |||
There was a problem hiding this comment.
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?
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
- 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
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!
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
__experimentalFontFamilyis set, copy tofontFamilybut leave__experimentalFontFamilyintact - When
fontFamilyis set, copy to__experimentalFontFamilyfor 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.
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.
Again, I’m perfectly fine with trying as is in this PR to see what issues might pop up and react accordingly. As long is it’s communicated with plugin and theme authors early so they know how to migrate code, where to raise issues or seek for help 👍
Next time, it's going to be easier to repeat the same process based on experience gained this time.
There was a problem hiding this comment.
Good points! Yeah, I'm leaning toward going with something like the current state of this PR, and essentially committing to allowing plugins to opt-in to block supports, but not explicitly deal with the situation of plugins "reading" the support keys.
In terms of what it'll mean for plugins in the current state:
- 👍 They can still register blocks and opt-in to typography supports using the
__experimentalprefix. - 👍 They can still filter
register_block_type_argsin PHP to opt existing blocks into typography supports that they might not have yet, as the filter in this PR will run last. - 👎 They won't be able to filter in JS on
blocks.registerBlockTypeto opt blocks in to__experimentalprefix supports that they don't already have, as the stabilization in JS happens prior to applying the filter.
In order to deal with the potential of #3, do you think it'd be worth us running the stabilization a second time after the blocks.registerBlockType filter, so that if anything has been opted into, it'll get moved over to the stable key again?
It does mean running the stabilization twice instead of once for each call to process the block type. But it isn't a hugely complex function, so might not be too bad 🤔
I'll leave this PR as is over the weekend and revisit it sometime next week. Thanks again for all the discussion!
There was a problem hiding this comment.
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.
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 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.
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