Block Variations: Have getActiveBlockVariation return variation wit…

…h highest specificity (#62031) Co-authored-by: ockham <[email protected]> Co-authored-by: ntsekouras <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: ndiego <[email protected]>
WordPress · May 31, 2024 · 4ca6528 · 4ca6528
1 parent 5576d89
commit 4ca6528
Show file tree

Hide file tree

Showing 3 changed files with 172 additions and 23 deletions.
diff --git a/docs/reference-guides/block-api/block-variations.md b/docs/reference-guides/block-api/block-variations.md
@@ -129,9 +129,9 @@ While the `isActive` property is optional, it's recommended. This API is used by
 
 If `isActive` is not set, the Editor cannot distinguish between an instance of the original block and your variation, so the original block information will be displayed.
 
-The property can be set to either a function or an array of strings (`string[]`).
+The property can be set to either an array of strings (`string[]`), or a function. It is recommended to use the string array version whenever possible.
 
-The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning a `true` or `false` (indicating whether this variation is inactive for this block instance).
+The `string[]` version is used to declare which of the block instance's attributes should be compared to the given variation's. Each attribute will be checked and the variation will be active if all of them match.
 
 As an example, in the core Embed block, the `providerNameSlug` attribute is used to determine the embed provider (e.g. 'youtube' or 'twitter'). The variations may be declared like this:
 
@@ -162,28 +162,32 @@ const variations = [
 ]
 ```
 
- The `isActive` function can compare the block instance value for `providerNameSlug` to the value declared in the variation's declaration (the values in the code snippet above) to determine which embed variation is active:
+The `isActive` property would then look like this:
 
 ```js
-isActive: ( blockAttributes, variationAttributes ) =>
- blockAttributes.providerNameSlug === variationAttributes.providerNameSlug,
+isActive: [ 'providerNameSlug' ]
 ```
 
-The `string[]` version is used to declare which attributes should be compared as a shorthand. Each attribute will be checked and the variation will be active if all of them match. Using the same example for the embed block, the string version would look like this:
+This will cause the block instance value for `providerNameSlug` to be compared to the value declared in the variation's declaration (the values in the code snippet above) to determine which embed variation is active.
+
+Nested object paths are also supported. For example, consider a block variation that has a `query` object as an attribute. It is possible to determine if the variation is active solely based on that object's `postType` property (while ignoring all its other properties):
 
 ```js
-isActive: [ 'providerNameSlug' ]
+isActive: [ 'query.postType' ]
 ```
 
-Nested object paths are also supported. For example, consider a block variation that has a `query` object as an attribute. It is possible to determine if the variation is active solely based on that object's `postType` property (while ignoring all its other properties):
+The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning a `true` or `false` (indicating whether this variation is inactive for this block instance).
+
+Using the same example for the embed block, the function version would look like this:
 
 ```js
-isActive: [ 'query.postType' ]
+isActive: ( blockAttributes, variationAttributes ) =>
+ blockAttributes.providerNameSlug === variationAttributes.providerNameSlug,
 ```
 
-### Caveats to using `isActive`
+### Specificity of `isActive` matches
 
-The `isActive` property can return false positives if multiple variations exist for a specific block and the `isActive` checks are not specific enough. To demonstrate this, consider the following example:
+If there are multiple variations whose `isActive` check matches a given block instance, and all of them are string arrays, then the variation with the highest _specificity_ will be chosen. Consider the following example:
 
 ```js
 wp.blocks.registerBlockVariation(
@@ -212,6 +216,6 @@ wp.blocks.registerBlockVariation(
 );
 ```
 
-The `isActive` check on both variations tests the `textColor`, but each variations uses `vivid-red`. Since the `paragraph-red` variation is registered first, once the `paragraph-red-grey` variation is inserted into the Editor, it will have the title `Red Paragraph` instead of `Red/Grey Paragraph`. As soon as the Editor finds a match, it stops checking.
+If a block instance has attributes `textColor: vivid-red` and `backgroundColor: cyan-bluish-gray`, both variations' `isActive` criterion will match that block instance. In this case, the more _specific_ match will be determined to be the active variation, where specificity is calculated as the length of each `isActive` array. This means that the `Red/Grey Paragraph` will be shown as the active variation.
 
-There have been [discussions](https://github.com/WordPress/gutenberg/issues/41303#issuecomment-1526193087) around how the API can be improved, but as of WordPress 6.3, this remains an issue to watch out for.
+Note that specificity cannot be determined for a matching variation if its `isActive` property is a function rather than a `string[]`. In this case, the first matching variation will be determined to be the active variation. For this reason, it is generally recommended to use a `string[]` rather than a `function` for the `isActive` property.
diff --git a/packages/blocks/src/store/selectors.js b/packages/blocks/src/store/selectors.js
@@ -237,10 +237,17 @@ export const getBlockVariations = createSelector(
 export function getActiveBlockVariation( state, blockName, attributes, scope ) {
  const variations = getBlockVariations( state, blockName, scope );
 
- const match = variations?.find( ( variation ) => {
+ if ( ! variations ) {
+ return variations;
+ }
+
+ const blockType = getBlockType( state, blockName );
+ const attributeKeys = Object.keys( blockType?.attributes || {} );
+ let match;
+ let maxMatchedAttributes = 0;
+
+ for ( const variation of variations ) {
  if ( Array.isArray( variation.isActive ) ) {
- const blockType = getBlockType( state, blockName );
- const attributeKeys = Object.keys( blockType?.attributes || {} );
  const definedAttributes = variation.isActive.filter(
  ( attribute ) => {
  // We support nested attribute paths, e.g. `layout.type`.
@@ -250,10 +257,11 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) {
  return attributeKeys.includes( topLevelAttribute );
  }
  );
- if ( definedAttributes.length === 0 ) {
- return false;
+ const definedAttributesLength = definedAttributes.length;
+ if ( definedAttributesLength === 0 ) {
+ continue;
  }
- return definedAttributes.every( ( attribute ) => {
+ const isMatch = definedAttributes.every( ( attribute ) => {
  const attributeValue = getValueFromObjectPath(
  attributes,
  attribute
@@ -266,11 +274,17 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) {
  getValueFromObjectPath( variation.attributes, attribute )
  );
  } );
+ if ( isMatch && definedAttributesLength > maxMatchedAttributes ) {
+ match = variation;
+ maxMatchedAttributes = definedAttributesLength;
+ }
+ } else if ( variation.isActive?.( attributes, variation.attributes ) ) {
+ // If isActive is a function, we cannot know how many attributes it matches.
+ // This means that we cannot compare the specificity of our matches,
+ // and simply return the best match we have found.
+ return match || variation;
  }
-
- return variation.isActive?.( attributes, variation.attributes );
- } );
-
+ }
  return match;
 }
 

diff --git a/packages/blocks/src/store/test/selectors.js b/packages/blocks/src/store/test/selectors.js
@@ -291,6 +291,7 @@ describe( 'selectors', () => {
  testAttribute: {},
  firstTestAttribute: {},
  secondTestAttribute: {},
+ thirdTestAttribute: {},
  },
  };
  const FIRST_VARIATION_TEST_ATTRIBUTE_VALUE = 1;
@@ -510,6 +511,136 @@ describe( 'selectors', () => {
  } )
  ).toEqual( variations[ 2 ] );
  } );
+ it( 'should return the active variation using the match with the highest specificity for the given isActive array (multiple values)', () => {
+ const variations = [
+ {
+ name: 'variation-1',
+ attributes: {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ },
+ isActive: [
+ 'firstTestAttribute',
+ 'secondTestAttribute',
+ ],
+ },
+ {
+ name: 'variation-2',
+ attributes: {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ thirdTestAttribute: 3,
+ },
+ isActive: [
+ 'firstTestAttribute',
+ 'secondTestAttribute',
+ 'thirdTestAttribute',
+ ],
+ },
+ {
+ name: 'variation-3',
+ attributes: {
+ firstTestAttribute: 1,
+ thirdTestAttribute: 3,
+ },
+ isActive: [
+ 'firstTestAttribute',
+ 'thirdTestAttribute',
+ ],
+ },
+ ];
+
+ const state =
+ createBlockVariationsStateWithTestBlockType( variations );
+
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ } )
+ ).toEqual( variations[ 0 ] );
+ // All variations match the following attributes. Since all matches have an array for their isActive
+ // fields, we can compare the specificity of each match and return the most specific match.
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ thirdTestAttribute: 3,
+ } )
+ ).toEqual( variations[ 1 ] );
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ thirdTestAttribute: 3,
+ } )
+ ).toEqual( variations[ 2 ] );
+ } );
+ it( 'should return the active variation using the first match given the isActive array (multiple values) and function', () => {
+ const variations = [
+ {
+ name: 'variation-1',
+ attributes: {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ },
+ isActive: [
+ 'firstTestAttribute',
+ 'secondTestAttribute',
+ ],
+ },
+ {
+ name: 'variation-2',
+ attributes: {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ thirdTestAttribute: 3,
+ },
+ isActive: [
+ 'firstTestAttribute',
+ 'secondTestAttribute',
+ 'thirdTestAttribute',
+ ],
+ },
+ {
+ name: 'variation-3',
+ attributes: {
+ firstTestAttribute: 1,
+ thirdTestAttribute: 3,
+ },
+ isActive: ( blockAttributes, variationAttributes ) =>
+ blockAttributes.firstTestAttribute ===
+ variationAttributes.firstTestAttribute &&
+ blockAttributes.thirdTestAttribute ===
+ variationAttributes.thirdTestAttribute,
+ },
+ ];
+
+ const state =
+ createBlockVariationsStateWithTestBlockType( variations );
+
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ } )
+ ).toEqual( variations[ 0 ] );
+ // All variations match the following attributes. However, since the third variation has a function
+ // for its isActive field, we cannot compare the specificity of each match, so instead we return the
+ // best match we've found.
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ secondTestAttribute: 2,
+ thirdTestAttribute: 3,
+ } )
+ ).toEqual( variations[ 1 ] );
+ expect(
+ getActiveBlockVariation( state, blockName, {
+ firstTestAttribute: 1,
+ thirdTestAttribute: 3,
+ } )
+ ).toEqual( variations[ 2 ] );
+ } );
  it( 'should ignore attributes that are not defined in the block type', () => {
  const variations = [
  {