Block variations: Add class name

[ockham]

For a given block my/block with a variation myvariation that's registered on the server side (via variation_callback or get_block_type_variations), add a class wp-block-my-block-myvariation to the block wrapper.

Design choices

This PR introduces a new function called get_active_block_variation(), which is modelled after its client-side counterpart, getActiveBlockVariation(). The signatures are almost identical, the major differences being:

• The client-side version accepts a block type name, whereas the server side one expects a WP_Block_Type object. Some discussion on that here.
• The client-side version accepts an additional scope argument, which is forwarded to getBlockVariations(). It is used to specify if a variation should show up in the inserter, in transforms, or if it should only be used internally by the block. Since the server-side's counterpart WP_Block_Type::get_variations() doesn't support a scope argument, it is also not implemented in get_active_block_variation(). (It should be possible to add it later on to both functions if we so wish.)

No separate block-supports. This doesn't introduce a separate block-supports field to control the addition of a generated variation class name; instead, it relies on the existing className block-supports, as does the block's generated class name (i.e. the wp-block-my-block thing). The reason for this is that the addition of an extra class name was considered innocuous and similar enough to the generated block class name that it didn't merit introducing a separate block-supports, which would otherwise increase the API surface.

Inference. Note that while get_active_block_variation respects the isActive field to determine the variation of a block given its attributes, it also has a fallback mechanism, which is to compare the variation's set of defining attributes to the block's.

I believe that this is a reasonable technique to determine a block variation, especially in the absence of an isActive field (which on the server side is furthermore limited to an array of attributes that need to be considered in determining a variation, as opposed to the client side, where arbitrary callbacks are allowed for isActive).

If we agree with this fallback mechanism, I'm inclined to add it to the client side's getActiveBlockVariation as well.

Edit: Removed the fallback mechanism, see https://github.com/WordPress/wordpress-develop/pull/6602/files#r1616104632.

TODO

• Carry over refinements to how the active block is determined if isActive is an array of attribute names. See
  • Block Variations: Support dot notation in isActive string array gutenberg#62088
  • Block Variations: Have getActiveBlockVariation return variation with highest specificity gutenberg#62031
  • Block Variations: Compare objects based on given properties gutenberg#62272
  • Block Variations: Detect active variation correctly based on RichText attribute gutenberg#62325
• Add unit test coverage for the above. Ideally, find a way to reuse test data from client-side getActiveBlockVariation unit tests.

Testing Instructions

Test with a Block Theme (such as TT4) that uses Template Part block (and ideally also Navigation Link blocks), as those are among the few blocks with server-side registered variations. Inspect the page source and verify that the aforementioned blocks have indeed a variation-specific classname added, e.g. wp-block-template-part-area_footer or wp-block-navigation-link-page.

Screenshots

---

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

---

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.

[ockham]

Should we change this to accept a block type name instead to make it consistent with the client-side getActiveBlockVariation()? It will mostly mean some additional (and somewhat redundant) calls to WP_Block_Type_Registry::get_instance()->get_registered() to get the WP_Block_Type instance.

[tjcafferkey]

Personally, I don't think so if the only reason is for that level of consistency. I think it's correct for it to accept the WP_Block_Type instead.

[ockham]

Yeah, I was leaning that way initially, too. Now I've seen how similar a lot of this code here is to the client side (e.g. or generated-classname.php/generated-class-name.js or getBlockDefaultClassName/wp_get_block_default_classname) that I'm starting to reconsider 😅

[gziolo]

I would rather update the client side code to be more flexible. Most of these API operate on the string or object passed to represent a block type. However internally, it often requires converting the string to the block type object.

[tjcafferkey]

I'm assuming we're going to need to open a GB counterpart to deal with the same logic for adding variation class names in the editor too?

[github-actions]

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.

[ockham]

I'm assuming we're going to need to open a GB counterpart to deal with the same logic for adding variation class names in the editor too?

Oh, good point! I was originally just focused on dynamic blocks, but you're right, we'll probably need something somewhere around addGeneratedClassName and/or getBlockDefaultClassName.

[ockham]

I'm assuming we're going to need to open a GB counterpart to deal with the same logic for adding variation class names in the editor too?

Oh, good point! I was originally just focused on dynamic blocks, but you're right, we'll probably need something somewhere around addGeneratedClassName and/or getBlockDefaultClassName.

@tjcafferkey Mind giving that a stab? 😄

[tjcafferkey]

Mind giving that a stab? 😄

I'd love to!

[github-actions]

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 bernhard-reiter, tomjcafferkey, ntsekouras, gziolo.

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

[ntsekouras]

I don't think a fallback like this can work well due to the nuances of object attributes handling. An example is query attribute of Query Loop block.

[ockham]

Ah, thank you for the pointer!

I'll drop the fallback then; I think the PR still makes sense if it only supports variations with isActive fields.

As a potential future enhancement, we could consider supporting dot notation in isActive array items, like I've proposed for the client-side counterpart.

[ockham]

Removed the fallback in 9d54dd6.

[gziolo]

That looks like a good improvement for block variations to automatically generate a unique class name for them 👍

[gziolo]

It feels like there should be a default value provided for backward compatibility.
If any 3rd party code uses it, it passes only one argument today.

It looks like there are no tests as it would caught that.

[gziolo]

I would rather update the client side code to be more flexible. Most of these API operate on the string or object passed to represent a block type. However internally, it often requires converting the string to the block type object.

[gziolo]

This can be simplified because empty also performs isset check internally.

[gziolo]

Two notes here:

• does it cover default values?
• can be the value equal to null? isset can be tricky with that value