Block Bindings / Pattern Overrides: Prevent normal attribute updates when a __default binding exists #62471
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
|
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. |
|
Size Change: +689 B (+0.04%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f54d88c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9478091565
|
Comment on lines
100
to
101
| const hasDefaultBinding = | ||
| props.attributes.metadata?.bindings?.[ DEFAULT_ATTRIBUTE ]; |
There was a problem hiding this comment.
Instead of just checking if it has the __default attribute, maybe we can check that it is core/pattern-overrides as we are doing here? If we do that, we might want to change the name of the variable.
If we decide to support __default in any source in the future, I am not sure if skipping setAttributes for any attribute would be okay for all of them. It seems specific to pattern overrides to me.
Additionally, adding the __default property pointing to any other source, currently not supported, would skip setAttributes as well.
There was a problem hiding this comment.
Sounds good. I've made that change 👍
Ultimately I'm not sure if this PR is the right fix, but it's a small change, and I can't think of many other options.
SantosGuillamot
approved these changes
Jun 12, 2024
talldan
deleted the
try/default-binding-prevents-normal-attribute-updates
branch
talldan
added
the
Backport to WP 6.7 Beta/RC
talldan
changed the title
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…pdates when a __default binding exists (WordPress#62471) * Try preventing normal attribute updates when a __default binding exists * Add an e2e test * Only skip normal attribute updates for pattern overrides ---- Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
29 tasks
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
…pdates when a __default binding exists (#62471) * Try preventing normal attribute updates when a __default binding exists * Add an e2e test * Only skip normal attribute updates for pattern overrides ---- Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
…pdates when a __default binding exists (#62471) * Try preventing normal attribute updates when a __default binding exists * Add an e2e test * Only skip normal attribute updates for pattern overrides ---- Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
|
I just cherry-picked this PR to the wp/6.6-beta-3 branch to get it included in the next release: 79d03f3 |
ellatrix
added
Backported to WP Core
ellatrix
pushed a commit
that referenced
this pull request
Jun 18, 2024
…pdates when a __default binding exists (#62471) * Try preventing normal attribute updates when a __default binding exists * Add an e2e test * Only skip normal attribute updates for pattern overrides ---- Co-authored-by: talldan <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
This was referenced Jun 25, 2024
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?
See #62287, and specifically the comment here - #62287 (comment).
When overriding an image block, the image caption set in the media library can show and then unexpectedly disappear after a save and a refresh.
At the moment, the caption field is unsupported as a block binding (or pattern overrides) attribute, so it shouldn't be changeable from a pattern instance. This may change in the future, but that's a separate investigation.
Why?
The issue happens due to the way block bindings continues to update the unbound attributes for a block. When adding an image to an image block, there are a range of attributes that are set by the block (
url,id,alt,captionetc), and only some of these are supported by block bindings.The ones that are supported get set as pattern overrides correctly. The ones that aren't still cause the normal attributes of the block to be updated. This is why the caption can appear. After a refresh, the caption disappears because the blocks in a pattern instance aren't saved anywhere.
The block bindings behavior makes sense for most binding sources. For example, if you only bind an image src to post meta, it's still possible to edit the other attributes. But for patterns, that's not true, users can't edit the unbound attributes.
It's worth noting that it's not just caption, the image block also sets some other attributes that might cause unexpected bugs, so even if caption is supported by block bindings in the future, a change like this is still required.
How?
The PR addresses it by leveraging the
__defaultbinding added in #60694. Essentially it considers the__defaultbinding as covering all block attributes, but only the supported ones have active bindings.Testing Instructions
In this branch: the caption doesn't appear on the image
In trunk: the caption appears on the image but disappears after a refresh
Screenshots or screencast
Before
Kapture.2024-06-11.at.16.58.10.mp4
After
Kapture.2024-06-11.at.17.00.57.mp4