Show top level sections in List View #65202
Conversation
getdave
added
[Type] Experimental
| @@ -114,6 +114,7 @@ export const getEnabledClientIdsTree = createSelector( | |||
| state.blockEditingModes, | |||
| state.settings.templateLock, | |||
| state.blockListSettings, | |||
| state.editorMode, | |||
There was a problem hiding this comment.
List View depends on this selector to determine which clientIds to display. As it's memoized, when we switch editor modes we will need to recompute the value to ensure it's accurate.
There was a problem hiding this comment.
I'd really like a confidence check on this one. I would have thought that having the state.blockEditingModes dependencies would be enough here but apparently not 😕
There was a problem hiding this comment.
@talldan This turns out to still be required otherwise List View doesn't not reflect the changes correctly. Going to have to continue digging to work out why though...
There was a problem hiding this comment.
To define the dependencies of a selector, we need to just look what selectors it uses and what state it users So we need to look at. getEnabledClientIdsTreeUnmemoized. So basically we need this to be the sum of getBlockEditingMode's dependencies and getBlockOrder's dependencies.
so state.blocks.order + state.editorMode + state.settings?.[ sectionRootClientIdKey ] + state.settings.templateLock + state.blockEditingModes + state.blocks.parents (same thing as order though, just denormalized) + state.blockListSettings + state.blocks.byClientId + state.blocks.attributes (for native only) + registered blocks.
It's a very hard selector to memoize IMO. We have options:
- Keep as is and risk re-renderings that are not needed that often.
- Remove the need for this selector (probably involves component refactoring)
- Denormalize the selector in a dedicated redux state (like we do for blocks.tree...) it's very hard to get right though and I would only do it if we realize this selector is a real bottleneck performance wise.
There was a problem hiding this comment.
Yeah, this is missing a lot of dependencies, not just state.editorMode. There's a good chance that there's other subtle bugs because of this, but adding this many dependencies pretty much negates the usefulness of the memoization.
For now, I think we should keep things as-is to fix the obvious bug of not updating the list view correctly. The editorMode doesn't get updated that frequently, so I don't think it will have that large of a performance impact.
There was a problem hiding this comment.
To define the dependencies of a selector, we need to just look what selectors it uses
I came to the same conclusion about dependencies here.
It's a very hard selector to memoize IMO
Agreed.
Keep as is and risk re-renderings that are not needed that often.
Remove the need for this selector (probably involves component refactoring)
Denormalize the selector in a dedicated redux state (like we do for blocks.tree...) it's very hard to get right though and I would only do it if we realize this selector is a real bottleneck performance wise.
editorMode doesn't change often and I can't imagine that changing in the future. We're never very unlikely going to change editorMode on an action that runs repeatedly as it would be poor for UX.
On balance I'd say running with the fix in this PR would be an acceptable trade off.
Would it help if I added a comment to the selector to note that it may be missing dependencies and referring to this discussion? That way if it becomes a problem in the future we know where to look.
There was a problem hiding this comment.
I think it's definitely acceptable to run with this PR, my worry is that one of the underlying selectors change and we don't update the dependencies here. It's indirect.
There was a problem hiding this comment.
I agree.
The only viable options seems to be:
Remove the need for this selector (probably involves component refactoring)
It would be cool if you could declare a selector as dependency and then it would automatically use that selector's dependencies as dependencies.
There was a problem hiding this comment.
I think you can kind of do that:
But I'm not sure it'd work here because of the way it depends on blockEditingMode (unmemoized, and also per block), which then depends on editorMode. 🤔
|
Size Change: -11.9 kB (-0.67%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
getdave
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @cat-og. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
getdave
changed the title
| ! isZoomOutSectionBlock && | ||
| hasNestedBlocks && | ||
| shouldShowInnerBlocks | ||
| ? expandedState[ clientId ] ?? isExpanded | ||
| : undefined; |
There was a problem hiding this comment.
Instead of adding custom code to the inner parts of List View to filter out blocks, I think a better alternative might be to pass in a filtered block tree at the top of List View.
This approach is already used for filtering out disabled blocks. Some references:
useListViewClientIds, this previously showed all blocks, but was updated to usegetEnabledClientIdsTreethat instead only shows enabled blocks -getEnabledClientIdsTree, this is the selector -gutenberg/packages/block-editor/src/store/private-selectors.js
Lines 82 to 99 in 07c7821
My thinking is you could add another private selector getZoomedOutClientIdsTree and then switch to using that in useListViewClientIds whenever zoomed out is active.
There was a problem hiding this comment.
Though I'm also not sure why getEnabledClientIdsTree is not enough. Does zoom out disable the inner block via some means that isn't blockEditingMode?
Maybe that's the thing to solve, along the lines of what @youknowriad mentioned here - #65027 (comment) 🤔
There was a problem hiding this comment.
Yeh I agree. Great feedback. I'll take a look at that approach.
There was a problem hiding this comment.
I took a look and worked out that we don't need to do anything as getEnabledClientIdsTree already does what we want. I've updated the PR to reflect this.
The problem is that getEnabledClientIdsTree is memozied and doesn't re-compute when we switch to/from Zoom Out mode unless we add state.editorMode to the dependencies array. This is what we discussed before.
Why is this?
Surely switching to Zoom Out changes the blocks' blockEditingModes and as state.blockEditingModes is listed as a dependency of the getEnabledClientIdsTree createSelector it should recompute when editor mode changes to Zoom Out?
gutenberg/packages/block-editor/src/store/private-selectors.js
Lines 105 to 113 in b96f5a6
Wrong 😓
In fact that assumption turns out to be incorrect. We never actually update the blockEditingModes state (via setBlockEditingMode) when switching to Zoom Out mode.
Instead the getBlockEditingMode selector determines the correct blockEditingMode based on it's own internal logic rather than the state.
gutenberg/packages/block-editor/src/store/selectors.js
Lines 2910 to 2925 in b96f5a6
This means that the state.blockEditingModes does not change which means that the state.blockEditingModes dependency does not cause getEnabledClientIdsTree to recompute. This in turn means the List View does not update to reflect the change.
I believe this is the reason why we also have state.settings.templateLock in the dependencies array as this is also used within the getBlockEditingMode selector:
gutenberg/packages/block-editor/src/store/selectors.js
Lines 2935 to 2943 in b96f5a6
I thought perhaps wrapping getBlockEditingMode in a createSelector and adding state.editorMode to the dependencies would fix the issue. However, I believe this isn't a subscription dependency, it merely determines whether the selector cache should be invalidated the next time it is called. So it doesn't work.
Therefore I think the only logical course for now is to add the state.editorMode to the dependencies list of getEnabledClientIdsTree.
I'd appreciate a confidence check on the above in case my logic is flawed 🙏
Maybe that's the thing to solve, along the lines of what @youknowriad mentioned here - #65027 (comment) 🤔
Yes I think that is highly relevant.
There was a problem hiding this comment.
Therefore I think the only logical course for now is to add the state.editorMode to the dependencies list of getEnabledClientIdsTree.
I think that makes sense. Thanks for digging into the issue!
| const draggedBlockCount = | ||
| draggedClientIds?.length > 0 | ||
| ? getClientIdsOfDescendants( draggedClientIds ).length + 1 | ||
| : 0; | ||
| return { | ||
| visibleBlockCount: getGlobalBlockCount() - draggedBlockCount, | ||
| shouldShowInnerBlocks: __unstableGetEditorMode() !== 'zoom-out', |
There was a problem hiding this comment.
Let's allow inner blocks to show in Zoom Out mode because we need to see the "sections" which are inner blocks of the sectionRoot.
What?
Shows the top level "sections" in the List View when in Zoom Out mode.
Closes #64260
Why?
It's expected that you can see the editable sections in the List View.
How?
true.Refactor list view to only hide children of "Sections".Testing Instructions
Contentblock and it's sub blocks are shown.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-09-10.at.14-44-37.mp4