WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.46k stars 4.18k forks source link

Patterns containing some dynamic blocks are limited in the pattern inserter panel #37072

Closed Andrew-Starr closed 3 months ago

Andrew-Starr commented 2 years ago

Description

With patterns in the same category that each contain the same dynamic* block, the available patterns seems to be limited to three patterns in the inserter side panel, and four in the Explore overlay.

*dynamic blocks such as Navigation or Query Loop.

Step-by-step reproduction instructions

Create five patterns each containing the Navigation block. The patterns should be categorized in the same pattern category. In the page/post editor, open the pattern inserter and notice only the first three patterns are displayed and available. Open the Explore overlay and there will only be the first four patterns available.

Additionally, whilst working on the page/post with the pattern side panel still open, the missing patterns sometimes suddenly appear after some time (maybe a few seconds, maybe a couple of minutes).

Screenshots, screen recording, code snippet

No response

Environment info

WP 5.8.2 Gutenberg 12.0.1 Windows 10 Firefox

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

Andrew-Starr commented 2 years ago

This issue also prevents the close button/toggle from being able to close the block inserter side panel.

I've been testing in different environments, and on shared hosting sometimes only the first two patterns are displayed. This is with only a couple of pages so the number of navigation items returned is quite minimal. Also with only one published post, so the query loop block has the very least number of posts to return, and the issue is still present.

I expect many themes will bundle a variety of patterns such as headers to give users a choice of header layouts. These will likely each contain a navigation block, resulting in some of the patterns being unavailable for selection by the user.

A solution I have found is to give each pattern its own category, but then we end up with as many categories as there are patterns, so the pattern discovery flow is rendered sub-par.

Andrew-Starr commented 2 years ago

Using the should_load_remote_block_patterns to remove the remote .org patterns speeds things up quite a bit and makes a difference.

I'd like to be able to keep the remote patterns though.

If anyone with in depth knowledge of the pattern inserter has any ideas I'd love to hear them :)

annezazu commented 2 years ago

@ntsekouras any chance you can share any insights here? :)

ntsekouras commented 2 years ago

Hey @Andrew-Starr, thanks for reporting this. I can reproduce the problem with Query Loop patterns and TwentyTwentyTwo.

In general patterns are really hard to handle in terms of performance as they can contain many many blocks and we not only have to parse and render them, but also do some recursive checks about the allowed blocks settings.

More things should be done there and there are already some optimisations in place like pre-parsing the patterns on load (browsers idle time) and lazy loading the patterns in the inserter/explorer. From my first quick look there seems to be an issue with the lazy loading of patters because patterns are already in state.

--edit With tt1-blocks the Query patterns are all shown. This might be affected by the total number of patterns, as TwentyTwentyTwo registered a lot more patterns..

youknowriad commented 2 years ago

I haven't been able to reproduce this personally, I've tried adding 8 similar patterns to the same category and using 2022 theme and everything is shown properly (you do notice the patterns appearing one by one but that's fine). Can you share the list of patterns you're adding as an example here?

ntsekouras commented 2 years ago

I can reproduce the problem with Query Loop patterns and TwentyTwentyTwo.

@youknowriad does this work well for you? I actually didn't try custom dynamic patterns, but seems to be the same problem.

youknowriad commented 2 years ago

@ntsekouras yes, I basically used this pattern 8 times and it worked.

register_block_pattern(
    'query/test-x',
    array(
        'title'      => __( 'Query Test X', 'gutenberg' ),
        'blockTypes' => array( 'core/query' ),
        'content'    => '<!-- wp:query {"query":{"perPage":1,"pages":0,"offset":0,"postType":"post","categoryIds":[],"tagIds":[],"order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true}} -->
                        <!-- wp:post-template -->
                        <!-- wp:post-title {"isLink":true} /-->
                        <!-- /wp:post-template -->
                        <!-- /wp:query -->',
                        'categories' => array( 'query' ),
    )
);
ntsekouras commented 2 years ago

{"query":{"perPage":1.....

TwentyTwentyTwo Query patterns have perPage big numbers (12, etc..). Can you activate this theme and just open the Query category in Inserter/Patterns explorer?

I think it's related with the time to render the previews (more posts for example more time to render) and might have some memory problem there? 🤔

youknowriad commented 2 years ago

Depending on the number of posts I have on the database, I do see a waterfall effect when rendering the patterns but it always renders everything after 3/4 seconds. (Maybe I have a very computer though)

ntsekouras commented 2 years ago

Yes, eventually if you leave it for some time it will load (in my case is minutes).

I do see a waterfall effect when rendering the patterns

I think this is what we need to find.

youknowriad commented 2 years ago

I think this is what we need to find.

This is intended. useAsyncList renders the list one by one and only start rendering the next one if the browser is "idle" (see requestIdleCallback). The problem here is that the browser doesn't seem to be idle, meaning if we remove useAsyncList, everything will freeze. I think we should potentially show empty placeholders for patterns that are yet to be rendered instead of not rendering them at all like now.

ntsekouras commented 2 years ago

This is intended. useAsyncList renders the list one by one and ...

I know that. I meant if there is something like a memory leak in the flow.

youknowriad commented 2 years ago

This could potentially be a symptom of another performance problem indeed but I doubt it's memory related. I think what could happen here is that maybe these "query blocks" re-render too much (or constantly) which means the browser is never idle (or rarely).

youknowriad commented 2 years ago

I tried debugging a bit more here and unfortunately I didn't see any obvious query performance issues.

ntsekouras commented 2 years ago

I tried debugging a bit more here and unfortunately I didn't see any obvious query performance issues.

Thanks Riad! I have this in my queue to take a shot as well soon.

youknowriad commented 3 months ago

Seems like there's nothing actually actionable here. So closing this issue.