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.

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

Hide drop indicator where block isn't allowed to drop. #56843

Merged
merged 16 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ import BlockIcon from '../block-icon';
export default function BlockDraggableChip( { count, icon, isPattern } ) {
const patternLabel = isPattern && __( 'Pattern' );
return (
<div className="block-editor-block-draggable-chip-wrapper">
<div
className="block-editor-block-draggable-chip-wrapper"
style={ { opacity: 'var(--wp--block-draggable-valid-opacity)' } }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this applied via an inline style and CSS variable instead of by classname and direct value in style.scss? Or to put it slightly differently, does the JS version of the component need to have an opinion about how it's styled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I don't think it has to be applied as an inline style; I initially added it that way for the sake of development speed. It does have to be a CSS custom property, because its value needs to change when the body class gets added, and given that the chip we see on the page is a stored copy of itself changing classes on the chip itself won't work. Essentially the custom property is acting like a ref to the element, so we can manipulate its value from outside.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does have to be a CSS custom property, because its value needs to change when the body class gets added

Oh, I was imagining something like the following:

.block-editor-block-draggable-chip {
	opacity: 1;
}

.block-draggable-invalid-drag-token {
	.block-editor-block-draggable-chip {
		opacity: 0.6;
	}
}

Or does that not work? (Totally fine to go with a CSS variable by the way, just trying to understand what the limitations are 🙂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, yeah that should work too! Would that be preferable to a custom property? I don't think specificity matters either way.

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 feel strongly either way, but I'd lean slightly more toward the nested selectors so that it's easy to swap out for (or combine) other styles like background color, border, or other subtle styling changes that folks might want to explore in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good point, yeah that would make it easier to change styles altogether if we wanted to.

>
<div
className="block-editor-block-draggable-chip"
data-testid="block-draggable-chip"
Expand Down
118 changes: 91 additions & 27 deletions packages/block-editor/src/components/block-draggable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
import { store as blocksStore } from '@wordpress/blocks';
import { Draggable } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { useEffect, useRef, useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import BlockDraggableChip from './draggable-chip';
import useScrollWhenDragging from './use-scroll-when-dragging';
import { store as blockEditorStore } from '../../store';
import { __unstableUseBlockRef as useBlockRef } from '../block-list/use-block-props/use-block-refs';
import { useIsDropTargetValid } from '../use-block-drop-zone';

const BlockDraggable = ( {
children,
Expand All @@ -20,31 +22,39 @@ const BlockDraggable = ( {
onDragStart,
onDragEnd,
} ) => {
const { srcRootClientId, isDraggable, icon } = useSelect(
( select ) => {
const {
canMoveBlocks,
getBlockRootClientId,
getBlockName,
getBlockAttributes,
} = select( blockEditorStore );
const { getBlockType, getActiveBlockVariation } =
select( blocksStore );
const rootClientId = getBlockRootClientId( clientIds[ 0 ] );
const blockName = getBlockName( clientIds[ 0 ] );
const variation = getActiveBlockVariation(
blockName,
getBlockAttributes( clientIds[ 0 ] )
);

return {
srcRootClientId: rootClientId,
isDraggable: canMoveBlocks( clientIds, rootClientId ),
icon: variation?.icon || getBlockType( blockName )?.icon,
};
},
[ clientIds ]
);
const { srcRootClientId, isDraggable, icon, getBlockListSettings } =
useSelect(
( select ) => {
const {
canMoveBlocks,
getBlockRootClientId,
getBlockName,
getBlockAttributes,
getBlockListSettings: _getBlockListSettings,
} = select( blockEditorStore );
const { getBlockType, getActiveBlockVariation } =
select( blocksStore );
const rootClientId = getBlockRootClientId( clientIds[ 0 ] );
const blockName = getBlockName( clientIds[ 0 ] );
const variation = getActiveBlockVariation(
blockName,
getBlockAttributes( clientIds[ 0 ] )
);

return {
srcRootClientId: rootClientId,
isDraggable: canMoveBlocks( clientIds, rootClientId ),
icon: variation?.icon || getBlockType( blockName )?.icon,
getBlockListSettings: _getBlockListSettings,
};
},
[ clientIds ]
);

const [ targetClientId, setTargetClientId ] = useState( null );

const isDropTargetValid = useIsDropTargetValid( clientIds, targetClientId );
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the BlockDraggable component need to know whether the drop target is valid? Could we use the getBlockInsertionPoint state to figure out if an insertion point is currently set instead?

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 don't think so; given that getBlockInsertionPoint is always meant to return an index, it would be hard to work out which targets are valid.

Copy link
Contributor

@andrewserong andrewserong Dec 13, 2023

Choose a reason for hiding this comment

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

Ah, gotcha! Looks like there's another selector isBlockInsertionPointVisible which handles the null case. Could that work if we'd called hideInsertionPoint in the main throttled useCallback in useBlockDropZone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice find! Lemme try that 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.

Ok checking if the insertion point is visible does fix the discrepancy between transparent drag chip and inserter, but I found it necessary to keep the check for valid insertion point in place, because otherwise the chip keeps flickering every time the insertion point switches places. Still it's working much better now so thanks for the tip!


const isDragging = useRef( false );
const [ startScrolling, scrollOnDragOver, stopScrolling ] =
useScrollWhenDragging();
Expand All @@ -61,6 +71,55 @@ const BlockDraggable = ( {
};
}, [] );

// Find the root of the editor iframe.
const blockRef = useBlockRef( clientIds[ 0 ] );
const editorRoot = blockRef.current?.closest( 'body' );

// Add a dragover event listener to the editor root to track the blocks being dragged over.
// The listener has to be inside the editor iframe otherwise the target isn't accessible.
// Check if the dragged blocks are allowed inside the target. If not, grey out the draggable.
useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this useEffect and its onDragOver still needed? Since the useBlockDropZone is hiding the insertion point, could we replace the whole useEffect with something like:

	useEffect( () => {
		if ( ! visibleInserter ) {
			window?.document?.body?.classList?.add(
				'block-draggable-invalid-drag-token'
			);
		} else {
			window?.document?.body?.classList?.remove(
				'block-draggable-invalid-drag-token'
			);
		}
	}, [ visibleInserter ] );

Then the logic of when to show and hide the insertion point could live in one place, so that the BlockDraggable doesn't have to also perform checks with its own drag event handler.

Or is there another reason why this component needs to perform its own separate checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so the crucial bit here is finding the targetClientId, which tells us the container that the draggable is currently hovering over. To find that we need a dragover event listener attached to the inside of the editor iframe (if it's on the outside, it won't be able to access the elements inside the iframe). We need the targetClientId to check if the drop target is valid, which is necessary because if the chip is hovering over a valid container but there's no drop indicator currently showing (if we're moving between drop areas for instance), we don't want it to grey out.

Unfortunately there's not much that can be done to restrict how many times this runs without slowing down the responsiveness of the drag chip to what it's hovering over. The problem here is that, unlike with the drop indicators that exist statically between all blocks so we know where they're placed from the start, we need to find out what the draggable is being dragged over, and afaik the only way to do that is through the event target of a dragover listener.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need the targetClientId to check if the drop target is valid, which is necessary because if the chip is hovering over a valid container but there's no drop indicator currently showing (if we're moving between drop areas for instance), we don't want it to grey out.

Ah, gotcha! Thanks for the explanation, that makes sense now 👍

Unfortunately there's not much that can be done to restrict how many times this runs without slowing down the responsiveness of the drag chip to what it's hovering over.

Yeah, it can be really tricky to get the right balance when slowing things down so that it's slow enough for performance reasons, but fast enough to feel responsive to a user! 😅

we need to find out what the draggable is being dragged over, and afaik the only way to do that is through the event target of a dragover listener.

That sounds about right to me, too. If it winds up being a bit of a rabbithole trying to get it working smoothly, would it be worth splitting out the useBlockDropZone change and landing that first? Totally cool if you want to keep it all together while figuring everything out of course!

if ( ! editorRoot ) {
return;
}

const onDragOver = ( event ) => {
Copy link
Contributor

@andrewserong andrewserong Dec 13, 2023

Choose a reason for hiding this comment

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

I think this callback might need to be throttled similar to the code in useBlockDropZone as this currently fires very frequently:

2023-12-14.10.37.55.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm yeah I'm wondering how best to do that given the inability to use hooks inside the effect callback 🤔

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 I ran into this issue when testing, it seemed to crash in Safari and I couldn't move the chip any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oo wait I see there's a throttle util in compose that we may be able to leverage, will try that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok the throttle seems to make it better! I set it to the same as draggable.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a little better, but it's still lagging a bit for me with the throttle at 16 (it's more visible when using CPU throttling in dev tools). I think a couple of things might be at play:

  • Since the useEffect doesn't use a dependency array, it seems that it's being called quite frequently, and potentially adding (and removing) event listeners too frequently? For example, if I increase the throttle time to 2000, it still appears to be firing rapidly
  • In draggable, 16ms is needed because it's updating the position of the drag chip. Here, I'm wondering if we can use a bigger delay / throttle, if that could smooth over the flicker when dragging between image blocks in a gallery block? I.e. if we had it at 200ms to match useBlockDropZone then if folks are dragging quickly between blocks, hopefully it shouldn't flicker as much?

Would it work to maybe move defining the onDragOver callback to be outside of the useEffect (possibly in a useCallback) so that the useEffect could have a dependency array and only attach the event listener once? Apologies if you've already tried that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I had a think about this 😄 and realised that we actually don't need to use local state for targetClientId, and we can use a dependency array for the effect, which improves things a little, but it still has to re-render whenever either draggedBlockNames or visibleInserter change. Moving the callback out of the effect makes no difference, because it calls whichever version of the callback was attached to the event listener when the effect ran, so if we want the values inside the callback to change the effect has to re-run.

I also tried increasing the throttle time and it doesn't seem to make things any worse, so might leave it at 200 or so. I just pushed an update, let me know if it improves things at all!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice work! That appears to have fixed up the performance issues for me. Subjectively, without CPU throttling, it's feeling smooth now, and with CPU throttling, it seems much the same as trunk to me now 🎉

if ( ! event.target.closest( '[data-block]' ) ) {
return;
}

const newTargetClientId = event.target
.closest( '[data-block]' )
.getAttribute( 'data-block' );
//Only update targetClientId if it has changed
// and if it's a container block
if (
targetClientId !== newTargetClientId &&
getBlockListSettings( newTargetClientId )
Copy link
Member

Choose a reason for hiding this comment

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

Is getBlockListSettings( newTargetClientId ) checking for nested blocks to see if it's a container?

I think calling a select outside useSelect is an anti-pattern as it risks not returning fresh data when the store is updated.

I tested by updating the list settings for a block via wp.data.dispatch('core/block-editor').updateBlockListSettings() and it looks like the return state is updated, I guess because a rerender is triggered by another prop. So it's working in that case as far as I can tell.

cc @jsnajdr in case I missed something.

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 don't think it's an anti-pattern, we use select functions outside of useSelect all over the place 😅

getBlockListSettings was the most reliable way I found of checking whether a block is a container, given that only containers are block lists (and they might not have any inner blocks if they're empty). I'm definitely open to ideas on this one though 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually scratch that, I don't think we need to check for the container anymore 😂 now we're using the inserter visibility as per @andrewserong's suggestion.

Still keen to hear about preferred code patterns though 👀

Copy link
Member

Choose a reason for hiding this comment

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

Here getBlockListSettings is called in an event handler (onDragOver) and that's fine. We want to read the fresh value when the event is being dispatched, and that's what happens here. No rerenders need to be triggered when the value changes.

The only thing I would change here is to write the code, for clarity, as two separate useSelect calls:

const { getBlockListSettings } = useSelect( blockEditorStore );
// or alternatively
const { getBlockListSettings } = useRegistry().select( blockEditorStore );

const { ...rest } = useSelect( ( select ) => ... );

One of them just gets a reference to the selector, without any store subscriptions or reactivity, and the second reads data in the usual reactive way.

Copy link
Member

@ramonjd ramonjd Dec 13, 2023

Choose a reason for hiding this comment

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

Thanks for confirming and setting me straight on that one, @jsnajdr!

And sorry about the misdirection @tellthemachines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks for the clarification!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing I would change here is to write the code, for clarity, as two separate useSelect calls

There are other selectors we're grabbing directly and using inside the effect such as getBlockType and getAllowedBlocks. I guess following that logic they should all be in a separate useSelect call?

) {
setTargetClientId( newTargetClientId );
}
// Update the body class to reflect if drop target is valid.
// This has to be done on the document body because the draggable
// chip is rendered outside of the editor iframe.
if ( isDropTargetValid ) {
window?.document?.body?.classList?.remove(
'block-draggable-invalid-drag-token'
);
} else {
window?.document?.body?.classList?.add(
'block-draggable-invalid-drag-token'
);
}
};

editorRoot.addEventListener( 'dragover', onDragOver );

return () => {
editorRoot.removeEventListener( 'dragover', onDragOver );
};
} );

if ( ! isDraggable ) {
return children( { draggable: false } );
}
Expand Down Expand Up @@ -102,7 +161,12 @@ const BlockDraggable = ( {
}
} }
__experimentalDragComponent={
<BlockDraggableChip count={ clientIds.length } icon={ icon } />
<BlockDraggableChip
count={ clientIds.length }
icon={ icon }
clientIds={ clientIds }
targetClientId={ targetClientId }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this addition currently doing anything? It looks like BlockDraggableChip isn't currently using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I must have left that in while experimenting 😄

/>
}
>
{ ( { onDraggableStart, onDraggableEnd } ) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,11 @@
font-size: $default-font-size;
}
}

:root {
--wp--block-draggable-valid-opacity: 1;
}

.block-draggable-invalid-drag-token {
--wp--block-draggable-valid-opacity: 0.6;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
__experimentalUseDropZone as useDropZone,
} from '@wordpress/compose';
import { isRTL } from '@wordpress/i18n';
import { isUnmodifiedDefaultBlock as getIsUnmodifiedDefaultBlock } from '@wordpress/blocks';
import {
isUnmodifiedDefaultBlock as getIsUnmodifiedDefaultBlock,
store as blocksStore,
} from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -191,6 +194,65 @@ export function getDropTargetPosition(
];
}

/**
* A Rect hook that takes an array of dragged block client ids and a target client id
* and determines if the dragged blocks can be dropped on the target.
*
* @param {string[]} draggedBlockClientIds The client ids of the dragged blocks.
* @param {string} targetClientId The client id of the target.
* @return {boolean} Whether the dragged blocks can be dropped on the target.
*/
export function useIsDropTargetValid( draggedBlockClientIds, targetClientId ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This hook appears to get called even when a drag event isn't happening. Is it possible to move some of the logic to the throttled useCallback so that we only figure out the allowed blocks in the throttled function that gets called while a user is dragging? Ideally it'd be good to limit what gets called before the drag actions occur, but it might not be possible to move everything over 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be refactored to a simple function taking getBlockType, allowedBlocks, draggedBlockNames, targetBlockName as parameters and then called inside the throttle. I'm unsure if determining isBlockDroppingAllowed inside the throttle will cause any lag but let's give it a try 😄

const { getBlockType, allowedBlocks, draggedBlockNames, targetBlockName } =
useSelect(
( select ) => {
const { getBlockType: _getBlockType } = select( blocksStore );
const { getBlockNamesByClientId, getAllowedBlocks } =
select( blockEditorStore );

return {
getBlockType: _getBlockType,
allowedBlocks: getAllowedBlocks( targetClientId ),
draggedBlockNames: getBlockNamesByClientId(
draggedBlockClientIds
),
targetBlockName: getBlockNamesByClientId( [
targetClientId,
] )[ 0 ],
};
},
[ draggedBlockClientIds, targetClientId ]
);

// At root level allowedBlocks is undefined and all blocks are allowed.
// Otherwise, check if all dragged blocks are allowed.
let areBlocksAllowed = true;
if ( allowedBlocks ) {
const allowedBlockNames = allowedBlocks?.map( ( { name } ) => name );

areBlocksAllowed = draggedBlockNames.every( ( name ) =>
allowedBlockNames?.includes( name )
);
}

// Work out if dragged blocks have an allowed parent and if so
// check target block matches the allowed parent.
const draggedBlockTypes = draggedBlockNames.map( ( name ) =>
getBlockType( name )
);
const targetMatchesDraggedBlockParents = draggedBlockTypes.every(
( block ) => {
const [ allowedParentName ] = block?.parent || [];
if ( ! allowedParentName ) {
return true;
}

return allowedParentName === targetBlockName;
}
);
return areBlocksAllowed && targetMatchesDraggedBlockParents;
}

/**
* @typedef {Object} WPBlockDropZoneConfig
* @property {?HTMLElement} dropZoneElement Optional element to be used as the drop zone.
Expand All @@ -216,15 +278,22 @@ export default function useBlockDropZone( {
operation: 'insert',
} );

const { isDisabled, parentBlockClientId, rootBlockIndex } = useSelect(
const {
parentBlockClientId,
rootBlockIndex,
isDisabled,
draggedBlockClientIds,
} = useSelect(
( select ) => {
const {
__unstableIsWithinBlockOverlay,
__unstableHasActiveBlockOverlayActive,
getBlockIndex,
getBlockParents,
getBlockEditingMode,
getDraggedBlockClientIds,
} = select( blockEditorStore );

const blockEditingMode = getBlockEditingMode( targetRootClientId );
return {
parentBlockClientId:
Expand All @@ -236,11 +305,17 @@ export default function useBlockDropZone( {
targetRootClientId
) ||
__unstableIsWithinBlockOverlay( targetRootClientId ),
draggedBlockClientIds: getDraggedBlockClientIds(),
};
},
[ targetRootClientId ]
);

const isBlockDroppingAllowed = useIsDropTargetValid(
draggedBlockClientIds,
targetRootClientId
);

const { getBlockListSettings, getBlocks, getBlockIndex } =
useSelect( blockEditorStore );
const { showInsertionPoint, hideInsertionPoint } =
Expand Down Expand Up @@ -338,7 +413,7 @@ export default function useBlockDropZone( {

return useDropZone( {
dropZoneElement,
isDisabled,
isDisabled: isDisabled || ! isBlockDroppingAllowed,
onDrop: onBlockDrop,
onDragOver( event ) {
// `currentTarget` is only available while the event is being
Expand Down
Loading