Image block: Ensure extenders that rely on media ids in block html are supported by block bindings #63013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
talldan
changed the title
wp-image-123 class and id attributes work with block bindings / pattern overrideswp-image-123 class and data-id attributes work with block bindings / pattern overrides
talldan
commented
Jul 1, 2024
talldan
commented
Jul 2, 2024
| @@ -28,12 +28,33 @@ function render_block_core_image( $attributes, $content, $block ) { | |||
| return ''; | |||
| } | |||
|
|
|||
| if ( isset( $attributes['data-id'] ) ) { | |||
There was a problem hiding this comment.
The one thing I'm nervous about in this PR is removing the use of the temporary data-id block attribute, as plugins could be filtering it to a different value. Though I don't know why they would do that.
Unfortunately it's very difficult to check whether that might be the case, and removing it is needed for block bindings to work as expected.
I do think it's a bit of tech debt—particularly the way it causes the block's id is being stored in two attributes and the camel casing of the attribute name, which is non-standard. I can add a dev note, but keen to get the thoughts of others.
There was a problem hiding this comment.
It is a breaking change 😢 . But filtering a temporary value is also not a really good development choice, right?
There was a problem hiding this comment.
As explained in this comment, I made a commit with a potential alternative approach that doesn't require removing the temporary data-id block attribute. It seems to work and it seems closer to the existing implementation.
There was a problem hiding this comment.
Thanks for pushing a commit. I did consider something similar to that, and I think it's not a bad choice, but still has some drawbacks.
With the new changes, the image block's id attribute ($rendered_id) is still being used for the data-id html attribute, whereas in trunk it's the injected data-id, so I don't think it completely addresses the back compat concerns. One use case for filtering data-id would be to modify the html attribute to something else, and that would no longer work. My conclusion is that we can't really support data-id (the block attribute) properly, and that's why I lean towards removing it in favor of the regular id (if it's going to break, then it's a good opportunity to clean up the code as much as possible).
The changes also seem to couple the wp-image- classname to the data-id attribute (the whole logic is in the isset( $attributes['data-id'] ) if statement), which wasn't the case before. data-id is only present for image blocks in galleries, so this would result in the classname only being replaced for gallery images and not standalone images. A small change would be needed to fix that.
|
Size Change: +21 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
talldan
requested review from
artemiomorales,
michalczaplinski and
geriux
as code owners
|
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. |
talldan
added
the
Backport to WP 6.7 Beta/RC
talldan
requested review from
glendaviesnz,
ramonjd,
kevin940726,
SantosGuillamot and
cbravobernal
|
It was not working on my side cause I had Gutenberg disabled 🤦♂️ |
|
I've been discussing it with @cbravobernal to see if we could keep the backward compatibility. I've made this commit with an alternative approach to keep the code as close as possible to the current implementation. It avoids removing the Feel free to revert it if you don't consider correct, @talldan . You might have more context than me. Apart from that, this seems related to a bigger related topic to block bindings. We should reconsider whether the block attributes should be modified during the block parsing and not only during the block rendering to support this kind of filters. But I am not sure how that could work or the implications, so it can be discussed in another place. |
cbravobernal
approved these changes
Jul 2, 2024
There was a problem hiding this comment.
Works as expected, and the solution implies less caveats. I also agree that @talldan has more context to do the final review. I leave the approve, as that way nor the PR creator or @SantosGuillamot has to do that.
This reverts commit ad46c2d.
|
Thanks for adding the commit @SantosGuillamot and for reviewing @cbravobernal. I decided to revert it due to what was mentioned in this comment - #63013 (comment). The quick breakdown is that it didn't restore full backwards compatibility because the Perhaps it might be better to break this PR up into two parts - one for the |
|
I started a slack thread in the core-editor channel to ask for more input on this - https://wordpress.slack.com/archives/C02QB2JS7/p1719990975720999. |
|
You're right that it doesn't fully address the backwards compatibility issue. There are three main aspects of the current status of the pull request that makes me a bit uncomfortable:
Another option I was considering is to keep the existing solution in if ( isset( $attributes['data-id'] ) ) {
$id = $attributes['data-id'];
// The id attribute could change during rendering if it is connected.
if ( ! empty( $attributes['metadata']['bindings']['id'] ) ) {
$id = $attributes['id'];
}
$p->set_attribute( 'data-id', $id );
}
// Replace class name
if ( isset( $attributes['id'] ) && ! empty( $attributes['metadata']['bindings']['id'] ) ) {
$image_classnames = $p->get_attribute( 'class' );
preg_match( '/wp-image-(\d+)/', $image_classnames, $matches );
if ( ! empty( $matches ) ) {
$wp_image_class_id = $matches[1];
}
if ( $wp_image_class_id !== $attributes['id'] ) {
$p->remove_class( 'wp-image-' . $wp_image_class_id );
$p->add_class( 'wp-image-' . $attributes['id'] );
}
}Although I assume it is not ideal either. |
That's a pretty good option. It keeps edit - I'll work on these changes tomorrow |
…e when a binding is applied
talldan
force-pushed
the
fix/image-block-id-classname-for-pattern-overrides
branch
from
bb95cb7 to
92ce782
Compare
|
Flaky tests detected in c4463bb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9788778747
|
kevin940726
approved these changes
Jul 4, 2024
|
I did a final test with jetpack, and this fixes the issue, will go ahead and merge now. Thanks everyone for the help finding a solution. ❤️ |
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Jul 4, 2024
… work with block bindings / pattern overrides (#63013) * Update the image block id classname via dynamic render when it mismatches the attribute value * Also update data-id so that it uses the id attribute * Adjust comment * Fix PHP lint * Only add `data-id` attribute to the image block when used in a gallery * Use multiple array keys to check for gallery parent * Change variable names * Add e2e test * Try alternative approach * Revert "Try alternative approach" This reverts commit ad46c2d. * Add extra test for standalone image * Only apply binding values for data-id attribute and wp-image classname when a binding is applied * Make it more succinct * Fix linting issues --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: jeherve <[email protected]> Co-authored-by: perezcarreno <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.6 branch to get it included in the next release: e9544f3 |
talldan
changed the title
wp-image-123 class and data-id attributes work with block bindings / pattern overrides
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
… work with block bindings / pattern overrides (WordPress#63013) * Update the image block id classname via dynamic render when it mismatches the attribute value * Also update data-id so that it uses the id attribute * Adjust comment * Fix PHP lint * Only add `data-id` attribute to the image block when used in a gallery * Use multiple array keys to check for gallery parent * Change variable names * Add e2e test * Try alternative approach * Revert "Try alternative approach" This reverts commit ad46c2d. * Add extra test for standalone image * Only apply binding values for data-id attribute and wp-image classname when a binding is applied * Make it more succinct * Fix linting issues --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: ndiego <[email protected]> Co-authored-by: jeherve <[email protected]> Co-authored-by: perezcarreno <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #62886
What?
The image block outputs the image id in its markup in a couple of places—a
data-idattribute (which only appears for images in galleries) and awp-image-123classname.When pattern overrides (and to some extend, other types of block binding) are used, these ids can be incorrect in the front-end. The html will still show the id from the original pattern and not for the overridden image.
Why?
In a couple of issues, it's mentioned how these are used by extenders to add additional features to the image block:
How?
Dynamically update the markup of the image block to use the correct ids.
For the classname, a new bit of code is added that detects a mismatch for the id, and if so updates the classname.
For the data attribute, the defacto
idattribute is used instead of the injecteddata-idattribute, since it'll have the correct value.Testing Instructions
idattribute)wp:blockcontentattribute)data-idattribtue andwp-image-class that reflect the id you noted in step 10.