Layout of post-template is broken on the editor when query block displayLayout is type flex.

[iidastudio]

Description

On Gutenberg 12.2.0, Layout of post-template is broken on the editor when query block displayLayout is type flex. When one list in the post-template is selected, the list (.block-editor-block-previewlive-content) becomes display:none, but it exists in the code. (At the same time, a .block-editor-block-listlayout is created.) So the unintended list becomes :nth-child(3n) and margin-right:0 is applied.

It seems to be difficult to fix in CSS, but what do you think?

Step-by-step reproduction instructions

1. install Gutenberg 12.2.0
2. change the theme to twenty twenty two
3. open home in editor
4. click on the post-template list

Screenshots, screen recording, code snippet

https://user-images.githubusercontent.com/43173384/147856056-100c877b-39f3-47c8-8343-3dabb25e3cc8.mp4

Environment info

WordPress 5.8.2 Gutenberg 12.2.0 TwentyTwentyTwo Theme Chrome (latest versions)

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[annezazu]

Ah ha. I saw this behavior too when I reported this issue: https://github.com/WordPress/gutenberg/issues/37645

Something funky seems to have changed with the Query loop block recently. cc @ntsekouras

[ockham]

This seems relevant (introduced by #36431).

I don't have a good grasp yet of what's going on, but maybe @andrewserong or @ramonjd can help enlighten? 🙏

[mcsf]

I don't have a good grasp yet of what's going on, but maybe andrewserong or ramonjd can help enlighten? 🙏

Based on the comment here, it seems to be both a fix for render flickering and a possible optimisation. However, by toggling display on rendered DOM elements we are breaking out of React to do something that React is meant to be doing (i.e. orchestrating the rendering of element trees), thereby setting the stage for conflicts between one system (React and Gutenberg's cascade) and the one underneath it (DOM).

What I would try to do is preserve the overall idea of cached elements by way of MemoizedPostTemplateBlockPreview, but without the display trick, i.e. if ( isHidden ) return null;.

[ramonjd]

I don't have a good grasp yet of what's going on, but maybe @andrewserong or @ramonjd can help enlighten? 🙏

👋 A lot of the good stuff is stored in @andrewserong's brain, which is still enjoying a well-deserved break. The above approach sounds very promising. Nevertheless, I'll flag it as something for us to keep an eye on. Thank you!

[ramonjd]

What I would try to do is preserve the overall idea of cached elements by way of MemoizedPostTemplateBlockPreview, but without the display trick, i.e. if ( isHidden ) return null;.

For fun, I tried returning null when isHidden === true instead of setting the style. The flicker returns but here's the difference I'm seeing:

Return null	Setting style.display

Also, I'm not yet sure this is relevant, but I noticed when activeContextId isn't defined, we assume equality with the first item's postId in the blockContexts collection in the isHidden check.

blockContext.postId === ( activeBlockContextId || blockContexts[ 0 ]?.postId );

But in a grid layout, clicking on the second or third item will fail.

I was wondering if it should be

blockContext.postId === ( activeBlockContextId || blockContexts[ index ]?.postId )

It removes the flicker effect, but also multiselects similar blocks

Also probably a question for @andrewserong when he gets back. I don't have the full context yet.

🍺

[iidastudio]

@annezazu Thank you I see that 5.9 Beta 4. has the same specification.

@ntsekouras Thank you I thought it was impossible with CSS, but it looks like gap can solve the problem! The following is the my theme, but resetting the margin and specifying gap worked well.

https://user-images.githubusercontent.com/43173384/148179123-166a2ed7-f119-4c10-a896-d057749bd4c3.mov

[andrewserong]

Thanks for the discussion, folks (and for the fix, Nik!)

However, by toggling display on rendered DOM elements we are breaking out of React to do something that React is meant to be doing (i.e. orchestrating the rendering of element trees), thereby setting the stage for conflicts between one system (React and Gutenberg's cascade) and the one underneath it (DOM).

Good point, I initially tried returning null. The optimisation of keeping the component mounted but styled to be hidden definitely feels hacky, however in practice the mounting / unmounting of the block preview appears to be expensive enough to cause a flicker, and using display to manage visibility was the best way I could find to avoid the flicker entirely when clicking between instances of the query loop. In terms of potential conflicts, the issue is slightly mitigated in that we're using styling to hide the unneeded element rather than manipulating the DOM directly, but it is still two sets of markup for one thing that is visible to a user.

So, it is a bit of awkward code, but 🤞 the performance benefit for interacting within the block outweighs the workaround. I'm very happy for us to change the implementation of course, if anyone comes up with a way to do it with a cheaper mount / unmount!

[andrewserong]

But in a grid layout, clicking on the second or third item will fail.

@ramonjd is that still an issue that we need to look into? From memory the fallback to blockContexts[ 0 ]?.postId in ( activeBlockContextId || blockContexts[ 0 ]?.postId ) is so that if a user hasn't yet interacted with the instances of the query loop block, then we assume that focus should be within the first element of the loop.

[ramonjd]

@ramonjd is that still an issue that we need to look into? From memory the fallback to blockContexts[ 0 ]?.postId in ( activeBlockContextId || blockContexts[ 0 ]?.postId ) is so that if a user hasn't yet interacted with the instances of the query loop block, then we assume that focus should be within the first element of the loop.

I wasn't really sure and had only a shallow reading of the code. So, no: if you think it's good, then it's good 👍 Thanks for clarifying!