Fix applying bindings or pattern overrides to button blocks with empty text #62220
Conversation
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. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ packages/block-library/src/button/index.php ❔ lib/blocks.php |
| $p = new WP_HTML_Tag_Processor( $content ); | ||
|
|
||
| /* | ||
| * The button block can render an `<a>` or `<button>` and also has a | ||
| * `<div>` wrapper. Find the a or button tag. | ||
| * | ||
| */ | ||
| $tag = null; | ||
| while ( $p->next_tag() ) { | ||
| $tag = $p->get_tag(); | ||
| if ( 'A' === $tag || 'BUTTON' === $tag ) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If this happens, the likelihood is there's no block content. | ||
| if ( null === $tag ) { | ||
| return ''; | ||
| } | ||
|
|
||
| // Build a string of the inner text. | ||
| $text = ''; | ||
| while ( $p->next_token() ) { | ||
| switch ( $p->get_token_name() ) { | ||
| case '#text': | ||
| $text .= $p->get_modifiable_text(); | ||
| break; | ||
|
|
||
| case 'BR': | ||
| $text .= ''; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
I haven't written a lot of tag processor code, so would be happy to receive feedback on this.
There was a problem hiding this comment.
this is great @talldan! here are some thoughts. it looks like you're attempting to remove all content inside the button except for the plain text content. that is, no formatting, no spans, no inline images, etc… I don't speak to whether that's a great idea or not, but I can provide feedback for the HTML API use in doing so.
first of all, this is technically the domain of the HTML Processor, which knows HTML structure, but since that's not really ready yet this will suffice. you did the right thing using the Tag Processor.
this code, however, is going to grab only the first text nodeat the start of the A or BUTTON. Maybe we don't expect any other content, but if you want to (unreliably) limit it to the inner contents, then you can add one more step which would improve the reliability a little bit and capture more text.
// Build a string of the inner text.
$text = '';
while ( $p->next_token() ) {
switch ( $p->get_token_name() ) {
/*
* Neither `A` nor `BUTTON` can nest, so it doesn't matter
* if these are opening or closing tags. Both will close
* the open element.
*/
case $tag:
break;
case '#text':
$text .= $p->get_modifiable_text();
continue;
case 'BR':
$text .= "\n";
continue;
}
}There was a problem hiding this comment.
case $tag:
break;
Thanks for the feedback! Should this also break out of the while loop in addition to the switch? I believe in PHP break 2 can be used to break two levels, but I might change it from a switch to an if statement.
There was a problem hiding this comment.
it looks like you're attempting to remove all content inside the button except for the plain text content. that is, no formatting, no spans, no inline images, etc… I don't speak to whether that's a great idea or not, but I can provide feedback for the HTML API use in doing so.
The goal in this PR is for the replace the old static implementation (removed in this PR):
gutenberg/packages/block-library/src/button/save.js
Lines 34 to 36 in dbf2014
Retaining the HTML from the save function allows for bindings to work with empty buttons.
It looks like the conditional logic introduced as a result of this issue - #17221.
It's not an accessibility reason. How many years have I incorrectly believed that?! I'll update the comment in the code.
There was a problem hiding this comment.
I did some further testing and realized there's also the case of inline images. I've now simplified the code down to just this:
// If the next token is the closing tag, the button is empty.
$p->next_token();
$is_empty = $p->get_token_name() === $tag;
if ( $is_empty ) {
return '';
}Which should better match how RichText.isEmpty works (even a space is considered content) - https://github.com/WordPress/gutenberg/blob/trunk/packages/rich-text/src/is-empty.js.
I should've endeavoured to match that implementation first.
Anything I might be overlooking here? I'll work on adding some tests for this.
There was a problem hiding this comment.
It's probably good enough, but keep in mind that there could be comments in there too.
$is_empty = true;
while ( $p->next_token() && $tag !== $p->get_token_name() && $is_empty ) {
if ( '#comment' !== $p->get_token_type() ) {
// Anything else implies this is not empty.
$is_empty = false;
}
}
talldan
added
the
No Core Sync Required
talldan
changed the title
|
Size Change: -11 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
talldan
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
This is a change I don't fully understand, so I'm probably overlooking something, but it looks like it wipes out all buttons, which seems non-optimal. Should we be checking for some condition before trashing the HTML?
Also even when we choose to, it might help to provide more insightful checks than "we didn't find an A or a BUTTON" in case plugin code has modified the markup in a way we don't expect. Perhaps we assume that if the tags are missing then someone else modified the content intentionally and we should leave it alone.
If this is for the block bindings, would it help avoid unintentional corruption if we looked for some block attribute indicating that we intend to perform Block Binding on it?
I'm just a bit nervous from this that the effect is a broad stroke that introduces normal corruption for the sake of fixing one much-rarer edge case. But like I acknowledge, I'm probably missing something major.
| $p = new WP_HTML_Tag_Processor( $content ); | ||
|
|
||
| /* | ||
| * The button block can render an `<a>` or `<button>` and also has a | ||
| * `<div>` wrapper. Find the a or button tag. | ||
| * | ||
| */ | ||
| $tag = null; | ||
| while ( $p->next_tag() ) { | ||
| $tag = $p->get_tag(); | ||
| if ( 'A' === $tag || 'BUTTON' === $tag ) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If this happens, the likelihood is there's no block content. | ||
| if ( null === $tag ) { | ||
| return ''; | ||
| } | ||
|
|
||
| // Build a string of the inner text. | ||
| $text = ''; | ||
| while ( $p->next_token() ) { | ||
| switch ( $p->get_token_name() ) { | ||
| case '#text': | ||
| $text .= $p->get_modifiable_text(); | ||
| break; | ||
|
|
||
| case 'BR': | ||
| $text .= ''; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
this is great @talldan! here are some thoughts. it looks like you're attempting to remove all content inside the button except for the plain text content. that is, no formatting, no spans, no inline images, etc… I don't speak to whether that's a great idea or not, but I can provide feedback for the HTML API use in doing so.
first of all, this is technically the domain of the HTML Processor, which knows HTML structure, but since that's not really ready yet this will suffice. you did the right thing using the Tag Processor.
this code, however, is going to grab only the first text nodeat the start of the A or BUTTON. Maybe we don't expect any other content, but if you want to (unreliably) limit it to the inner contents, then you can add one more step which would improve the reliability a little bit and capture more text.
// Build a string of the inner text.
$text = '';
while ( $p->next_token() ) {
switch ( $p->get_token_name() ) {
/*
* Neither `A` nor `BUTTON` can nest, so it doesn't matter
* if these are opening or closing tags. Both will close
* the open element.
*/
case $tag:
break;
case '#text':
$text .= $p->get_modifiable_text();
continue;
case 'BR':
$text .= "\n";
continue;
}
}
|
If I am not mistaken, we don't need to process the content and modify it. Right now, the modifications of the block attributes to apply the bindings are done during the rendering. This means that we can check if the text attribute exists (or if it is empty). In that case, we just return null. And if not, we just return the content. Something like this: if ( empty( $attributes["text"] ) ) {
return null;
} else {
return $content;
}I quickly tested and it seems to work, but I didn't take a close look at the different scenarios. Do you think something like that could work? |
@SantosGuillamot My first hope is that the solution would be as simple as that, but I don't think it is. My understanding is that what you propose will only work when bindings are applied ( |
|
Oh, okay. I had assumed that
If that's the case and I'm understanding it correctly, @dmsnell can help better identify the easiest way to do that check now. I can't say if the current approach is the right one or not. If I am not mistaken, and according to this comment, in the future, we could potentially rely on the HTML processor to read the inner text (something like if ( ! empty( $attributes[ "text" ] ) ) {
return $content;
}
while ( $p->next_tag() ) {
$tag = $p->get_tag();
if ( 'A' === $tag || 'BUTTON' === $tag ) {
if ( $p->get_inner_text() ){
return $content;
} else {
return null;
}
}
}But again, I believe this is not possible right now and I am not that familiar with the HTML API, so it may be wrong. |
|
Flaky tests detected in 9de03b5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9445102255
|
|
From my testing in a pattern with the empty second button. <!-- wp:block {"ref":199,"content":{"2":{"text":"123"}}} /-->In the editor it looks correctly:
However, on the frontend, the second button disappears and magically erases the first character. Is there something I'm doing wrong? |
I couldn't reproduce it, and I'm not sure how that could happen. I'll do some more testing. It has been pretty challenging working on this fix as my local xdebug isn't working for block PHP. Not sure what's causing that. |
|
There's one caveat that I'll note for testing, this won't work for patterns already saved before you checked out this branch. Those patterns are already missing the button markup, so there's very little that can be done to fix them (beyond making the button block completely dynamic). And as a side note, I managed to figure out what was happening with xdebug. A breakpoint needs to be set in the |
I can only reproduce it if the override for the empty button starts with a number. e.g. |
Yep, I can reproduce it now, thanks for the extra info! |
|
I can reproduce that in |
|
I just answered in the mentioned issue:
|
|
I've tested it and it seems to work fine! Regarding bindings, I think it is ready. But I'll defer it to @dmsnell because most of the changes are related to how to use the HTML Tag Processor. |
talldan
force-pushed
the
update/button-block-optional-rendering-to-be-dynamic
branch
from
61f1e67
to
f622395
Compare
…y text (#62220) * Dynamically check button text and avoid rendering a button when empty * Handle both anchor and button tags * Fix lint issues * Address review feedback * Simplify checks for empty content * Check for comment tokens * Update native test snapshots * Update initial HTML native test snippet ---- Co-authored-by: talldan <[email protected]> Co-authored-by: dmsnell <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
…y text (#62220) * Dynamically check button text and avoid rendering a button when empty * Handle both anchor and button tags * Fix lint issues * Address review feedback * Simplify checks for empty content * Check for comment tokens * Update native test snapshots * Update initial HTML native test snippet ---- Co-authored-by: talldan <[email protected]> Co-authored-by: dmsnell <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
…y text (WordPress#62220) * Dynamically check button text and avoid rendering a button when empty * Handle both anchor and button tags * Fix lint issues * Address review feedback * Simplify checks for empty content * Check for comment tokens * Update native test snapshots * Update initial HTML native test snippet ---- Co-authored-by: talldan <[email protected]> Co-authored-by: dmsnell <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: SantosGuillamot <[email protected]>
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
|
This was cherry-picked to the wp/6.6 branch. |
What?
Fixes #61303
An issue for block bindings and pattern overrides is when a button block's text is left empty, the editor saves no html due to a conditional return in the block's save function.
When a binding tries dynamically to update a button block in this state, it won't be able to due to the missing content.
This PR changes the button block so that the conditional rendering is implemented dynamically on the server. This allows block bindings to dynamically change the content of the block, even when there's empty text.
How?
Uses the tag processor to check for inner content.
The
textof a button block is a sourced attribute, so the sever is unable to parse the attribute value, so it needs to be read from the content.Testing Instructions
Expected: The button should appear with the override text.
As a general smoke test, it's also worth checking that buttons outside of patterns continue to work as expected when they have and do not have text.
You should also be able to add a button that has text to a pattern, and then override it with no text, and it won't render in the frontend
Screenshots or screencast
Editor
Frontend