Block Bindings: Fix label in pattern overrides #65302
Closed
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
artemiomorales
added
[Type] Bug
artemiomorales
requested review from
tellthemachines,
ellatrix and
ajitbohra
as code owners
artemiomorales
requested review from
gziolo,
SantosGuillamot and
cbravobernal
|
Size Change: +74 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
artemiomorales
commented
Sep 13, 2024
| : null; | ||
| blockContext[ 'pattern/overrides' ] = | ||
| parentBlock?.attributes?.content; | ||
| } |
There was a problem hiding this comment.
This doesn't seem like the most elegant solution to me. Is there another way to get the pattern attributes other than to look up the values manually and insert them like this?
Also, I'm unsure if we can assume that the first parent will be the pattern. Gutenberg seems to flatten the hierarchy of blocks in pattern instances, so this appears to work, but it'd be great to additional insight on this 🙏
There was a problem hiding this comment.
Also, I'm unsure if we can assume that the first parent will be the pattern. Gutenberg seems to flatten the hierarchy of blocks in pattern instances, so this appears to work, but it'd be great to additional insight on this
Yep I think you'll need to iterate through all block parents to find the core/block block. Also need to take into account a pattern within a nested pattern, and ensure you find the nearest one.
There's a few places with this kind of code 😅
Just to note, this component would be removed if #65204 lands as is.
|
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. |
artemiomorales
commented
Sep 13, 2024
| if ( | ||
| customName && | ||
| ! attributes?.metadata?.bindings?.source === 'pattern/overrides' | ||
| ) { |
There was a problem hiding this comment.
I don't like the hardcoding of pattern/overrides here. I also don't like that the pattern special case would need to be accounted for in every relevant block. Should we explore an alternative solution?
There was a problem hiding this comment.
I think the name is wrong ('core/pattern-overrides').
On a different note, I'm wondering whether this is a pattern override specific issue. The problem seems to be that when a paragraph is named, the name is announced instead of the content. It is perhaps worse for pattern overrides since all editable blocks will be named, but the problem still exists for regular paragraphs too.
Either way it seems like screenreader users might miss valuable context (if the name isn't announced a user might wonder why given they explicitly gave it the name).
A compromize could be to announce both the name and the content if it can be made clear enough.
There was a problem hiding this comment.
I think the name is wrong ('core/pattern-overrides').
pattern/overrides is how it's defined in the block context, so I was just following that pattern:
On a different note, I'm wondering whether this is a pattern override specific issue. The problem seems to be that when a paragraph is named, the name is announced instead of the content. It is perhaps worse for pattern overrides since all editable blocks will be named, but the problem still exists for regular paragraphs too.
Either way it seems like screenreader users might miss valuable context (if the name isn't announced a user might wonder why given they explicitly gave it the name).
A compromize could be to announce both the name and the content if it can be made clear enough.
This is a good point. I think announcing both the name and content is the right approach. I can implement that change as part of this PR if we decide to move forward with it, or create an issue and perhaps handle in a separate PR if not.
There was a problem hiding this comment.
pattern/overrides is how it's defined in the block context, so I was just following that pattern:
This particular line refers to the binding source, which is defined here:
|
As the base PR for this is no longer relevant, I'm going to close this. |
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?
This PR builds on top of #65114 to fix the label in patterns.
Why?
#65114 does not work for patterns, and it's important for screen readers to announce accurate content when users are navigating and editing.
How?
It adds pattern information to the current block context in the BlockSelectionButton by searching the parent blocks, then passes that context to the normal block bindings logic, allowing us to retrieve the override values.
It also modifies
__experimentalLabelin the paragraph block so that the override value is used.Testing Instructions