-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Block Bindings: Support Image block's caption attribute, remove <figcaption> if empty
#9702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,7 +109,7 @@ class WP_Block { | |
| private const BLOCK_BINDINGS_SUPPORTED_ATTRIBUTES = array( | ||
| 'core/paragraph' => array( 'content' ), | ||
| 'core/heading' => array( 'content' ), | ||
| 'core/image' => array( 'id', 'url', 'title', 'alt' ), | ||
| 'core/image' => array( 'id', 'url', 'title', 'alt', 'caption' ), | ||
| 'core/button' => array( 'url', 'text', 'linkTarget', 'rel' ), | ||
| 'core/post-date' => array( 'datetime' ), | ||
| ); | ||
|
|
@@ -434,7 +434,18 @@ private function replace_html( string $block_content, string $attribute_name, $s | |
| ) ) { | ||
| // TODO: Use `WP_HTML_Processor::set_inner_html` method once it's available. | ||
| $block_reader->release_bookmark( 'iterate-selectors' ); | ||
| $block_reader->replace_rich_text( wp_kses_post( $source_value ) ); | ||
| /* | ||
| * If the value returned from the Block Bindings source is empty | ||
| * for a block attribute that whose selector is `rich-text` or `html`, | ||
| * we remove the HTML node denoted by its selector. For example, this | ||
| * means removing an Image block's `<figcaption>` node if there's no | ||
| * caption supplied. | ||
| */ | ||
| if ( empty( $source_value ) ) { | ||
| $block_reader->remove_node(); | ||
| } else { | ||
| $block_reader->replace_rich_text( wp_kses_post( $source_value ) ); | ||
| } | ||
| return $block_reader->get_updated_html(); | ||
| } else { | ||
| $block_reader->seek( 'iterate-selectors' ); | ||
|
|
@@ -502,6 +513,37 @@ public function replace_rich_text( $rich_text ) { | |
|
|
||
| return true; | ||
| } | ||
|
|
||
| public function remove_node() { | ||
| if ( $this->is_tag_closer() ) { | ||
| return false; | ||
| } | ||
|
|
||
| $depth = $this->get_current_depth(); | ||
|
|
||
| $this->set_bookmark( '_wp_block_bindings_tag_opener' ); | ||
| // The bookmark names are prefixed with `_` so the key below has an extra `_`. | ||
| $tag_opener = $this->bookmarks['__wp_block_bindings_tag_opener']; | ||
| $start = $tag_opener->start; | ||
| $this->release_bookmark( '_wp_block_bindings_tag_opener' ); | ||
|
|
||
| // Find matching tag closer. | ||
| while ( $this->next_token() && $this->get_current_depth() >= $depth ) { | ||
| } | ||
|
Comment on lines
+530
to
+532
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need these condition?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is needed. It's a loop that advances the internal cursor until a matching tag closer is found for the opener that we started from. |
||
|
|
||
| $this->set_bookmark( '_wp_block_bindings_tag_closer' ); | ||
| $tag_closer = $this->bookmarks['__wp_block_bindings_tag_closer']; | ||
| $end = $tag_closer->start + $tag_closer->length; | ||
| $this->release_bookmark( '_wp_block_bindings_tag_closer' ); | ||
|
|
||
| $this->lexical_updates[] = new WP_HTML_Text_Replacement( | ||
| $start, | ||
| $end - $start, | ||
| '' | ||
| ); | ||
|
|
||
| return true; | ||
| } | ||
| }; | ||
|
|
||
| return $internal_processor_class::create_fragment( $block_content ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -83,6 +83,16 @@ public function data_update_block_with_value_from_source() { | |||||||||||||||||||||
| , | ||||||||||||||||||||||
| '<div class="wp-block-button"><a class="wp-block-button__link wp-element-button">test source value</a></div>', | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| 'image block' => array( | ||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my testing experience described in WordPress/gutenberg#71483 (comment), we might need some coverage when replacing HTML attributes, too. In that particular case,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see it's covered in wordpress-develop/tests/phpunit/tests/block-bindings/render.php Lines 414 to 423 in e2560f1
That might be an issue in the Gutenberg implementation. However, adding more diverse cases here could still be beneficial.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Test coverage for this scenario was added on the GB side as part of the fix to the bug you mentioned ( The bug was specific to GB's compat layer (and can't manifest in Core in the same way), but we can carry over the test coverage to Core just for parity's sake.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done over at the new PR: 339fbe8 |
||||||||||||||||||||||
| 'caption', | ||||||||||||||||||||||
| <<<HTML | ||||||||||||||||||||||
| <!-- wp:image {"id":66,"sizeSlug":"large","linkDestination":"none"} --> | ||||||||||||||||||||||
| <figure class="wp-block-image size-large"><img src="breakfast.jpg" alt="" class="wp-image-1"/><figcaption class="wp-element-caption">Breakfast at a <em>café</em> in Wrocław.</figcaption></figure> | ||||||||||||||||||||||
| <!-- /wp:image --> | ||||||||||||||||||||||
| HTML | ||||||||||||||||||||||
| , | ||||||||||||||||||||||
| '<figure class="wp-block-image size-large"><img src="breakfast.jpg" alt="" class="wp-image-1"/><figcaption class="wp-element-caption">test source value</figcaption></figure>', | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -133,6 +143,50 @@ public function test_update_block_with_value_from_source( $bound_attribute, $blo | |||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| public function test_remove_containing_tag_if_rich_text_attribute_source_value_is_empty() { | ||||||||||||||||||||||
| $get_value_callback = function () { | ||||||||||||||||||||||
| return ''; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| register_block_bindings_source( | ||||||||||||||||||||||
| self::SOURCE_NAME, | ||||||||||||||||||||||
| array( | ||||||||||||||||||||||
| 'label' => self::SOURCE_LABEL, | ||||||||||||||||||||||
| 'get_value_callback' => $get_value_callback, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $block_content = <<<HTML | ||||||||||||||||||||||
| <!-- wp:image {"id":66,"sizeSlug":"large","linkDestination":"none"} --> | ||||||||||||||||||||||
| <figure class="wp-block-image size-large"><img src="breakfast.jpg" alt="" class="wp-image-1"/><figcaption class="wp-element-caption">Breakfast at a <em>café</em> in Wrocław.</figcaption></figure> | ||||||||||||||||||||||
| <!-- /wp:image --> | ||||||||||||||||||||||
| HTML; | ||||||||||||||||||||||
| $parsed_blocks = parse_blocks( $block_content ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $parsed_blocks[0]['attrs']['metadata'] = array( | ||||||||||||||||||||||
| 'bindings' => array( | ||||||||||||||||||||||
| 'caption' => array( | ||||||||||||||||||||||
| 'source' => self::SOURCE_NAME, | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $block = new WP_Block( $parsed_blocks[0] ); | ||||||||||||||||||||||
| $result = $block->render(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| $this->assertSame( | ||||||||||||||||||||||
| '', | ||||||||||||||||||||||
| $block->attributes['caption'], | ||||||||||||||||||||||
| "The 'caption' attribute should be updated with the empty string value returned by the source." | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| $this->assertSame( | ||||||||||||||||||||||
| '<figure class="wp-block-image size-large"><img src="breakfast.jpg" alt="" class="wp-image-1"/></figure>', | ||||||||||||||||||||||
| trim( $result ), | ||||||||||||||||||||||
| 'The <figcaption> and its rich text content should be removed.' | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Test if the block_bindings_supported_attributes_{$block_type} filter is applied correctly. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing function doc.