Skip to content
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

Blocks: Fix incorrect placement for hooked blocks in the parent container #54349

Merged
merged 7 commits into from
Sep 14, 2023

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Sep 11, 2023

What?

Fixes #54307.

Every block that has inner blocks also has some wrapping HTML element. The way hooked blocks are currently implemented is that it puts the injected block at exactly first (firstChild) or last place (lastChild) in the parsed block, which happens to be outside the wrapping element. A simplified example based on REST API response that shows the issue:

<!-- wp:group {"tagName":"main","style":{"spacing":{"margin":{"top":"var:preset|spacing|50","bottom":"var:preset|spacing|70"}}},"layout":{"type":"constrained"}} -->
    <main class="wp-block-group" style="margin-top:var(--wp--preset--spacing--50);margin-bottom:var(--wp--preset--spacing--70)">
        <!-- wp:pattern {"slug":"twentytwentythree/cta"} /-->
    </main>
    <!-- wp:ockham/like-button /-->
<!-- /wp:group /-->

Why?

The injected block is misplaced and therefore incorrectly styled on the front end. It should be with the main tag instead in the example provided:

<!-- wp:group {"tagName":"main","style":{"spacing":{"margin":{"top":"var:preset|spacing|50","bottom":"var:preset|spacing|70"}}},"layout":{"type":"constrained"}} -->
    <main class="wp-block-group" style="margin-top:var(--wp--preset--spacing--50);margin-bottom:var(--wp--preset--spacing--70)">
        <!-- wp:pattern {"slug":"twentytwentythree/cta"} /-->
        <!-- wp:ockham/like-button /-->
    </main>
<!-- /wp:group /-->

How?

Instead of injecting the hooked block as the first item (firstChild) or the last item (lastChild), there is no logic that tries to locate the exact place where the child block is marked with null in the array containing innerContent of the block. Once the first null value is found, then another null value is placed next to it. In the case, when there is no child block (no null value), then the null marker is added as the first item (firstChild) or the last item (lastItem) in the array.

Note: the handling for the case with no inner blocks but the existing HTML wrapper for inner blocks isn't handled perfectly. We can discuss options for improving that separately.

Testing Instructions

  1. Install and activate the plugin with the hooked block. Example block can be downloaded from https://github.com/ockham/like-button/releases/tag/v0.4.1.
  2. Activate Twenty Twenty-Three theme or any other theme that uses core/comment-template block.
  3. Go to the single post page and see where the Like Button gets injected. Ensure that all templates and template parts used on that page don't have any customizations applied.
  4. Check the source of the page and notice that the Like button is placed correctly before the closing tag for the comment row wrapper.

There is now a test coverage presenting how hooked blocks get injected depending on the state of the container block:

  • inner blocks present
  • no inner blocks present
  • no inner blocks and inner content present

Screenshots or screencast

Screenshot 2023-09-11 at 11 56 31 Screenshot 2023-09-11 at 13 22 39

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. labels Sep 11, 2023
@gziolo gziolo self-assigned this Sep 11, 2023
@github-actions
Copy link

github-actions bot commented Sep 11, 2023

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
❔ phpunit/fixtures/hooked-block/block.json
❔ phpunit/tests/blocks/renderHookedBlocks.php
❔ lib/compat/wordpress-6.4/block-hooks.php

@gziolo gziolo force-pushed the fix/incorrect-placement-hooked-blocks branch from 0b93c5e to ff1c019 Compare September 11, 2023 11:36
@gziolo gziolo requested a review from ockham September 11, 2023 13:14
@gziolo gziolo mentioned this pull request Sep 11, 2023
19 tasks
@gziolo
Copy link
Member Author

gziolo commented Sep 12, 2023

@ockham, should we add unit tests directly to WordPress core and only apply the fix to the logic in the Gutenberg plugin?

Well, if we are going to keep gutenberg_serialize_blocks then maybe it makes sense to add the tests here, too.

@ockham
Copy link
Contributor

ockham commented Sep 12, 2023

@ockham, should we add unit tests directly to WordPress core and only apply the fix to the logic in the Gutenberg plugin?

Well, if we are going to keep gutenberg_serialize_blocks then maybe it makes sense to add the tests here, too.

I think it makes sense to keep unit tests here, at least for now 👍 (Definitely good for confidence in the bugfix.)

Comment on lines 175 to 181
$chunk_index = count( $block['innerContent'] ) - 1;
for ( $index = $chunk_index; $index >= 0; $index-- ) {
if ( is_null( $block['innerContent'][ $index ] ) ) {
$chunk_index = $index;
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting that this sets $chunk_index to -1 if $block['innerContent'] is empty. Which I think is okay, since array_splice() also works if the offset arg is negative (see).

Copy link
Member Author

@gziolo gziolo Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I think I should set $chunk_index to 0 initially and only update when null is in the array. I now realized that it's possible that the array is empty 😄

I was studying unit tests for the parser earlier today, some interesting findings for innerContent:

describe( 'innerBlock placemarkers', () => {
test( 'innerContent exists', () => {
expect( parse( 'test' )[ 0 ] ).toHaveProperty( 'innerContent', [
'test',
] );
expect( parse( '<!-- wp:void /-->' )[ 0 ] ).toHaveProperty(
'innerContent',
[]
);
} );
test( 'innerContent contains innerHTML', () => {
expect(
parse( '<!-- wp:block -->Inner<!-- /wp:block -->' )[ 0 ]
).toHaveProperty( 'innerContent', [ 'Inner' ] );
} );
test( 'block locations become null', () => {
expect(
parse(
'<!-- wp:block --><!-- wp:void /--><!-- /wp:block -->'
)[ 0 ]
).toHaveProperty( 'innerContent', [ null ] );
} );
test( 'HTML soup appears after blocks', () => {
expect(
parse(
'<!-- wp:block --><!-- wp:void /-->After<!-- /wp:block -->'
)[ 0 ]
).toHaveProperty( 'innerContent', [ null, 'After' ] );
} );
test( 'HTML soup appears before blocks', () => {
expect(
parse(
'<!-- wp:block -->Before<!-- wp:void /--><!-- /wp:block -->'
)[ 0 ]
).toHaveProperty( 'innerContent', [ 'Before', null ] );
} );
test( 'blocks follow each other', () => {
expect(
parse(
'<!-- wp:block --><!-- wp:void /--><!-- wp:void /--><!-- /wp:block -->'
)[ 0 ]
).toHaveProperty( 'innerContent', [ null, null ] );
} );
} );

In particular, the fact that inner block locations always become null, so you can have multiple of them. However, if there are no inner blocks then we won't have null, and it can even be an empty array 🤔

I need to study some existing blocks to see how they serialize when there are no inner blocks. Group, Columns, Column, it might get tricky. I'm wondering if at least for core blocks we always enforce a HTML wrapper (I would assume yes, but it isn't a requirement for the parser), and whether we show the wrapper when there are no inner blocks inserter. I hope that with Columns that can be explored.

Basically, what @ockham raised in #54349 (comment).

Copy link
Member Author

@gziolo gziolo Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example for the Columns block with 3 columns and the middle one has no child blocks:

<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column -->
<div class="wp-block-column"><!-- wp:paragraph -->
<p>First</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column -->
<div class="wp-block-column"></div>
<!-- /wp:column -->

<!-- wp:column -->
<div class="wp-block-column"><!-- wp:paragraph -->
<p>Third</p>
<!-- /wp:paragraph --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->

A Group block with no child blocks:

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"></div>
<!-- /wp:group -->

A Group Block with a Cover block where I removed inner blocks:

<!-- wp:group {"layout":{"type":"constrained"}} -->
<div class="wp-block-group"><!-- wp:cover {"url":"http://localhost:8888/wp-content/uploads/2023/09/Maldives.jpg","id":18,"dimRatio":50,"layout":{"type":"constrained"}} -->
<div class="wp-block-cover"><span aria-hidden="true" class="wp-block-cover__background has-background-dim"></span><img class="wp-block-cover__image-background wp-image-18" alt="" src="http://localhost:8888/wp-content/uploads/2023/09/Maldives.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"></div></div>
<!-- /wp:cover --></div>
<!-- /wp:group -->

@ockham
Copy link
Contributor

ockham commented Sep 12, 2023

I wonder if there are any blocks with inner blocks that don't include any block wrapper HTML. It seems unlikely; I guess we'll find out (and can adjust the code accordingly if needed) 😅

In theory, a hooked block can be inserted as a first or last child into a block that doesn't have any inner blocks at all. This probably doesn't make any sense (and maybe we should forbid it; though that might not be straight-forward); in any case, I think our logic would work even then (and at least not crash 😬 ).

Furthermore, I wonder if there's ever going to be more than one consecutive non-null array element in innerContent (since any "plain" HTML is probably just represented as a string), so maybe we wouldn't even need the loop. But it also doesn't hurt to have it 😬

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One documentation nitpick; plus I agree that

It would be great to land #54293 first to remove the need for special code for bootstrapping PHP unit tests.

Other than that: :shipit:

@gziolo gziolo force-pushed the fix/incorrect-placement-hooked-blocks branch from 634c0a6 to 013f74c Compare September 13, 2023 13:26
@gziolo
Copy link
Member Author

gziolo commented Sep 13, 2023

I rebased with trunk after landing #54293. I addressed the feedback from @ockham. I plan to add more tests to cover edge cases as discussed in the comments.

@gziolo gziolo force-pushed the fix/incorrect-placement-hooked-blocks branch from 013f74c to b737b99 Compare September 14, 2023 07:38
@gziolo gziolo marked this pull request as ready for review September 14, 2023 07:38
@gziolo
Copy link
Member Author

gziolo commented Sep 14, 2023

I wonder if there are any blocks with inner blocks that don't include any block wrapper HTML. It seems unlikely; I guess we'll find out (and can adjust the code accordingly if needed) 😅

As documented in #54349 (comment), while the block parser allows inner blocks without a wrapper, in practice it doesn't happen because the way inner blocks are implemented in the block editor enforces it with the current design. The block wrapper is necessary for the UI to work correctly, so while it's still possible to not include it, it would most likely make it impossible to select the block without using the List View.

In theory, a hooked block can be inserted as a first or last child into a block that doesn't have any inner blocks at all. This probably doesn't make any sense (and maybe we should forbid it; though that might not be straight-forward); in any case, I think our logic would work even then (and at least not crash 😬 ).

I added unit tests that document this case when there is also a block wrapper. The way it currently works isn't perfect, but it definitely won't crash. We can discuss later what to do about it.

Furthermore, I wonder if there's ever going to be more than one consecutive non-null array element in innerContent (since any "plain" HTML is probably just represented as a string), so maybe we wouldn't even need the loop. But it also doesn't hurt to have it 😬

It's possible, see the example for the Columns block in the block fixtures folder from integration tests:

"innerContent": [
"\n\t<div class=\"wp-block-column\">\n\t\t",
null,
"\n\t\t",
null,
"\n\t</div>\n\t"
]

@gziolo gziolo enabled auto-merge (squash) September 14, 2023 08:03
@gziolo gziolo merged commit cd7370d into trunk Sep 14, 2023
51 checks passed
@gziolo gziolo deleted the fix/incorrect-placement-hooked-blocks branch September 14, 2023 08:19
@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Sep 14, 2023
@ockham
Copy link
Contributor

ockham commented Sep 14, 2023

Furthermore, I wonder if there's ever going to be more than one consecutive non-null array element in innerContent (since any "plain" HTML is probably just represented as a string), so maybe we wouldn't even need the loop. But it also doesn't hurt to have it 😬

It's possible, see the example for the Columns block in the block fixtures folder from integration tests:

"innerContent": [
"\n\t<div class=\"wp-block-column\">\n\t\t",
null,
"\n\t\t",
null,
"\n\t</div>\n\t"
]

Sorry, I don't see it 😅 In your example, non-null and null elements are alternating; but what I meant with consecutive non-null elements is that there are two (or more) non-null elements with no nulls in betweeen them 🤔 I.e. something like

 "innerContent": [ 
 	"\n\t<div class=\"wp-block-column\">\n\t\t", 
 	"\n\t\t", 
 	null, 
 	"\n\t</div>\n\t" 
 ] 

@gziolo
Copy link
Member Author

gziolo commented Sep 14, 2023

Sorry, I don't see it 😅 In your example, non-null and null elements are alternating; but what I meant with consecutive non-null elements is that there are two (or more) non-null elements with no nulls in betweeen them 🤔

Interesting observation. I wasn't aware it groups HTML into one entry before and one after the child block. Anyway, the loop will run twice in most of the cases when there is a block wrapper, so it's probably fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Hooks: Incorrect placement in the parent container for hooked blocks
2 participants