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

Stabilize experimental block supports including typography, border, and common flags #7069

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Jul 23, 2024

Trac ticket: https://core.trac.wordpress.org/ticket/61728

This PR brings the changes from the following Gutenberg PRs to core:

Note: The approach here is slightly different to in Gutenberg which relies on filters. Here, the transformations that map experimental to stable keys are handled within the WP_Block_Type class to avoid reliance on filters.

The proposal here is to stabilize the __experimental form of block support keys when props are set on a block type instance. This occurs during registration or when a WP_Block_Type object is instantiated. There was some discussion in an earlier Gutenberg PR about potentially performing this transformation in the block registry instead — however I couldn't find a good precedent for it, and it seems slightly more encapsulated to me to keep the transformation close to where the supports key is stored.

I'm very happy for feedback on this, though!

What's being stabilized?

Experimental block supports stabilized include:

  • Border support: __experimentalBorderborder

Experimental block support features stabilized include:

  • __experimentalFontFamilyfontFamily
  • __experimentalTextDecorationtextDecoration
  • __experimentalFontStylefontStyle
  • __experimentalFontWeightfontWeight
  • __experimentalLetterSpacingletterSpacing
  • __experimentalTextTransformtextTransform

Shared or common experimental block support flags stabilized include:

  • __experimentalSkipSerializationskipSerialization
  • __experimentalDefaultControlsdefaultControls

Test Instructions

Editor

JS package updates will be required for the editor to function with the stabilized block supports.

The editor's block supports use JS hooks to function and without the package updates will still be checking the old experimental block support keys. After applying the block support stabilization in the core block type class, the experimental keys won't exist, so the UI will not correctly reflect the stable config.

Frontend

To test the PHP side of block support stabilization:

  1. On trunk:
    • add several blocks of different types with different block supports to a post
    • ensure some of the included blocks skip serialization, such as an Image block with borders
    • apply some custom global styles for the blocks added above
    • select individual blocks and apply block support styles for those block instances
    • save the post and view it in a separate tab to reference later
  2. Check out this PR branch
    • confirm the block supports and theme.json resolver unit tests are passing
    • in a new tab, view the earlier post on the frontend
    • make sure all the global styles and block instance styles are still applied correctly

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@@ -562,6 +580,29 @@ public function set_props( $args ) {
}
}

// Stabilize experimental block supports.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code intentionally does not unset the experimental version of the supports. This is so that the un-updated JS code in the editor can handle the experimental syntax, and (for now at least) feels slightly safer in terms of backwards compatibility.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@andrewserong
Copy link
Contributor Author

Based on discussion over in WordPress/gutenberg#63401 (comment), I've updated this to move the stabilization to after the filter is run. This ensures that any opt-outs, etc, performed by plugins are appropriately stabilized before the props are set.

I've also moved the logic into a private method so that set_props is easier to read.

@andrewserong andrewserong force-pushed the try/stabilize-typography-block-support-features branch from 76ceb8d to 886f3f1 Compare August 28, 2024 01:50
@andrewserong andrewserong marked this pull request as ready for review August 28, 2024 01:52
Copy link

github-actions bot commented Aug 28, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props aaronrobertshaw, andrewserong, ramonopoly.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@aaronrobertshaw
Copy link

The latest plan is to backport all the block support stabilization updates in a single PR. This will include stabilizing border block supports and shared experimental flags such as __experimentalDefaultControls and __experimentalSkipSerialization.

More details can be found in WordPress/gutenberg#66918 (comment).

In the near future, I'll pick up this backport and update it accordingly, while @andrewserong is away.

@aaronrobertshaw aaronrobertshaw force-pushed the try/stabilize-typography-block-support-features branch from 886f3f1 to 2c6f1a3 Compare November 28, 2024 07:06
@aaronrobertshaw aaronrobertshaw changed the title [WIP] Stabilize experimental Typography block support features within the block type class Stabilize experimental block supports including typography, border, and common flags Nov 28, 2024
@aaronrobertshaw
Copy link

I've pushed some updates to this PR bringing in the broader stabilization of other block supports e.g. __experimentalBorder, __experimentalSkipSerialization etc.

The trac ticket and PR description have been updated with the latest info.

@ramonjd
Copy link
Member

ramonjd commented Dec 3, 2024

So far I've tested this PR against WordPress/gutenberg#63401 (Typography)

Typography controls work as expected with and without __experimental*
Everything renders correctly in the site editor and frontend
Disabling supports, e.g., __experimentalFontFamily disables controls in the UI

Actually, I made a mistake here. I had the plugin installed. 🤦🏻

I'm not sure what I should be seeing in the editor. I understand the packages eventually need to be updated, but I expected no regressions in the UI.

Here's what I'm seeing for paragraph blocks

Trunk This PR
Screenshot 2024-12-03 at 5 09 38 pm Screenshot 2024-12-03 at 5 09 00 pm

How have you been testing this @aaronrobertshaw ?

@ramonjd
Copy link
Member

ramonjd commented Dec 3, 2024

Same for border supports.

Here's what I'm seeing for the group block:

Trunk This PR
Screenshot 2024-12-03 at 5 21 57 pm Screenshot 2024-12-03 at 5 22 14 pm

Am I doing something wrong? I would have assumed, even without the package update, that the controls work with the current __experimental flags.

@aaronrobertshaw aaronrobertshaw force-pushed the try/stabilize-typography-block-support-features branch from 2c6f1a3 to 101126a Compare December 3, 2024 07:38
@aaronrobertshaw
Copy link

Thanks for testing @ramonjd and apologies for the confusion. I should have updated the test instructions on this PR further.

I'm not sure what I should be seeing in the editor. I understand the packages eventually need to be updated, but I expected no regressions in the UI.

The TL;DR is we need the JS package updates.

I've added the longer version to the PR test instructions but the PHP changes here stabilize the block supports in the block type. My understanding is the stabilized block type data is passed to the editor but the JS block support hooks etc will still be checking the __experimental prefixed block support keys. They don't exist after stabilization hence the "missing" UI elements as the editor believes the blocks don't have support.

How have you been testing this @aaronrobertshaw ?

I'd primarily tested using the unit tests and by creating blocks with global and block support styles applied on trunk, then switching to this branch and checking they were still applied correctly on the frontend. I'm definitely open to better testing approaches but my brain broke trying to come up with one.

The PR test instructions have been updated to something more useful. I'll re-test it all myself tomorrow as I'm out of time this evening.

( $key_positions[ $support ] ?? PHP_INT_MAX ) <
( $key_positions[ $stable_support_key ] ?? PHP_INT_MAX );

if ( is_array( $supports[ $stable_support_key ] ) ) {
Copy link
Member

@ramonjd ramonjd Dec 4, 2024

Choose a reason for hiding this comment

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

I'm not sure but I think the is_array( $config ) check from this Gutenberg PR is missing.

So testing using the steps in WordPress/gutenberg#66918, when I set the entire support config to a bool, e.g., settings.__experimentalBorder = false using a filter I get the PHP error:

Fatal error: Uncaught TypeError: array_merge(): Argument #2 must be of type array, bool given in /var/www/html/wp-includes/class-wp-block-type.php:774

Presumably because $stable_config is a bool (false).

Suggested change
if ( is_array( $supports[ $stable_support_key ] ) ) {
if ( is_array( $supports[ $stable_support_key ] ) && is_array( $stable_config ) ) {

Choose a reason for hiding this comment

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

Thanks for flagging this, I'll investigate it.

The border support stabilization's approach and code was changed by the final PR stabilizing the common experimental flags. There's a good chance that last PR introduced this bug to Gutenberg as well. I'll need to looking into both.

Choose a reason for hiding this comment

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

At a quick glance, this backport does reflect the Gutenberg code in its current state. Both will need to be updated to handle a scenario where one of the top-level keys (experimental or stable) is a boolean.

I'll update this shortly and spin up a quick PR to fix Gutenberg.

Choose a reason for hiding this comment

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

A quick PR for the Gutenberg side of this is up in WordPress/gutenberg#67552.

Choose a reason for hiding this comment

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

I've brought across the fix from WordPress/gutenberg#67552 in ef6959b. I'm out of time today but will give it more of a test tomorrow.

@ramonjd
Copy link
Member

ramonjd commented Dec 4, 2024

To test I used the Gutenberg plugin, but disabled the gutenberg_stabilize_experimental_block_supports hook and prevented Gutenberg from registering typography and border block supports.

So far, with the exception of border, the support and common flags appear stable and work as expected.

I'll do another test tomorrow.

@ramonjd
Copy link
Member

ramonjd commented Dec 5, 2024

Thanks for pulling across WordPress/gutenberg#67552!!

This is testing well for me in all scenarios so far.

typgography.__experimental* and their stable counterparts work as expected
__experimentalBorder and border the same
✅ blocks with skipSerialization e.g., image, appear as expected
✅ changes to __experimentalDefaultControls and defaultControls have effect in the UI

@aaronrobertshaw
Copy link

Quick update:

There's an alternate approach being explored as part of WordPress/gutenberg#67729. I'll hold off on committing this PR until that plays out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants