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

Double click block to exit zoom out mode #64573

Merged
merged 10 commits into from
Sep 4, 2024
Merged

Double click block to exit zoom out mode #64573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b472b19
Add action for exiting Zoom Out mode
getdave Aug 16, 2024
deb4c4c
Add new hook for handling existing zoom out mode
getdave Aug 16, 2024
2940cb5
Use correct words
getdave Aug 22, 2024
172f9bc
Remove selectBlock conditional
getdave Aug 23, 2024
44f7824
Set edit mode directly
getdave Aug 23, 2024
5d89474
Remove new action
getdave Aug 23, 2024
088f6c5
Remove unused isBlockSelected prop from dependency array
jeryj Sep 3, 2024
388ee4b
Add editorMode to blockProps context to reduce useSelect and event li…
jeryj Sep 3, 2024
eab3e1b
Remove block selection from zoom out hook exit
jeryj Sep 3, 2024
4fdbf7c
Remove unused comment
jeryj Sep 3, 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
8 changes: 6 additions & 2 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,6 +646,8 @@ function BlockListBlockProvider( props ) {
blocksWithSameName.length &&
blocksWithSameName[ 0 ] !== clientId;

const editorMode = __unstableGetEditorMode();

return {
...previewContext,
mode: getBlockMode( clientId ),
Expand All @@ -672,6 +674,7 @@ function BlockListBlockProvider( props ) {
) && hasSelectedInnerBlock( clientId ),
blockApiVersion: blockType?.apiVersion || 1,
blockTitle: match?.title || blockType?.title,
editorMode,
isSubtreeDisabled:
blockEditingMode === 'disabled' &&
isBlockSubtreeDisabled( clientId ),
Expand All @@ -680,8 +683,7 @@ function BlockListBlockProvider( props ) {
! isDragging(),
initialPosition:
_isSelected &&
( __unstableGetEditorMode() === 'edit' ||
__unstableGetEditorMode() === 'zoom-out' ) // Don't recalculate the initialPosition when toggling in/out of zoom-out mode
( editorMode === 'edit' || editorMode === 'zoom-out' ) // Don't recalculate the initialPosition when toggling in/out of zoom-out mode
? getSelectedBlocksInitialCaretPosition()
: undefined,
isHighlighted: isBlockHighlighted( clientId ),
Expand Down Expand Up @@ -728,6 +730,7 @@ function BlockListBlockProvider( props ) {
themeSupportsLayout,
isTemporarilyEditingAsBlocks,
blockEditingMode,
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.

As we're touching the deps, we should monitor the editor performance after merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was remembering #56994 about combining block store subscriptions, and assumed it would be better to add it here rather than check on each block. Maybe the performance potential isn't worth it though. I do like the implementation though, as it makes the final result quite clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point about combining store subscriptions. Let's try it "as is" and watch the performance charts post-merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some smoke test with running performance in the inspector and entering into/out of zoom out mode and this method of using the combined subscription and not adding listeners was always faster in both total scripting and rendering, so I say we go with it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. I say we merge it and then watch the performance dashboard in case of any changes post-merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeryj Notice any problems in the Dashboard? I'm not seeing any.

mayDisplayControls,
mayDisplayParentControls,
index,
Expand Down Expand Up @@ -782,6 +785,7 @@ function BlockListBlockProvider( props ) {
hasOverlay,
initialPosition,
blockEditingMode,
editorMode,
isHighlighted,
isMultiSelected,
isPartiallySelected,
Expand Down
3 changes: 3 additions & 0 deletions packages/block-editor/src/components/block-list/use-block-props/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import { useFocusHandler } from './use-focus-handler';
import { useEventHandlers } from './use-selected-block-event-handlers';
import { useNavModeExit } from './use-nav-mode-exit';
import { useZoomOutModeExit } from './use-zoom-out-mode-exit';
import { useBlockRefProvider } from './use-block-refs';
import { useIntersectionObserver } from './use-intersection-observer';
import { useScrollIntoView } from './use-scroll-into-view';
Expand Down Expand Up @@ -85,6 +86,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
name,
blockApiVersion,
blockTitle,
editorMode,
isSelected,
isSubtreeDisabled,
hasOverlay,
Expand Down Expand Up @@ -115,6 +117,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
useFocusHandler( clientId ),
useEventHandlers( { clientId, isSelected } ),
useNavModeExit( clientId ),
useZoomOutModeExit( { editorMode } ),
useIsHovered( { clientId } ),
useIntersectionObserver(),
useMovingAnimation( { triggerAnimationOnChange: index, clientId } ),
Expand Down
44 changes: 44 additions & 0 deletions packages/block-editor/src/components/block-list/use-block-props/use-zoom-out-mode-exit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* WordPress dependencies
*/
import { useDispatch } from '@wordpress/data';
import { useRefEffect } from '@wordpress/compose';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../../store';
import { unlock } from '../../../lock-unlock';

/**
* Allows Zoom Out mode to be exited by double clicking in the selected block.
*
* @param {string} clientId Block client ID.
*/
export function useZoomOutModeExit( { editorMode } ) {
getdave marked this conversation as resolved.
Show resolved Hide resolved
const { __unstableSetEditorMode } = unlock(
useDispatch( blockEditorStore )
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Zoom out mode is fairly rare right now. Rather than adding a double click to all blocks, could we early return here if we're not in zoom out? That way, we only add these listeners when it's useful?

return useRefEffect(
( node ) => {
if ( editorMode !== 'zoom-out' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having the editorMode as a dependency of the useRefEffect and regenerating a new ref / new event listener everytime this change. We could just call getEditorMode within onDoubleClick event listener (Like we do for useNavModeExit

This would also remove the need to pass the editorMode top/down through the context...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing the opposite comment above. I think personally the performance impact of having the double click always there is less than re-rendering when the editorMode changes, it's also way simpler in terms of code. But I'm ok if you all disagree.

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 had the same thought process. I believe @jeryj did some performance profiling on this so he may be able to answer more concretely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had done profiling on the page and not adding the event listener used a lot less resources. The metrics for typing are going up. I'm having a hard time isolating a PR that is causing it. It's odd to me that adding the mode to the block props would cause that, as the editing mode doesn't really change often and definitely isn't changed in the middle of the performance tests. I can make another branch that reworks this to this older style and we can compare.

Copy link
Contributor

Choose a reason for hiding this comment

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

#65326

@getdave the original way we did this also included passing clientId to the refEffect which would have made a difference in the profiling. We don't need that anymore, so hopefully the performance will be better now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any major changes in performance with either approach.

Copy link
Member

Choose a reason for hiding this comment

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

The typing metic was probably affected by a new store subscription. See #65337.

return;
}

function onDoubleClick( event ) {
if ( ! event.defaultPrevented ) {
event.preventDefault();
__unstableSetEditorMode( 'edit' );
}
}

node.addEventListener( 'dblclick', onDoubleClick );

return () => {
node.removeEventListener( 'dblclick', onDoubleClick );
};
},
[ editorMode, __unstableSetEditorMode ]
);
}
Loading