Fix: duplicate markup on anchor attribute #48438
Conversation
|
Size Change: +185 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 99ff106. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4539390688
|
t-hamano
force-pushed
the
fix/anchor-dynamic-block
branch
from
8052222 to
7701453
Compare
t-hamano
added
[Type] Bug
| let saveElement; | ||
| try { | ||
| saveElement = | ||
| typeof settings.save === 'function' | ||
| ? settings.save() | ||
| : settings.save; | ||
| if ( saveElement === null || saveElement === undefined ) { | ||
| // If the save function returns null or undefined, save the anchor | ||
| // attribute as a block's comment delimiter without specifying | ||
| // the source. | ||
| settings.attributes = { | ||
| ...settings.attributes, | ||
| anchor: { | ||
| type: ANCHOR_SCHEMA.type, | ||
| }, | ||
| }; | ||
| } else { | ||
| settings.attributes = { | ||
| ...settings.attributes, | ||
| anchor: ANCHOR_SCHEMA, | ||
| }; | ||
| } | ||
| } catch ( error ) { | ||
| // If the save function returns an error, the anchor will be saved | ||
| // in the markup as an id attribute. | ||
| settings.attributes = { | ||
| ...settings.attributes, | ||
| anchor: ANCHOR_SCHEMA, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is probably the most complicated part, but depending on the result of the value of the save property, I register the anchor attribute with the appropriate definition.
There was a problem hiding this comment.
It does feel a bit complicated.
I'd probably be more in favor of something declarative on the blocks that need it rather than magically detecting how it should work.
i.e.
supports: {
anchor: { attributeSource: 'html'|'delimiter' } // defaults to 'html' when `anchor: true`
}Another possible option is to migrate static blocks to use the block delimiter attribute instead of the id html attribute, so that it works the same on client and server.
| saveDefinitions.forEach( ( { value, schema } ) => { | ||
| const settings = registerBlockType( { | ||
| ...blockSettings, | ||
| save: value, | ||
| supports: { | ||
| anchor: true, | ||
| }, | ||
| } ); | ||
| expect( settings.attributes.anchor ).toEqual( schema ); | ||
| } ); |
There was a problem hiding this comment.
In the unit test, it is checked whether the anchor attribute with the correct definition is registered according to the value of the save property.
| let sanitizedAttributes = | ||
| attributes && | ||
| __experimentalSanitizeBlockAttributes( block, attributes ); | ||
| let sanitizedAttributes; | ||
| if ( attributes ) { | ||
| // Anchor attribute isn't supported on the server side and | ||
| // should be excluded from the parameters. | ||
| const { anchor, ...restAttributes } = attributes; | ||
| sanitizedAttributes = | ||
| restAttributes && | ||
| __experimentalSanitizeBlockAttributes( block, restAttributes ); | ||
| } |
There was a problem hiding this comment.
On the server side, the id attribute is no longer recognized and must be explicitly removed from the parameter. Also, the id attribute should not be rendered on the block editor.
There was a problem hiding this comment.
Thanks for tackling this issue @t-hamano 🙇
While reviewing the code I had a vague feeling that even "dynamic" blocks can have saved content that might not be 100% representative of what would be rendered normally. This is a different situation than discussed in #48246 (comment).
The Gutenberg Handbook states the following:
For dynamic blocks, the return value of save could represent a cached copy of the block’s content to be shown only in case the plugin implementing the block is ever disabled.
That suggests to me it is at least possible for a "dynamic" block to have saved content but still need to have its anchor attribute value saved within the block comment.
Is that how you see it?
|
Thank you for the review, @aaronrobertshaw and @talldan! I wondered if I could somehow automatically determine the schema of the anchor attributes, but after listening to your advice, I am beginning to think that this may be practically difficult 😅
I think this approach might make sense. For example, I am considering the following approach: if ( hasBlockSupport( settings, 'anchor' ) ) {
if ( settings.attributes.anchor === true ) {
// Refer to the id attribute in html.
settings.attributes = {
...settings.attributes,
anchor: {
type: 'string',
source: 'attribute',
attribute: 'id',
selector: '*',
},
};
} else if ( settings.attributes.anchor === 'delimiter' ) {
// Save as the comment delimiter.
settings.attributes = {
...settings.attributes,
anchor: {
type: 'string',
},
};
}
}Or, as I mentioned in this comment, the approach is to explicitly define an |
|
Personally, I'd lean towards the modified support property or migrating all static blocks as Dan suggested. The modified support property is basically what I had in mind in my comment over on #48246 so I could be biased there.
It might be splitting hairs but I think discovering or remembering a single config value is less burdensome than remembering to register the attribute. Making the attribute registration part of the block support gives us control should we ever need to change it. |
|
I think more test and discussion may be needed to ideally fix this issue. We don't have much time left before the release of WordPress 6.2. If allowed, one idea might be to revert anchor support for dynamic blocks as done in #44771. |
|
I will change the status of the project as I think we need to discuss and decide on a direction for this PR. |
There was a problem hiding this comment.
Thanks for working on this, @t-hamano!
Maybe we should revert the feature from 6.2 and not rush the solution. We can leave the change required for this feature to be implemented in the plugin - https://github.com/WordPress/wordpress-develop/pull/3497/files#diff-c6bbd1a7e8a627a4c08445569689f029a68156e3b00a69e20564fe410ca0e66fR181.
What do you think?
cc/ @ntsekouras, @ndiego, @annezazu
|
@Mamaduka To revert this feature, I think we also need to remove the anchor support added to the dynamic block's |
|
@t-hamano, this is the "revert" roadmap I've in mind:
|
|
Thank you, @t-hamano! Use the |
|
In WordPress 6.2, anchor support was reverted from dynamic blocks in #48592. The code to automatically add anchor attributes on the server side has been reverted from the core: I will leave this PR open to continue to try to resolve this issue on the Gutenberg plugin. |
t-hamano
force-pushed
the
fix/anchor-dynamic-block
branch
from
7701453 to
528a08d
Compare
t-hamano
requested review from
michalczaplinski,
tellthemachines,
ajitbohra,
ajlende,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Update: I have tried the following approach to solve this problem:
Another approach would be to change the anchor schema to be saved as a comment delimiter in all blocks, as described in this comment. I would love to hear your opinion on whether my approach makes sense 🙏 Also #47868 which adds |
|
As we approach the beta phase of WordPress 6.3, I would love to discuss again what we should do with this PR. This PR fixes the static block issue while maintaining anchor support for dynamic blocks. Currently, all dynamic blocks on the Gutenberg plugin support To avoid this, I see three options.
My opinion is the second or third. |
|
Thanks for the excellent summary, @t-hamano! I like the second option; it will give us more time to thoroughly test the implementation without worrying about WP 6.3 timeline. |
Fixes: #48232
Fixes: #49382
Fixes: #49578
Related to: #44771
Alternative to #48246
What?
This PR fixes a problem that
idattribute is saved to both the comment delimiter and markup in blocks that supportanchorattribute. This will prevent block validation from breaking blocks that previously stored theidattribute only in the markup.Why?
In #44771, dynamic blocks now support the
anchor. The server registers theanchorattribute for all blocks that support theanchorin theregister_attributescallback function. The attribute then has only atypeproperty of typestring.However, previous blocks that supported the
anchorread theidattribute from the markup and don't store the value in the comment delimiter. If theanchorattribute is already registered on the server side, it will take precedence, so its definition will be different from the conventional attributes.As a result, I think the difference in the definition of this property caused the generation of incorrect markup, the
anchorattribute as an unnecessary comment delimiter, and the failure of block validation as reported in #48232.I think we need to separate the definition of the
anchorproperty between blocks that save theidattribute as markup and those that don't save any markup in thesavefunction, as many dynamic blocks do.How?
On the server side, we don't know whether the block saves the
idattribute as markup, i.e., whether thesavefunction returns null or not.Therefore, I have decided NOT to register theanchorattribute on the server side, but to check the rendering result of thesavefunction on the client side and register theanchorattribute with an appropriate definition according to the result.Testing Instructions
The addition of server-side anchor support has already been incorporated into the core, so comment out the following line:https://github.com/WordPress/wordpress-develop/blob/7d8b5e89bc8b980e74eb7ba39b10ba68d52da7b4/src/wp-includes/block-supports/anchor.php#L70(@edit: In #48592, anchor support in core blocks has been reverted. Therefore, the above is not necessary.)
Confirm that anchor attributes are saved and rendered as expected in the following two types of blocks.
Blocks that have asavefunctionBlocks with anchor property
trueor"html"idattribute should be saved as part of the markup and not in the comment delimiter.Blocks that don't have asavefunction or return nullBlocks with anchor property
delimiteranchorattribute should be saved in the comment delimiter.ServerSideRendercomponent should not cause errors when theanchorattribute is set.