Block Bindings: Fix label in pattern overrides#65302
Closed
artemiomorales wants to merge 2 commits into
Closed
Conversation
artemiomorales
added
[Type] Bug
artemiomorales
requested review from
ajitbohra,
ellatrix and
tellthemachines
as code owners
artemiomorales
requested review from
SantosGuillamot,
cbravobernal and
gziolo
|
Size Change: +74 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
artemiomorales
commented
Sep 13, 2024
| : null; | ||
| blockContext[ 'pattern/overrides' ] = | ||
| parentBlock?.attributes?.content; | ||
| } |
Contributor
Author
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 🙏
Contributor
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' | ||
| ) { |
Contributor
Author
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?
Contributor
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.
Contributor
Author
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.
Contributor
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:
Contributor
Author
|
As the base PR for this is no longer relevant, I'm going to close this. |
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
2 participants
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