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

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

Merged
merged 16 commits into from
Dec 21, 2023
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a2fd12c
Hide drop indicator where block isn't allowed to drop.
tellthemachines Dec 7, 2023
761fb61
Don't show indicator when drop outside of allowed parent
tellthemachines Dec 7, 2023
4f0513f
logic to change the draggable icon
tellthemachines Dec 11, 2023
e0a9753
Make chip transparent when dragging over undroppable area.
tellthemachines Dec 12, 2023
43a1bc0
Remove unused prop
tellthemachines Dec 12, 2023
359b5b8
Check insertion point visibility
tellthemachines Dec 13, 2023
59607c9
Move some code around
tellthemachines Dec 13, 2023
9e40e4e
Throttle the event listener
tellthemachines Dec 14, 2023
3bf8a0f
Add dependencies to effect
tellthemachines Dec 14, 2023
5270a64
Update CSS
tellthemachines Dec 14, 2023
f8aba09
Fade chip only when dragging in editor canvas
tellthemachines Dec 14, 2023
1da2470
Merge trunk and solve conflicts with #57032
tellthemachines Dec 18, 2023
f194767
Improve disabled chip
tellthemachines Dec 18, 2023
37096af
Update to styled span.
tellthemachines Dec 19, 2023
e9f3a48
Only check validity of container blocks.
tellthemachines Dec 21, 2023
240cd39
Merge remote-tracking branch 'origin/trunk' into fix/drop-indicator-n…
tellthemachines Dec 21, 2023
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
5 changes: 4 additions & 1 deletion packages/block-editor/src/components/block-draggable/draggable-chip.js
View file
Open in desktop
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
91 changes: 87 additions & 4 deletions packages/block-editor/src/components/block-draggable/index.js
View file
Open in desktop
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 { isDropTargetValid } from '../use-block-drop-zone';

const BlockDraggable = ( {
children,
Expand All @@ -20,15 +22,28 @@ const BlockDraggable = ( {
onDragStart,
onDragEnd,
} ) => {
const { srcRootClientId, isDraggable, icon } = useSelect(
const {
srcRootClientId,
isDraggable,
icon,
visibleInserter,
getBlockType,
getAllowedBlocks,
draggedBlockNames,
getBlockNamesByClientId,
} = useSelect(
( select ) => {
const {
canMoveBlocks,
getBlockRootClientId,
getBlockName,
getBlockAttributes,
isBlockInsertionPointVisible,
getDraggedBlockClientIds,
getBlockNamesByClientId: _getBlockNamesByClientId,
getAllowedBlocks: _getAllowedBlocks,
} = select( blockEditorStore );
const { getBlockType, getActiveBlockVariation } =
const { getBlockType: _getBlockType, getActiveBlockVariation } =
select( blocksStore );
const rootClientId = getBlockRootClientId( clientIds[ 0 ] );
const blockName = getBlockName( clientIds[ 0 ] );
Expand All @@ -40,11 +55,21 @@ const BlockDraggable = ( {
return {
srcRootClientId: rootClientId,
isDraggable: canMoveBlocks( clientIds, rootClientId ),
icon: variation?.icon || getBlockType( blockName )?.icon,
icon: variation?.icon || _getBlockType( blockName )?.icon,
visibleInserter: isBlockInsertionPointVisible(),
getBlockType: _getBlockType,
getAllowedBlocks: _getAllowedBlocks,
draggedBlockNames: _getBlockNamesByClientId(
getDraggedBlockClientIds()
),
getBlockNamesByClientId: _getBlockNamesByClientId,
};
},
[ clientIds ]
);

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

const isDragging = useRef( false );
const [ startScrolling, scrollOnDragOver, stopScrolling ] =
useScrollWhenDragging();
Expand All @@ -61,6 +86,64 @@ 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 •
edited
Loading

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 ) {
setTargetClientId( newTargetClientId );
}

const allowedBlocks = getAllowedBlocks( targetClientId );
const targetBlockName = getBlockNamesByClientId( [
targetClientId,
] )[ 0 ];
const dropTargetValid = isDropTargetValid(
getBlockType,
allowedBlocks,
draggedBlockNames,
targetBlockName
);

// 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 ( ! dropTargetValid && ! visibleInserter ) {
window?.document?.body?.classList?.add(
'block-draggable-invalid-drag-token'
);
} else {
window?.document?.body?.classList?.remove(
'block-draggable-invalid-drag-token'
);
}
};

editorRoot.addEventListener( 'dragover', onDragOver );

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

if ( ! isDraggable ) {
return children( { draggable: false } );
}
Expand Down
8 changes: 8 additions & 0 deletions packages/block-editor/src/components/block-draggable/style.scss
View file
Open in desktop
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;
}
86 changes: 84 additions & 2 deletions packages/block-editor/src/components/use-block-drop-zone/index.js
View file
Open in desktop
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,50 @@ export function getDropTargetPosition(
];
}

/**
* Check if the dragged blocks can be dropped on the target.
* @param {Function} getBlockType
* @param {Object[]} allowedBlocks
* @param {string[]} draggedBlockNames
* @param {string} targetBlockName
* @return {boolean} Whether the dragged blocks can be dropped on the target.
*/
export function isDropTargetValid(
getBlockType,
allowedBlocks,
draggedBlockNames,
targetBlockName
) {
// 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 +263,27 @@ export default function useBlockDropZone( {
operation: 'insert',
} );

const { isDisabled, parentBlockClientId, rootBlockIndex } = useSelect(
const {
parentBlockClientId,
rootBlockIndex,
isDisabled,
getBlockType,
allowedBlocks,
draggedBlockNames,
targetBlockName,
} = useSelect(
( select ) => {
const {
__unstableIsWithinBlockOverlay,
__unstableHasActiveBlockOverlayActive,
getBlockIndex,
getBlockParents,
getBlockEditingMode,
getDraggedBlockClientIds,
getBlockNamesByClientId,
getAllowedBlocks,
} = select( blockEditorStore );
const { getBlockType: _getBlockType } = select( blocksStore );
const blockEditingMode = getBlockEditingMode( targetRootClientId );
return {
parentBlockClientId:
Expand All @@ -236,6 +295,15 @@ export default function useBlockDropZone( {
targetRootClientId
) ||
__unstableIsWithinBlockOverlay( targetRootClientId ),

getBlockType: _getBlockType,
allowedBlocks: getAllowedBlocks( targetRootClientId ),
draggedBlockNames: getBlockNamesByClientId(
getDraggedBlockClientIds()
),
targetBlockName: getBlockNamesByClientId( [
targetRootClientId,
] )[ 0 ],
};
},
[ targetRootClientId ]
Expand All @@ -258,6 +326,16 @@ export default function useBlockDropZone( {
const throttled = useThrottle(
useCallback(
( event, ownerDocument ) => {
const isBlockDroppingAllowed = isDropTargetValid(
getBlockType,
allowedBlocks,
draggedBlockNames,
targetBlockName
);
if ( ! isBlockDroppingAllowed ) {
return;
}

const blocks = getBlocks( targetRootClientId );

// The block list is empty, don't show the insertion point but still allow dropping.
Expand Down Expand Up @@ -331,6 +409,10 @@ export default function useBlockDropZone( {
getBlockIndex,
parentBlockClientId,
rootBlockIndex,
getBlockType,
allowedBlocks,
draggedBlockNames,
targetBlockName,
]
),
200
Expand Down
Loading