Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show top level sections in List View #65202

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Show top level sections in List View #65202

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90c7702
Show top level sections in List View
getdave Sep 10, 2024
98ba5ff
Recompute enabled clientIds when editorMode changes
getdave Sep 10, 2024
3fb66f4
Revert initial changes
getdave Sep 11, 2024
45e98a0
Avoid special conditional for Zoom Out mode
getdave Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/list-view/branch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ function ListViewBranch( props ) {
blockIndexes,
expandedState,
draggedClientIds,
sectionBlockClientIds,
isZoomOutMode,
} = useListViewContext();

if ( ! canParentExpand ) {
Expand Down Expand Up @@ -174,8 +176,14 @@ function ListViewBranch( props ) {
: `${ position }`;
const hasNestedBlocks = !! innerBlocks?.length;

const isZoomOutSectionBlock =
isZoomOutMode &&
sectionBlockClientIds?.includes( clientId );

const shouldExpand =
hasNestedBlocks && shouldShowInnerBlocks
! isZoomOutSectionBlock &&
hasNestedBlocks &&
shouldShowInnerBlocks
? expandedState[ clientId ] ?? isExpanded
: undefined;
Copy link
Contributor

@talldan talldan Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 use getEnabledClientIdsTree that instead only shows enabled blocks -

    gutenberg/packages/block-editor/src/components/list-view/use-list-view-client-ids.js

    Lines 25 to 26 in 07c7821

    clientIdsTree:
    blocks ?? getEnabledClientIdsTree( rootClientId ),
  • getEnabledClientIdsTree, this is the selector -

    gutenberg/packages/block-editor/src/store/private-selectors.js

    Lines 82 to 99 in 07c7821

    function getEnabledClientIdsTreeUnmemoized( state, rootClientId ) {
    const blockOrder = getBlockOrder( state, rootClientId );
    const result = [];
    for ( const clientId of blockOrder ) {
    const innerBlocks = getEnabledClientIdsTreeUnmemoized(
    state,
    clientId
    );
    if ( getBlockEditingMode( state, clientId ) !== 'disabled' ) {
    result.push( { clientId, innerBlocks } );
    } else {
    result.push( ...innerBlocks );
    }
    }
    return result;
    }

My thinking is you could add another private selector getZoomedOutClientIdsTree and then switch to using that in useListViewClientIds whenever zoomed out is active.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh I agree. Great feedback. I'll take a look at that approach.

Copy link
Contributor Author

@getdave getdave Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export const getEnabledClientIdsTree = createSelector(
getEnabledClientIdsTreeUnmemoized,
( state ) => [
state.blocks.order,
state.blockEditingModes,
state.settings.templateLock,
state.blockListSettings,
]
);

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

if ( editorMode === 'zoom-out' ) {
const { sectionRootClientId } = unlock( getSettings( state ) );
if ( clientId === '' /* ROOT_CONTAINER_CLIENT_ID */ ) {
return sectionRootClientId ? 'disabled' : 'contentOnly';
}
if ( clientId === sectionRootClientId ) {
return 'contentOnly';
}
const sectionsClientIds = getBlockOrder(
state,
sectionRootClientId
);
if ( ! sectionsClientIds?.includes( clientId ) ) {
return 'disabled';
}
}

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

const templateLock = getTemplateLock( state, rootClientId );
if ( templateLock === 'contentOnly' ) {
const name = getBlockName( state, clientId );
const isContent =
select( blocksStore ).__experimentalHasContentRoleAttribute(
name
);
return isContent ? 'contentOnly' : 'disabled';
}

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


Expand Down
55 changes: 36 additions & 19 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ import { BlockSettingsDropdown } from '../block-settings-menu/block-settings-dro
import { focusListItem } from './utils';
import useClipboardHandler from './use-clipboard-handler';

import { unlock } from '../../lock-unlock';

const expanded = ( state, action ) => {
if ( action.type === 'clear' ) {
return {};
Expand Down Expand Up @@ -119,24 +121,36 @@ function ListViewComponent(
const blockIndexes = useListViewBlockIndexes( clientIdsTree );

const { getBlock } = useSelect( blockEditorStore );
const { visibleBlockCount, shouldShowInnerBlocks } = useSelect(
( select ) => {
const {
getGlobalBlockCount,
getClientIdsOfDescendants,
__unstableGetEditorMode,
} = select( blockEditorStore );
const draggedBlockCount =
draggedClientIds?.length > 0
? getClientIdsOfDescendants( draggedClientIds ).length + 1
: 0;
return {
visibleBlockCount: getGlobalBlockCount() - draggedBlockCount,
shouldShowInnerBlocks: __unstableGetEditorMode() !== 'zoom-out',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
},
[ draggedClientIds ]
);

const { sectionBlockClientIds, isZoomOutMode, visibleBlockCount } =
useSelect(
( select ) => {
const {
getGlobalBlockCount,
getClientIdsOfDescendants,
__unstableGetEditorMode,
getSectionRootClientId,
getBlockOrder,
} = unlock( select( blockEditorStore ) );
const draggedBlockCount =
draggedClientIds?.length > 0
? getClientIdsOfDescendants( draggedClientIds ).length +
1
: 0;

const _isZoomOutMode = __unstableGetEditorMode() === 'zoom-out';

return {
visibleBlockCount:
getGlobalBlockCount() - draggedBlockCount,
sectionBlockClientIds: getBlockOrder(
getSectionRootClientId()
),
isZoomOutMode: _isZoomOutMode,
};
},
[ draggedClientIds ]
);

const { updateBlockSelection } = useBlockSelection();

Expand Down Expand Up @@ -305,6 +319,8 @@ function ListViewComponent(
setInsertedBlock,
treeGridElementRef: elementRef,
rootClientId,
sectionBlockClientIds,
isZoomOutMode,
} ),
[
blockDropPosition,
Expand All @@ -322,6 +338,8 @@ function ListViewComponent(
insertedBlock,
setInsertedBlock,
rootClientId,
sectionBlockClientIds,
isZoomOutMode,
]
);

Expand Down Expand Up @@ -397,7 +415,6 @@ function ListViewComponent(
fixedListWindow={ fixedListWindow }
selectedClientIds={ selectedClientIds }
isExpanded={ isExpanded }
shouldShowInnerBlocks={ shouldShowInnerBlocks }
showAppender={ showAppender }
/>
</ListViewContext.Provider>
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export const getEnabledClientIdsTree = createSelector(
state.blockEditingModes,
state.settings.templateLock,
state.blockListSettings,
state.editorMode,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#65202 (comment)

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@getdave getdave Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can kind of do that:

gutenberg/packages/block-editor/src/store/selectors.js

Line 850 in 9bdfebf

...getSelectedBlockClientIds.getDependants( state ),

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. 🤔

]
);

Expand Down
Loading