Size Change: +75 B (0%)

Total Size: 1.94 MB

_{compressed-size-action}

Do you have any reasons why complex alternatives should be even considered in contrast to this proposal?

• Block Bindings: Add support to image caption attribute in block bindings #70642
• Image block: Save <figcaption> if caption attr is bound #71483

Do you have any reasons why complex alternatives should be even considered in contrast to this proposal?

• Block Bindings: Add support to image caption attribute in block bindings #70642
• Image block: Save <figcaption> if caption attr is bound #71483

Right now I find the "simple" solution (this PR) quite compelling, as it doesn't require new directives, block deprecations, etc.

It does however depend on this assumption:

What I meant is a version with no directives at all -- where WP just determines based on the block attribute description that if this is a rich-text sourced attribute that can be connected to Block Bindings, and it's empty, then we also remove the HTML element denoted by the selector.

The latter is implemented by WordPress/wordpress-develop#9702. (I have yet to backport this to GB; I've started work on backporting the underlying Core changeset here).

As for potential advantages of the more complex solutions: It could be argued that solutions like the data-wp-maybe-remove directive decouple some of the required Block Bindings processing from the block, as they are agnostic of Block Bindings and simply instruct WP to remove an HTML element depending on the directive's value. However, it's kind of verbose (especially since it needs to be added to all Image blocks, and thus requires updates to existing markup), and I'm not 100% convinced how well it would scale (e.g. to constructs like loops). Finally, I ended up with the current approach since all the information that data-wp-maybe-remove expresses is readily available from the caption attribute (i.e. its value, block binding, or pattern override), so the directive seemed redundant.

We have full control over which block attributes are supported by Block Bindings, so we could try to go ahead with this PR and see which of the attributes that conditionally add HTML elements we can cover with it (e.g. Pullquote's citation should work), and if we run into any cases where this approach doesn't cut it. Since we're not introducing any deprecations, it should be easy enough to change course.

Thank you for sharing the additional context. It feels like focusing on this PR and addressing all the points from #71483 (comment) is the most straightforward way to go without going into complex backward compatibility challenges.

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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ockham <[email protected]>
Co-authored-by: gziolo <[email protected]>

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

Once #71389 lands, we can use the block_bindings_supported_attributes_{$block_type} filter to set the caption attribute as supported by Block Bindings as part of this PR.

Yes, let's include it. One remark, I would put the filter inside the Gutenberg compat layer, rather than next to the code that gets synced to WordPress core.

After rebasing with trunk and including the filter with a callback presented in the description, everything works as advertised:

I was able to use the default UI in the Attributes panel to connect caption with the source.

I found one edge case, when working with Pattern Overrides:

On the frontend, the Image block gets completely removed.

The code of the pattern that I created:

<!-- wp:heading {"metadata":{"name":"Headline","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<h2 class="wp-block-heading">Just do it!</h2>
<!-- /wp:heading -->

<!-- wp:image {"metadata":{"name":"Featured image","bindings":{"__default":{"source":"core/pattern-overrides"}}}} -->
<figure class="wp-block-image"><img alt=""/><figcaption class="wp-element-caption"></figcaption></figure>
<!-- /wp:image -->

It might be something related to the fact, that I didn't set the fallback image. If I do it, it works as expected.

Well, if I set a fallback image, and pick a different one in the pattern, then it doesn't get replaced. In effect, it means that url replacement doesn't work properly.

Flaky tests detected in 3b103b2.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17776642404
📝 Reported issues:

• [Flaky Test] should paginate #69568 in /test/e2e/specs/site-editor/user-global-styles-revisions.spec.js
• [Flaky Test] should refresh the page when stylesheet loading fails #71135 in /test/e2e/specs/interactivity/router-styles.spec.ts

Once #71389 lands, we can use the block_bindings_supported_attributes_{$block_type} filter to set the caption attribute as supported by Block Bindings as part of this PR.

Yes, let's include it. One remark, I would put the filter inside the Gutenberg compat layer, rather than next to the code that gets synced to WordPress core.

Good point. Done in 4d94de2.

I'll file a PR to do the same to move this filter out of the Date block's code.

Is it ready for the final round of testing? Any leads on what wasn't working for me that I noted in #71483 (comment)?

Thank you!

I'd like to merge #71691 before this, since otherwise, this PR has the potential to actually break Pattern Overrides in the edge case that you found. I'll keep it set to "Blocked" for now.

@ockham is there a reason we need to limit this to the image block, or would this work generally for all blocks as part of passing data into save()? For instance, what if we provided a value for the block bindings to the attributes on save() such that each block rendered assuming that content were there?

We could save some zero-width space, or an internal Unicode code point, or a zero, or whatever is appropriate for the type of the attribute, and clear it out after save(). Perhaps that would at least cover existing issues with conditionally-generated HTML structure?

@ockham is there a reason we need to limit this to the image block, or would this work generally for all blocks as part of passing data into save()?

I think it's going to be our strategy for all blocks that have attributes that we want to support Block Bindings, and whose presence determines whether or not certain elements are included in the markup. (Another example would be the Pullquote block's citation attribute which, if present, leads to a <cite> tag being included.)

AFAICS, this will always require some logic in the individual block's save() method, as only the block can know which elements depend on which attributes.

For instance, what if we provided a value for the block bindings to the attributes on save() such that each block rendered assuming that content were there?

We could save some zero-width space, or an internal Unicode code point, or a zero, or whatever is appropriate for the type of the attribute, and clear it out after save(). Perhaps that would at least cover existing issues with conditionally-generated HTML structure?

Based on the points you raised on this ticket, and on some further discussion with @gziolo and @cbravobernal, our current thinking is to conditionally remove empty elements in the block's render method, rather than trying a more "magic" approach that would attempt to work for all such blocks.

IIUC, the placeholder characters you're suggesting would unlock a more generic way of removing "unneeded"/empty HTML elements. They do seem to come with some drawbacks; for example, as you hinted at, we might need to make them compatible with different attribute types. Do you think they're overall preferential over "making it the individual block's problem"?

IIUC, the placeholder characters you're suggesting would unlock a more generic way of removing "unneeded"/empty HTML elements. They do seem to come with some drawbacks; for example, as you hinted at, we might need to make them compatible with different attribute types. Do you think they're overall preferential over "making it the individual block's problem"?

I still believe that it will be simpler and more reliable and easier for developers to simply duplicate their block rendering code in PHP than constructing all of these systems.

However, I hadn’t considered previously that we might have ways to automate this a bit in a more reliable way. Actually this is more akin to the approach Bits would provide were they were. The Bit itself is content inside an attribute so the situation kind of takes care of itself.

Basically, I think it’s reasonable to assume that the presence of a binding suggests non-empty data. This is definitely not always the case, but usually will be. A post’s author or item’s price will likely be there, otherwise we wouldn’t add them as a binding. Conditional logic has to be handled in any system using it.

On the other hand, the placeholder is a blurry way of saying, “just store a Bit”. By passing bindings into attribute on the way into the save() function maybe we can ignore the problems just enough that in most cases the system handles itself properly. We won’t have to solve the common problem of finding an element which isn’t there (the FIGCAPTION or CITE elements, for example), meanwhile the simpler problem of removing an empty element is still there.

I still believe that it will be simpler and more reliable and easier for developers to simply duplicate their block rendering code in PHP than constructing all of these systems.

Correct me if I'm wrong. Do you propose we start working on bits, and use them too as placeholders for bindings?

That way, the bit is added on the save.js function, and later processed in PHP?

How do you imagine adding a figcaption? Can you provide an example?

Correct me if I'm wrong. Do you propose we start working on bits, and use them too as placeholders for bindings?

Well I do think that Bits is still good to work on, but a different problem than this one, as Bits still don’t answer the question of what to do when markup is conditional, or computed based on the value of the Bit.

That way, the bit is added on the save.js function, and later processed in PHP?

If we do store the Bit as the placeholder then we don’t have to do anything on the Gutenberg side — the Image block will save a <ficaption> because there’s content inside of the caption attribute. It’s a Bit placeholder, meaning it’s not an empty string.

How do you imagine adding a figcaption? Can you provide an example?

The main thrust of what I was getting at with the quote you provided is that I think building a generalized system to handle conditional rendering is going to be a deadend and we’ll hit it fast. My proposal is that if someone wants to add or remove FIGURE and FIGCAPTION markup, it will be easier for them and easier for us and easier for everyone if we avoid creating a new third system for trying to harmonize between JS and PHP. The cognitive overhead of learning a new custom system, the lack of documentation, the lack of debugging tools, and the introduction of new complications and bugs will far outweigh the costs of duplicating the JS logic from save() into a PHP function that replaces the markup dynamically on render.

Whether by block Bindings or by Bits, these realized values can be passed into the render callback and the block rendering code need not be aware of the fact that an attribute’s value is pulled from a binding vs. having been written statically. All it knows is that there is HTML already there and there are concrete attribute values.