Block Variations: Have getActiveBlockVariation fall back to default variation
#63858
Conversation
ockham
added
[Type] Enhancement
|
Size Change: +38 B (0%) Total Size: 1.94 MB
ℹ️ View Unchanged
|
|
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. |
There was a problem hiding this comment.
This makes sense!
- If there is a match for the
isActivefield, we return that match. - If there isn't a match for the
isActivefield, we return the variation with theisDefaultflag.
I don't see any flaws in that logic 🙂. I would have also expected the isDefault to work this way.
In a follow-up, it would be nice to log a warning if multiple blocks accidentally have the isDefault flag. With the current implementation, the first variation in the array would be returned, which is reasonable behavior.
|
Thank you @michalczaplinski! Unfortunately, e2e tests do not seem to agree with this reasoning 😂 -- I'll have to figure out what's going on there. |
ockham
force-pushed
the
update/active-variation-fall-back-to-default
branch
from
99535f0 to
ab758fd
Compare
😅 . Yeah, you're right. I thought this might be because the current PR should perhaps be stacked on top of #63100. I checkout out both and merged #63100 into the current one locally, but the e2e tests still fail. |
|
I've started digging. Here's one of the tests that's failing: gutenberg/test/e2e/specs/editor/plugins/block-variations.spec.js Lines 47 to 60 in bfffbe0 The "Large Quote" variation of the Quote block is registered by an e2e test helper plugin: gutenberg/packages/e2e-tests/plugins/block-variations/index.js Lines 23 to 30 in bfffbe0 Here's a screenshot (that's part of the artifact generated by CI when the e2e test failed):
|
|
I tried to repro that e2e error manually but haven't been able so far. Anyway, I suspect it has to do with two different variations having |
It’s possible to accidentally set multiple variations as default through plugins so it picks the first default found if I recall correctly. |
|
This issue has been punted to 6.8 and has not seen any activity since. Let's move this issue to the 6.9 Editor Task Board and see what can be done in 6.9. For now, I would like to merge the latest trunk into this PR, but first I would appreciate it if you could check whether this PR is currently valid. |
github-project-automation
bot
moved this to 🔎 Needs Review
in WordPress 6.9 Editor Tasks
|
From what I can tell, the reason for the E2E test failure is that the displayed block name has changed. For example, try selecting the 
I think we need the logic to check the |
|
Flaky tests detected in ea07950. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17402571594
|
|
Thank you very much for picking this up, @t-hamano, and apologies for not replying sooner! Your fix looks good and makes sense to me. It might require some more tweaking, as the newly added unit tests are failing now. |
| // If no variation matches the isActive condition, we return the default variation, | ||
| // but only if it doesn't have an isActive condition that wasn't matched. | ||
| // This fallback is only applied for 'block' scope to avoid affecting block name display. | ||
| if ( ! match && scope === 'block' ) { |
There was a problem hiding this comment.
The unit tests don't currently set the scope argument, so they fall back to its default value. It turns out that getActiveBlockVariation simply passes the scope argument through to getBlockVariations.
There, it defaults to [ 'block', 'inserter' ].
In order to account for this case, we might need to adjust the check here.
I realized that 80338cf is not the right approach 😅 If I cherry-pick ecedae0 on top of this PR, the normal Group variation is no longer marked as active in the sidebar: 
In any case, this PR does not appear to be essential for WP 6.9, so I would like to remove it from the project board. |
I think I found a solution to this problem: 7dad4d2 We still need to update the unit test to set the
Yep, that's perfectly fine IMO 👍 |
|
@ockham Thanks for the update! P.S. When we ship this PR, the logic of the |
|
P.S. I tested #63100 (comment) again with the following code: wp.blocks.registerBlockVariation("core/group", {
name: "wpcomsp/marquee-content",
title: "Marquee Content",
icon: "editor-table",
description: "A marquee content block.",
scope: ["block", "inserter"],
attributes: {
isMarquee: true,
layout: { type: "flex", flexWrap: "nowrap" },
className: "wpcomsp-marquee-content",
allowedBlocks: [
"core/paragraph",
"core/heading",
"core/image",
"core/buttons",
],
},
isActive: ["layout.type", "isMarquee"],
});
I tested the above behavior and this PR seems to be better than trunk. When inserting the "Marquee Content" block...
|
ockham
force-pushed
the
update/active-variation-fall-back-to-default
branch
from
ea07950 to
5d939da
Compare
|
Thank you for resurrecting this, @t-hamano! |
|
Just to be safe, let's add the Needs Dev Note label. |
t-hamano
added
the
Needs Dev Note
Dev NoteWordPress 6.9 introduces a small change to active block variation detection. To understand what that change looks like, recall that a block variation can include an The change concerns the case in which none of the Two limitations apply:
In practice, the updated behavior should be consistent with users' and developers' expectations, as the Together with the improvements made to active block variation detection in WordPress 6.6 , this change allows block authors to express the criteria for active block detection in a very succinct and JSON-only manner. For an example, see how these principles were applied to rewrite Group block variations. |
What?
If a block doesn't match any of the block type's variations, return the variation that has
isDefaultset totrue(as long as it doesn't have anisActivefield, as that would mean that it didn't match the latter).Why?
See #63100 (comment).
How?
Explained above :)
Testing Instructions
Cherry-pick ecedae0 (from #63100) on top of this PR.
Then, in the editor, insert multiple Group blocks, choosing a different variation each time. Save your post and reload the editor. Verify that the block inspector reflects the correct variation for each block.