List View: Fix performance issue when selecting all blocks #54900
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
andrewserong
added
[Type] Bug
25 tasks
|
Size Change: -6 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
apeatling
reviewed
Sep 28, 2023
|
As an aside not for this PR, it is not easy to select all blocks using only the list view. I would expect to be able to select a block in the list view and use Cmd+A to select all the blocks in the list view. I also expected a "Select All" in the dropdown menu. |
|
Selecting is at least 5x faster than trunk! 👍🏻 Here's what I'm seeing when scrolling after selection: 2023-09-29.09.52.46.mp4So while I have the impression I can select multiple blocks more quickly, if I want to browse the selected blocks, the lag seems to be deferred. I have no idea if this is possible, but is there a way to batch selection? So, show block that pass |
|
@apeatling, there's a separate PR for "Select all" in the list view. See #54899. |
|
Thanks for testing, everyone!
Yes, I split out this PR to see if we can land a performance improvement for 6.4, leaving the feature enhancement for 6.5. Assuming we can get this PR working adequately, of course 🙂
Oh, great catch, I didn't notice the windowing slowing down so much like that. Given that selecting all blocks is a little hidden as an action, I think it'd be worth digging into the performance a little more here rather than unintentionally expose a performance issue that could be more prominent than what's currently on trunk 🤔
Great question, I have no idea either! 😄 Thanks folks, all that feedback gives me more avenues to explore! |
|
Oh, this has been a fun one to investigate! Thanks @ramonjd, your observation about the slowdown when scrolling the list view highlighted the real culprit, which is that I ran out of time to put up a good fix today, but I'll continue digging in tomorrow... I'll be keen to see if we can hopefully tidy up the focusing behaviour. Overall, though, if we use the fix in this PR (don't render selected blocks we don't need to), coupled with avoiding calling |
… editor canvas in long posts
…ew, instead of within each block
andrewserong
force-pushed
the
fix/performance-in-list-view-when-selecting-all-blocks
branch
from
4f388e9 to
d4f40db
Compare
|
Update: I think I've come up with a reasonable fix in d4f40db. Broadly, it does the following:
All up, I think this improves the performance with selected blocks overall, and in manual testing it appears to be better in the following cases:
This will likely need some more manual testing (especially across browsers) to make sure it hasn't introduced any regressions, but all up, I think we've honed in on a good performance improvement now. As always, happy for any feedback, especially if there are concerns with the change! I'll ping a couple more folks as reviewers, just for visibility 🙂 |
|
Flaky tests detected in d4f40db. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6388478901
|
mikachan
added
the
Backport to WP 6.7 Beta/RC
apeatling
approved these changes
Oct 3, 2023
|
Great find @andrewserong ! I've only tested in Chrome so far, but just from profiling I can tell that the focus() isn't being constantly called when rendering the hidden items: BeforeThere's a constant and solid call to focus 
AfterThe "dip" is the point where the hidden items are being rendered on scroll. 
Screencast
|
kevin940726
approved these changes
Oct 4, 2023
|
Thanks for the reviews, everyone! 🙇
No, no, I don't think it was you! I think we both merged PRs that might have exposed the bug a little bit, but it turns out the underlying issue dates right back to the introduction of the persistent list view in #28637 as the logic there (at the block level, instead of at the root of the component) could result in many simultaneous (or close to it) calls to Hopefully this neatens things up a little bit! I'll merge it in now, happy to look into any follow-ups if we run into any further issues. |
andrewserong
deleted the
fix/performance-in-list-view-when-selecting-all-blocks
branch
mikachan
pushed a commit
that referenced
this pull request
Oct 4, 2023
* List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block
|
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: fba6504 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
karmatosed
pushed a commit
that referenced
this pull request
Oct 9, 2023
* Add clearer labels and context to BlockPatternsSyncFilter (#54838) * Add help text & clearer labeling * Theme & Plugins * Font Library: use snake_case instead of camelCase on fontFamilies endpoint param (#54977) * use snake_case instead of camelCase on endpoint param * updating test * Fix output of Navigation block classnames in the editor. (#54992) * Block Editor: Avoid double-wrapping selectors when transforming the styles (#54981) * Block Editor: Avoid double-wrapping selectors when transforming the styles * Include space in the check * Don't display the navigation section in template parts details when a menu is missing (#54993) * Scripts: Properly use CommonJS for default Playwright config (#54988) * Fix path to `globalSetup` in default Playwright config Oversight from #54856 * `module.exports` * Fix default export usage * Add template replace flow to template inspector (#54609) * Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <[email protected]> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <[email protected]> --------- Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> * List View: Fix performance issue when selecting all blocks (#54900) * List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block * Fix left and right aligmnent in children of Post Template (#54997) * Fix left and right aligmnent in children of Post Template * Add align center styles * Fix image placeholder disappearing * Site Editor: Avoid stale navigation block values when parsing entity record (#54996) * Removed unwanted space from the string (#54654) * Update styles.js Removed unwanted space with translation * Update deleted-navigation-warning.js Unwanted space at the end of the string shows translation warning. * Update inspector-controls.js Unwanted space at the end of the string shows translation warning * Fix Deleted Navigation Menu warning string (#55033) * [Inserter]: Fix reset of registered media categories (#55012) * [Inserter]: Fix reset of registered media categories * convert `useInserterMediaCategories` to selector and make private * Try fixing the flaky 'Toolbar roving tabindex' e2e test (#54785) * Try fixing the flaky 'Toolbar roving tabindex' e2e test * Add a link in the comment * Fallback to Twitter provider when embedding X URLs (#54876) * Fallback to Twitter provider when embedding X URLs * Avoid processing empty urls when trying a different provider * Update `react-native-editor` changelog # Conflicts: # packages/react-native-editor/CHANGELOG.md * Based on the efforts in #51761, remove caps case from Template Part and prefer sentence case. As all instances of this string are stand alone, it's okay to have Template capitalized as it's the start of a sentence. (#54709) * Update pattern import menu item (#54782) * Update pattern import menuitem * Revert label * Image Block: Fix browser console error when clicking "Expand on Click" (#54938) * Patterns: Remove category description in inserter panel? (#54894) * Media & Text: Fix React warning (#55038) * Block Style: Display default style labels correctly in the block sidebar (#55008) * Site Editor: Do not display 'trashed' navigation menus in Sidebar (#55072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <[email protected]> --------- Co-authored-by: Aaron Robertshaw <[email protected]> * Image: Fix Lightbox display bug in Classic Themes. (#54837) * If current theme is not a block theme add a default value for $background_color and $close_button_color. * Set lightbox buttons' background & border to none on hover & focus * Change logic to support lightbox in classic themes * Update logic to avoid unnecessary calls * Add style fixes * Remove unnecessary code * Fix close button color --------- Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Ricardo Artemio Morales <[email protected]> * Latest Posts: add screen reader title text to Read more links and use alternative to excerpt_more filter (#55029) * In the editor: adds a screen reader text span with the post title in the i18n interpolator In the frontend: removes excerpt_more filter so we don't override themes and also replaces the default ellipsis with an accessible read more link * Removing "of" preposition in favour of a semi-colon. "Read more" is already translated so using a specifier to add it to the string * Fix Image block lightbox missing alt attribute and improve accessibility (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <[email protected]> # Conflicts: # packages/block-library/CHANGELOG.md * Patterns: Add category selector to pattern creation modal (#55024) --------- Co-authored-by: Kai Hao <[email protected]> * Iframe: Fix positioning when dragging over an iframe (#55150) * Site Editor: Fix template part area listing when a template has no edits (#55115) * Alternative: Fix template part area listing when a template has no edits * Fix typos --------- Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Matias Benedetto <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Pascal Birchler <[email protected]> Co-authored-by: Ben Dwyer <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Andrew Serong <[email protected]> Co-authored-by: mujuonly <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: Nik Tsekouras <[email protected]> Co-authored-by: Carlos Garcia <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Michal <[email protected]> Co-authored-by: Mario Santos <[email protected]> Co-authored-by: Ricardo Artemio Morales <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Kai Hao <[email protected]>
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.
What?
Part of: #49563
Fix a performance issue with the List View when selecting all blocks within the editor canvas, with the list view open. For long posts or pages, the current performance issue means it will likely feel broken as it can take many seconds to apply the selection.
Why?
There are two related performance issues on
trunkthat affect this behaviour:ListViewBlock) is currently responsible for directing focus to it when the list view is mounted. Because it occurs at the block item level, it is possible for dozens of calls to competingcellRef.current.focus()to occur at once, causing the editor to slow down until they are all complete.The result of these two issues interacting is that when you select all blocks with the list view open, (or go to open the list view after selecting all blocks), the list view slows to a crawl until all the
.focus()calls have completed.How?
utils(the logic was introduced back in [ListView] Allow deleting blocks using keyboard #50422).Testing Instructions
trunk, it'll take a long time to select all blocks in a long post or pageAssuming all this is working okay for this PR, we then need to make sure that we haven't broken the scroll into view logic when single block selection changes (introduced in #46895). Follow the testing instructions from that PR:
Finally, double check that focus still appears to be correct when deleting blocks via the list view (with an item in the list view focused, press the Backspace key).
Screenshots or screencast
Note: the List View must be open to test out this performance issue.
The following screengrabs are from a post containing 203 blocks. The first press of CMD+A works quickly to select the individual block. It's the second press of CMD+A to select all blocks that's the problem here!
Before (takes 8 seconds or so to select all)
2023-09-28.15.58.21.mp4
After (takes < 1 second to select all)
2023-09-28.16.02.43.mp4