Block Bindings: Remove caption from edit on EditWithGeneratedProps.#73552
Merged
cbravobernal merged 3 commits intotrunkfrom Dec 15, 2025
Merged
Block Bindings: Remove caption from edit on EditWithGeneratedProps.#73552cbravobernal merged 3 commits intotrunkfrom
EditWithGeneratedProps.#73552cbravobernal merged 3 commits intotrunkfrom
Conversation
cbravobernal
added
the
[Type] Bug
|
Size Change: -13 B (0%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
cbravobernal
requested review from
ajitbohra,
ellatrix and
fabiankaegy
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @Copilot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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
reviewed
Dec 11, 2025
Comment on lines
266
to
269
| if ( isCaptionBound ) { | ||
| // Explicitly set caption to undefined to clear it when bound. | ||
| mediaAttributes.caption = undefined; | ||
| } else { |
Contributor
There was a problem hiding this comment.
Thanks for this PR, Carlos!
I was unsure behind the reason for adding this if ( captionBound ) logic. The image block (when unbound) keeps the existing caption if the user replaces the image with one that doesn't have a media library caption. So the new code added here changes the way the image block works.
TBH while I do personally prefer the behavior you implemented, I think it might be best to stick with what the image block does already so that it works consistently. What do you think?
Contributor
Author
There was a problem hiding this comment.
@copilot how could we apply the fix that Daniel told us un the previous comment?
Contributor
There was a problem hiding this comment.
I agree with @danr here -- the PR should probably be limited to making the behavior of a bound image block consistent with the unbound behavior. If we consider changing the overall behavior (since it might be more intuitive), that should be done separately.
Contributor
There was a problem hiding this comment.
Makes me wonder if the existing behavior (in trunk) is fine for bound image blocks as well (if it reflects that of unbound ones)? 🤔 Or was there any other bug that this PR is meant to fix?
Maybe this removal of the delete keptAttributes.caption line is all we need? (Removing that should have probably been part of #71483 or #72476, but I missed it at the time.)
Contributor
There was a problem hiding this comment.
In any case, it's probably a good idea to add regression tests to enshrine the desired behavior.
(For unbound image blocks, that should probably be the current behavior, per Dan's comment.)
Contributor
|
@cbravobernal I've opened a new pull request, #73914, to work on those changes. Once the pull request is ready, I'll request review from you. |
…havior (#73914) * Initial plan * Revert caption handling to preserve existing behavior Unlinked contributors: Copilot. Co-authored-by: talldan <[email protected]> Co-authored-by: cbravobernal <[email protected]> ---------
|
Flaky tests detected in f8c924d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20177434150
|
talldan
approved these changes
Dec 15, 2025
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
Thanks @cbravobernal, feels nice to delete some code!
This file contains hidden or 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
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.
What?
Following the previous comment. Says that href and caption should be remove from that function.
This PR introduced it originally and has the testing instructions: Pattern overrides: Fix aspect ratio not working in image with overrides. It looks like when replacing images the code I linked above suppressed setting a caption (the image in the media library might have a caption). Perhaps it’s possible to delete that line and allow the caption to be set now.
The aspect ratio is still working as expected.
Fixing the caption in pattern overrides
With this PR, the image caption attribute ( the one that is set on the media options ) will be the source of truth to show or not to show the caption when the override happens.
In order to test it, you can do two workflows:
no-caption-to-caption.mp4
caption-to-no-caption.mp4
I'm happy to add regression tests if you consider this is how it should be.