Block Bindings: Update label for bound paragraphs and headings #65114
Conversation
artemiomorales
added
[Type] Bug
artemiomorales
requested review from
gziolo,
afercia,
SantosGuillamot and
cbravobernal
|
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. |
|
I assume this is going to be the case for other blocks and attributes. For example, does this happen with the heading content or the button text? If that's the case, I believe we probably need to think about it more broadly and solve it for any block attribute, potentially using the bound value instead of a default message. |
|
If accepted, it should be replicated with other blocks that can be connected. The challenge is that the attribute connected isn'tand always the most important one, connections for attributes editable in the sidebar shouldn't become too prominent. |
|
To start, I just hardcoded a new label for bound paragraphs to get the conversation started. Ideally, we would retrieve the actual binding value, but I'm not sure what would be the best way to go about that. The logic for reading the bindings value is currently in use-binding-attributes.js, and I'm not sure it's necessary to get so involved. In addition to some text indicating that the content is dynamic, perhaps even just using the binding source and key could suffice? That information is already easily available in the block attributes. |
|
Size Change: -9.94 kB (-0.56%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Personally, I don't feel comfortable hardcoding something that just works for the paragraph. If it is the |
@SantosGuillamot This indeed happens with the heading block.
If it's just the paragraph and heading, do you think that warrants a broader solution? Potentially we could intervene at an earlier level. For example, this is where the gutenberg/packages/blocks/src/api/utils.js Lines 148 to 173 in 1a8aa20 |
artemiomorales
added
the
Needs Accessibility Feedback
Is it expected or it is just because we aren't passing I'm not familiar with this part of the codebase, but I guess we will encounter this problem for any block using For example, it seems it is also used in the image block: link. I haven't tested it, but maybe there is the same problem there. There are already a bunch of blocks relying on that, and it could even create more issues when we add support for more blocks. Looking at the |
I believe it's because __experimentalLabel hasn't been defined for the button block.
Ok I've started looking at devising a general solution for this.
Yes, the same problem exists with the image block, except that the image uses the |
|
Flaky tests detected in 38eb29c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10786914084
|
| function labelWithBlockBindings( settings ) { | ||
| return ( attributes, { context } ) => { | ||
| if ( attributes?.metadata?.bindings && context === 'accessibility' ) { | ||
| return 'Overriding label'; |
There was a problem hiding this comment.
For now, I just check whether the paragraph is bound and, if so, return the label
Paragraph with dynamic content.
Is Overriding label a leftover added for testing? In any case, it should be translatable.
There was a problem hiding this comment.
Thanks for the feedback! Rather than hardcoding this, I've now modified the PR so it reads the actual bindings values. I've updated the PR description accordingly.
artemiomorales
changed the title
|
Update: I've revised the PR so that it retrieves the actual bindings value and uses it for the label. The PR description has been updated accordingly. Note that pattern overrides required some additional handling. I explore a solution in #65302, which is meant to accompany this PR or be merged shortly after. |
|
I didn't find time to properly review this yet (it's on my to-do list). But there are a couple of things I wanted to ask:
|
|
@SantosGuillamot As far as I can tell, this is root of the problem: The block bindings hook in Here's where the attributes are overridden in the gutenberg/packages/block-editor/src/hooks/use-bindings-attributes.js Lines 304 to 312 in b7c326c Worth noting is that gutenberg/packages/block-editor/src/components/block-edit/index.js Lines 57 to 99 in 03d93b4 Both gutenberg/packages/block-editor/src/components/block-list/block.js Lines 130 to 152 in 03d93b4 And the gutenberg/packages/block-editor/src/components/block-canvas/index.js Lines 26 to 85 in de01da2 The To solve this, we could potentially refactor to wrap the Another option, like I've done here, is to replicate the logic to compute the bindings values in the
It's not really a different solution — it's essentially replicating the logic that we normally use in As far as other registered sources goes, this issue with the label is currently specific to sources that override the
So in summary, I see two solutions:
Perhaps there's an easy way to do the first approach, though I just wanted to get this working and keep iterating, and I didn't want to go down that potential rabbit hole unless we felt it was necessary. I could be off base here, though. Would be happy to hear any additional thoughts or insights 🙏 |
|
I've been trying to test the original issue and consequently the potential fix, but it seems the navigation mode was removed in this pull request. More context can be found in this other issue, where a new mechanism is being discussed. Having said that, I think this raises again the discussion about when to use the original block attributes and when to use the binding values. You can find more information in this old pull request, where that possibility was explored. My gut says we need to provide a way to let developers decide if they want to use the binding values or the original attributes, but the use cases weren't clear at that moment. This could be done via a new selector or even accepting a new parameter in |
Ok, on one hand, we could try to land this in core then, but any effort invested into a solution likely won't be viable for more than one release. I think we should see what the new mechanism ends up being before investing more time into this.
I think we could potentially offer the developers a way to customize what's used for the label, but what would the sensible default be? I think creating a label like, "{Name of attribute}: {Binding value}" could work well in the majority of cases. I can't say that for sure, though any approach we take would likely be better than the current one with post meta, where it just reads the original content. |
|
I may be wrong here but, if I remember this particular issue was only a problem for the Navigation mode, right? I think it was working fine in my tests in other modes. When I said "provide a way to let developers decide if they want to use the binding values or the original attributes" I wasn't referring to letting them decide the label discussed in this particular issue, I was referring to providing a method to let any developer, in any part of the code, decide if they want to use the original attributes or the values of the bindings. |
Right, since Navigation Move has been removed, then this PR is no longer relevant. Keyboard navigation no longer works well in the editor, but that's a bigger issue than just block bindings.
I think we're both in agreement here; I was just thinking of what a good default would be. In any case, I'm going to close this. We can continue the conversation at a later date if it becomes relevant. |
What?
This PR fixes the label for bound paragraphs and headings, using their bindings value rather than fallback content.
Why?
Addresses #62933
The label is currently read out loud by screen readers. Unless we update the label for bound blocks, screen readers will simply announce the value in the markup, which is misleading, as the actual content will be rendered by the block bindings API.
How?
It replicates some logic from
use-bindings-attributes.jsinside ofblock-selection-button.jsin order to retrieve the bindings value, and also extracts key logic into a newgetBindingsValuesfunction so that it can be reused.Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. Add heading and paragraph blocks bound to a custom field using the Code Editor