Inner blocks: Reduce tree depth to improve performance. #50447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcalhoun
added
[Feature] Blocks
|
Size Change: +1.17 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
dcalhoun
commented
May 8, 2023
There was a problem hiding this comment.
👋🏻 @geriux. I published this draft to share the initial approach. This draft is very raw. I combined the components without much deduplication with a focus on getting a working version, with the plan to follow up to make it "right," then make it fast. It's easiest to review with "Hide whitespace" enabled.
Also, I wanted to ask a question regarding the scroll view refs. Please see that in the inline comments.
packages/block-editor/src/components/block-list/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/index.native.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 81e8d51. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4931745624
|
|
@geriux also, if there aspects of wordpress-mobile/gutenberg-mobile#5617 or wordpress-mobile/gutenberg-mobile#5618 to which you would like to begin contributing towards, feel free to do so and push to this branch as necessary. I believe we could resolve any merge conflicts. |
I can take wordpress-mobile/gutenberg-mobile#5618 😎 |
The test helper executes asynchronous updates to the layout. If this is not awaited, test failures can occur in certain circumstances.
Reduce code duplication between the separate components, e.g. item render, footer, end-of-list block appender button, empty list placeholder.
Partially fix multi-column layouts by passing existing styles to a wrapping view. All of the passed styles may not be necessary, need to investigate. Also, Buttons render broken, wrapping lines and overflowing the parent container.
Nested lists now rely upon `BlockListItem`, which returns `null` if the `blockWidth` has not yet been set. In order for test queries for nested list items to succeed, we must trigger the `onLayout` callback for the nested block lists. `BlockListItem` is now used to expand capabilities for nested blocks, e.g. multi-column grid items.
Fixes Buttons, but Columns remain broken.
The list is no longer shared, so we can merely set the appropriate styles on each list element.
Now that inner blocks do not use a virtualized list, a unique list key is no longer necessary.
The scroll ref is no longer passed to inner blocks as they do not use virtualized lists.
The relocated styles may need to be separate from the top-level block list element, as the styles may conflict with other styles. It does not fix Columns, however.
This caused issues for non-Buttons inner block alignment. The original issue of Buttons inner blocks not rendering inline was addressed in 43c0b14918f8ed03037c01d94321922dc31a7fa3.
These styles caused columns to align to the center when their width sum did not equal or execeed 100%. Removing these styles did not appear to negatively impact other inner blocks or the use cases outlined in #25621.
This mirrors the approach for cells of top-level block lists. It is also likely more robust, providing better performance when reordering blocks.
The inner blocks list now sets a key on a wrapping view, rather than via the `renderItem` function. Thus, this key is no superfluous.
dcalhoun
force-pushed
the
perf/reduce-inner-blocks-tree-depth
branch
from
2cb3c6e to
445a5b1
Compare
The parent flex styles were applied to the footer element. This wrapping view prevents those styles from erroneously stretching the provided footer element.
dcalhoun
commented
May 10, 2023
| @@ -74,7 +66,6 @@ export default function BlockList( { | |||
| renderAppender, | |||
| renderFooterAppender, | |||
| rootClientId, | |||
| title, | |||
There was a problem hiding this comment.
From what I could find, this prop is unused. The header prop is utilized for displaying the post title in the root block list.
Comment on lines
-252
to
-254
| listKey={ | ||
| rootClientId ? `list-${ rootClientId }` : 'list-root' | ||
| } |
There was a problem hiding this comment.
This is no longer necessary now that only the root list is virtualized.
| @@ -52,7 +52,7 @@ describe( 'BlockList', () => { | |||
| await addBlock( screen, 'Social Icons' ); | |||
| const socialLinksBlock = await getBlock( screen, 'Social Icons' ); | |||
| fireEvent.press( socialLinksBlock ); | |||
| triggerBlockListLayout( socialLinksBlock ); | |||
| await triggerBlockListLayout( socialLinksBlock ); | |||
There was a problem hiding this comment.
We should always await the asynchronous triggerBlockListLayout helper. The lack of await was erroneous and can cause act warnings and test failures in certain circumstances.
| @@ -70,6 +70,7 @@ describe( 'List block', () => { | |||
| // Select List block | |||
| const [ listBlock ] = screen.getAllByLabelText( /List Block\. Row 1/ ); | |||
| fireEvent.press( listBlock ); | |||
| await triggerBlockListLayout( listBlock ); | |||
There was a problem hiding this comment.
Now that inner blocks rely upon BlockListItem, the nested list values are not rendered until the onLayout handler is triggered to set the blockWidth.
Comment on lines
+169
to
+173
| // Await recently indented list item layout | ||
| const [ listItemBlock1 ] = screen.getAllByLabelText( | ||
| /List item Block\. Row 1/ | ||
| ); | ||
| await triggerBlockListLayout( listItemBlock1 ); |
There was a problem hiding this comment.
Resolves an act warning in the test logs.
… option to render the appender for InnerBlocks
|
I'm currently focusing on testing, so far on Android I see no regressions or performance issues with these new changes. I tested on both a Redmi Note 8T (Android 11) and a Samsung A23 5G (Android 13) ✅ |
|
I finished testing on an iPhone 14 Pro (OS 16.4.1) and on an iPad (iPad OS 16.1), on both portrait and landscape including the mentioned column PRs testing cases and its working as expected ✅ |
geriux
reviewed
May 11, 2023
There was a problem hiding this comment.
Code changes look good to me! I think the only thing pending is to see why CI is failing over Gutenberg mobile.
This Xpath selector became outdated with the block list refactor.
Appium reported this cached element had been removed from the DOM. Relocating the query seemingly resolved this issue.
geriux
approved these changes
May 12, 2023
This was referenced May 17, 2023
cbravobernal
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related PRs:
What?
Reduce the rendering complexity and depth of the block list for inner blocks.
Why?
Closes wordpress-mobile/gutenberg-mobile#5617.
Reducing rendering complexity is likely to improve performance overall. Also,
React Native has shown it has a limit in regards to number of nested views it
can render. Rendering too deep of trees can result in crashes; we observed this
when List v2 began using inner blocks.
How?
BlockListandBlockListCompactto reduce code duplication betweenthe separate components, e.g. item render, footer, end-of-list block appender
button, empty list placeholder.
through the addition of features like the nested block appender.
The following diagram visually showcases the new structure of the block list used for
both the root and inner blocks.
Testing Instructions
We should test adding, editing, re-ordering, and removing various block types,
including nested blocks. Additionally, we should test various device
orientations and sizes.
Specifically, we should heavily test various organizations of inner blocks with
different alignments applied to the parent and children blocks. The testing
instructions for a couple of Columns block PRs are a good start: #19013,
#25621.
Testing Instructions for Keyboard
n/a
Screenshots or screencast
n/a