Typography: Stabilize typography block supports within block processing #63401
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +103 B (+0.01%) Total Size: 1.78 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 🙏
| @@ -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.
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.
There was a problem hiding this comment.
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
);There was a problem hiding this comment.
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 😄
andrewserong
force-pushed
the
try/stabilize-typography-supports-in-block-processing
branch
2 times, most recently
from
6934b8a
to
bce2a4b
Compare
… removed once 6.7 is the required minimum version
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>
andrewserong
force-pushed
the
try/stabilize-typography-supports-in-block-processing
branch
from
bce2a4b
to
4c2c130
Compare
|
Update: I've given this PR a rebase and re-tested it, and I believe it's still working as expected, so this is ready for review. A couple of notes:
At this point, I think the main question to address before landing is whether or not the approach here gives enough support for plugins that might be attempting to either opt-out or opt-in to these block supports. Does this PR cover the majority of cases, or does it still feel risky? I think the answer to that question will likely determine whether we think this is good to attempt for WP 6.7. For full disclosure, if this PR and the backport lands, I might have limited time to work on follow-ups, but happy to help review where I can. |
| const settings = applyFilters( | ||
| '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 typography object is provided, so it should be fast enough.
mtias
added
[Type] Code Quality
There was a problem hiding this comment.
Nice work iterating on this one @andrewserong, there are definitely some tricky edge cases to reconcile here.
While giving this another test and review, a couple of questions came up.
- We can't control what support keys plugins might filter on. How can we handle which key overrides which (should a stable key values always override experimental or vice versa)?
- I don't think we can solve this issue completely
- It's also not clear how much of a real world problem this is
- Can adequate testing within Gutenberg, community outreach, and dev notes mitigate enough of the risk?
- The state of block support keys is different between PHP and JS.
- On the JS side, this PR stabilizes the support keys, applies filters, then re-stabilizes. This allows plugins to lean into stabilized keys as desired.
- On the PHP side, the support keys are only stabilized after filters.
- To avoid confusion and help move towards only using stabilized keys, I think making the PHP side also stabilize keys before and after other filters might be good.
Given the concerns around not being able to be 100% sure which keys should override which after filters, this might benefit from an extended run in Gutenberg before landing in core. What do you think about aiming to land this very early in the 6.8 cycle?
|
Thanks for reviewing and for the feedback @aaronrobertshaw!
Yes, I'm leaning that way now, too. I think because we don't really know the impact yet of this PR, it'd be good to land it early in the 6.8 cycle in Gutenberg (as you mention) where we'd have longer to test it out and catch any edge cases before attempting to merge it in core. My feeling is that it's likely in pretty good shape, but without that extra window for testing, we won't know for sure. FYI @annezazu and @justintadlock as I know you were both interested in this change. We're proposing punting this change to 6.8 as it's still a little unknown what the impact of the change will be, and there's a bit of uncertainty surrounding the PHP implementation that would be good to resolve. |
|
It sounds like the best decision (waiting for early 6.8 cycle). We can potentially give this PR a call out for early testing in What's new for developers? too: WordPress/developer-blog-content#309 |
|
Thank you for the proactive ping. Like Justin, that also sounds wise! |
|
Thanks folks! I've moved this to "Punted to 6.8" and will update the PR again early in (or just prior to) the 6.8 cycle 👍 |
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