List View: avoid re-rendering all items on block focus. Enable persistent List View in the widget editor. #35706
Conversation
|
Size Change: +281 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Initial results look promising for the focus case, manually profiling with the performance test post, focus time is back to ~180ms vs ~900ms
|
gwwar
added
[Feature] List View
gwwar
requested review from
ajitbohra,
ellatrix,
nerrad and
ntwb
as code owners
| selectedClientIds, | ||
| showOnlyCurrentHierarchy | ||
| ) => | ||
| const useListViewClientIdsTree = ( blocks, showOnlyCurrentHierarchy ) => |
There was a problem hiding this comment.
What do you think about inlining this code? The hooks seem really simple now (which is great!)
There was a problem hiding this comment.
@talldan do you mean moving the useSelect call to index.js or simplifying code in use-list-view-client-ids.js?
| { hasAppender && ( | ||
| <ListViewAppender | ||
| parentBlockClientId={ parentBlockClientId } | ||
| position={ rowCount } | ||
| rowCount={ appenderPosition } | ||
| level={ level } | ||
| terminatedLevels={ terminatedLevels } | ||
| path={ [ ...path, appenderPosition ] } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
I see it was mentioned in the PR description, was it removed because of performance issues?
The appender isn't mentioned in either of the tracking issues, so probably fine to remove. cc @priethor
There was a problem hiding this comment.
Good call, I don't think adding blocks directly in the list view is a top priority as it opens another whole can of worms, but we should at least have a separate discussion/issue; removing it here should be fine.
There was a problem hiding this comment.
was it removed because of performance issues?
Yes, I was avoiding needing branch.js to query isSelected state. Some of the logic around the appender required isSelected state, and I didn't find any used references.
In general, if we have code that is unused, I'd prefer to remove it since it helps reduce package size and makes it easier to maintain. In the future we can always open a new PR to add the appender back.
Removing it here should be fine.
✨ Excellent!
| showAppender && | ||
| ! isTreeRoot && | ||
| isClientIdSelected( parentClientId, selectedClientIds ); | ||
| const hasAppender = itemHasAppender( parentBlockClientId ); | ||
| // Add +1 to the rowCount to take the block appender into account. |
There was a problem hiding this comment.
The comment could be removed
| // 'is-last-of-selected-branch': | ||
| // withExperimentalPersistentListViewFeatures && | ||
| // isLastOfSelectedBranch, |
There was a problem hiding this comment.
Yes, I'll investigate if we need these classes (and if we do I'll fix logic here).
There was a problem hiding this comment.
We can fully remove this one, the border radius isn't even visible unless we add a background.
CleanShot.2021-10-18.at.14.10.26.mp4
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import ListViewBlock from './block'; | ||
| import ListViewAppender from './appender'; | ||
| import { isClientIdSelected } from './utils'; | ||
| import { useListViewContext } from './context'; | ||
|
|
||
| export default function ListViewBranch( props ) { |
There was a problem hiding this comment.
I like the simplification of this component 🎉
| // Make updates to the selected or dragged blocks synchronous, | ||
| // but asynchronous for any other block. | ||
| const isDragged = !! draggedClientIds?.includes( clientId ); | ||
|
|
||
| return ( | ||
| <AsyncModeProvider key={ clientId } value={ ! isSelected }> |
There was a problem hiding this comment.
I think the root of ListView still has AsyncModeProvider, so I think that would make the whole list async, even the selected block. Not seeing any regressions of #34519 though, so maybe it's ok ... I don't know why though!
There was a problem hiding this comment.
Good catch, I'll try and tidy that. I mostly moved back to sync here, since we didn't have isSelected, and I know there are some visual glitches if we try rendering all items async.
There was a problem hiding this comment.
Moving back to sync adds back some focus slowness. I instead moved the nested AsyncModeProvider call down to ListViewBlock in c344ca1
|
|
||
| return __unstableGetClientIdsTree(); | ||
| }, | ||
| [ blocks, selectedClientIds, showOnlyCurrentHierarchy ] | ||
| [ blocks, showOnlyCurrentHierarchy ] |
There was a problem hiding this comment.
So I double-checked behavior in trunk and it looks like ListView is still being used as a dropdown in the widget editor.
Currently if showOnlyCurrentHierarchy is set, if an item is selected this will render a subset of the tree.
| block selected | with no blocks selected |
|---|---|
 |
 |
Can we use a sidebar instead in widgets editor? It feels like the popover has limited utility and that folks aren't very aware of this use case. (Note the scrollbars in trunk). If folks agree, I can create a separate PR or add on to this one.
There was a problem hiding this comment.
I'd love unifying on a sidebar, if nothing else then just to avoid drift. But I'll defer to @critterverse who may have thoughts on this.
There was a problem hiding this comment.
Tried this out in 1557543
CleanShot.2021-10-18.at.15.30.25.mp4
There was a problem hiding this comment.
Nice! This could close #32311.
There was a problem hiding this comment.
+1 to adding it to the widgets screen!
gwwar
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
gwwar
force-pushed
the
try/list-view-decouple-is-selected
branch
from
819ebba
to
1557543
Compare
|
Thanks for the review @talldan @jasmussen @priethor ! Scope here increased since I also tried displaying a ListView sidebar for edit-widgets. It'd be lovely to know if folks agree on the Widget ListView sidebar change, or if we should keep the dropdown. Note that the sidebar change should also be easy to move that to a different PR. I'm aware that I also need to fix some E2Es from the widget change (I ran out of time today). |
I am all for removing the dropdown and replacing it with a sidebar! And apparent;ly so is everyone here #35706 (comment) 🎉 |
gwwar
force-pushed
the
try/list-view-decouple-is-selected
branch
from
1557543
to
b453e31
Compare
|
Edit: since I wasn't sure I didn't include these changes. |
|
I have a small PR going that replaces I rebased that PR on top of these changes and took it for a spin to check if anything stood out. I couldn't spot any obvious regressions on desktop browsers. In the Widget and Block Editors:
all worked as expected. I also tested on iOS Safari.
Also +1 for adding the sidebar. Thank you!! |
|
Just as a quick test, this is working well for me, seeing no regressions. I did also try and stress it with drag/dropping in a list view with 900 blocks, and it completely choked, just like trunk, because this is not your windowing PR. So landing both of those would be just great, and if anyone reading this can help review and land them, we'll be in a vastly better place! 👌 |
…w Block to avoid re-rendering all items on block focus
gwwar
force-pushed
the
try/list-view-decouple-is-selected
branch
from
4d131a3
to
28f60b5
Compare
|
@priethor was kind enough to look over the changelog notes, so this one is all set ✨ Thanks for the reviews folks! |
|
🔥 |
gwwar
changed the title
Proposed changes in this PR move the querying of isSelected to the ListViewBlock to avoid slowing down the editor on block focus.
The key thing to highlight is block focus on this branch is 221.22 ms vs 930.92 ms on trunk with List View open. Changes here don't affect first render time which still sits in the 3-5 second range to open List View with the performance test post. Using a form of windowing should cut first render down to a more respectable
200mssee #35230Functionality Changes
The widget editor was the only consumer of the ListView dropdown (BlockNavigationDropdown). This required some isSelected state to render a subset of items. From the scrollbars in the popover seen in
trunkthis isn't a well known usage, so I'm proposing that we use a ListView sidebar instead for consistency. If this change is too large, I can pull out this change to a separate PR.Testing Instructions
Full Performance Results
Edit: note that performance suite changes were removed in a rebase to avoid slowing down performance tests in trunk before landing. (What I measured with can be seen at c6b08e5)
To give us solid numbers on the proposed changes, I've also included changes in the performance tests to have list view open during
focusandtypingin addition to a new test that toggles the list view open and closed.Problem
While experimenting with the windowing technique in #35230 we discovered some unexpected behavior with block focus time.
Focusing on a block in the main editor pane will re-render all ListViewBlock items. Depending on if list view is open or not, we can see some dramatic differences in how long it takes to resolve a focus event.
If we profile using react devtools, here we can spot some slow renders on each block focus. Long story short, we query for selectedIds on the parent ListView component. In React if a parent updates all child components by default will update regardless of prop changes (which may or may result in DOM updates). So on focus, all items updated.
What we changed here was moving the selectedId querying to the child ListViewBlock component, which as we can see from the performance results above cleared up the slow render.
TODOs
useListViewClientIdsWe never pass in selectedClientIds, so perhaps we can remove logic there.Do we still need the appender code in List View? I didn't find any active referencesAppender code is okay to removeVerify any differences between async/sync rendering. I ended up removing it in this PR since we no longer had selectedId in the branch to mark items to explicitly render syncMoved the nested async call to ListViewBlockCan we delete BlockNavigationDropdown, or is this needed for backwards compatibility?I opted to not remove the component in this PRFolks are welcome to hop into the branch directly if you're aware of any needed functionality changes.