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

Jump to bottom

Allow dragging between adjacent container blocks based on a threshold #56466

Merged
merged 9 commits into from
Dec 12, 2023
Merged

Allow dragging between adjacent container blocks based on a threshold #56466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6a59465
Try: Allow drag between blocks based on a threshold
andrewserong Nov 23, 2023
14a73dd
Update insertion point to reflect threshold
andrewserong Nov 24, 2023
66d4144
Fix threshold for Group block by passing in dropZoneElement
andrewserong Nov 28, 2023
b949ab0
Simplify by reusing operation option
andrewserong Dec 6, 2023
9e0a9f0
Re-use logic that factors in the dragged blocks if we are ultimately …
andrewserong Dec 6, 2023
f9aadc9
Simplify a little further
andrewserong Dec 7, 2023
bf4ba5e
Factor in orientation
andrewserong Dec 7, 2023
3617b49
Add minimum height for threshold so that short blocks are still usable
andrewserong Dec 7, 2023
b1e0fad
Allow threshold when parent block is horizontal, i.e. a Row block
andrewserong Dec 12, 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
128 changes: 114 additions & 14 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 @@ -20,6 +20,10 @@ import {
} from '../../utils/math';
import { store as blockEditorStore } from '../../store';

const THRESHOLD_DISTANCE = 30;
const MINIMUM_HEIGHT_FOR_THRESHOLD = 120;
Copy link
Member

@ramonjd ramonjd Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So, if a block is 120px high, its "inner" dropzone will be 60px high?

Is this an arbitrary min height or does it correspond to some average height for new blocks?

The computed height for a new, empty group block is 48px in my browser.

I guess it makes more sense to want to drop things inside an empty group block rather than around 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.

I guess it makes more sense to want to drop things inside an empty group block rather than around it 😄

That was my thinking. These are just arbitrary numbers for now, but in practice, I figured you probably wouldn't want the threshold for the top/bottom positions being active unless there was at least the same amount of room on the inside of the block to allow for "real" internal dragging. So 120 seemed like a reasonable default to go with for now as it's 4 * the threshold distance. Happy to change any of these values, of course!

const MINIMUM_WIDTH_FOR_THRESHOLD = 120;

/** @typedef {import('../../utils/math').WPPoint} WPPoint */
/** @typedef {import('../use-on-block-drop/types').WPDropOperation} WPDropOperation */

Expand Down Expand Up @@ -48,24 +52,86 @@ import { store as blockEditorStore } from '../../store';
* @param {WPBlockData[]} blocksData The block data list.
* @param {WPPoint} position The position of the item being dragged.
* @param {WPBlockListOrientation} orientation The orientation of the block list.
* @param {Object} options Additional options.
* @return {[number, WPDropOperation]} The drop target position.
*/
export function getDropTargetPosition(
blocksData,
position,
orientation = 'vertical'
orientation = 'vertical',
options = {}
) {
const allowedEdges =
orientation === 'horizontal'
? [ 'left', 'right' ]
: [ 'top', 'bottom' ];

const isRightToLeft = isRTL();

let nearestIndex = 0;
let insertPosition = 'before';
let minDistance = Infinity;

const {
dropZoneElement,
parentBlockOrientation,
rootBlockIndex = 0,
} = options;

// Allow before/after when dragging over the top/bottom edges of the drop zone.
if ( dropZoneElement && parentBlockOrientation !== 'horizontal' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that this doesn't seem to be a problem with blocks in horizontal containers even if they have zero space between them (in my testing the inserter always works)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually spoke too soon 😅 it can be a problem if trying to insert a block between two Stacks inside a Row.

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! We could always expand the logic here. Do you mind sharing a video if you get a chance? I'll continue working on this next week!

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a screengrab of trying to insert a Heading between two Stacks nested inside a Row. You can only really see the drop indicator when hovering at the end of the Row, and that takes a little doing:

drag-in-row.mov

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, yes, that is pretty challenging that one!

I suppose two potential options here:

  • Add in handling for the horizontal orientation here, too, and see if that resolves it (i.e. use threshold on the horizontal axis if the parent is horizontal), or:
  • Treat this PR as vertical only for now, and look at horizontal in a potential follow-up

I'm happy to go either way, but I'll have a play around with horizontal blocks next week and give it a little more thought 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Either option sounds good! Anything we do here will be an improvement on the current state of things 😄

const rect = dropZoneElement.getBoundingClientRect();
const [ distance, edge ] = getDistanceToNearestEdge( position, rect, [
'top',
'bottom',
] );

// If dragging over the top or bottom of the drop zone, insert the block
// before or after the parent block. This only applies to blocks that use
// a drop zone element, typically container blocks such as Group or Cover.
if (
rect.height > MINIMUM_HEIGHT_FOR_THRESHOLD &&
distance < THRESHOLD_DISTANCE
) {
if ( edge === 'top' ) {
return [ rootBlockIndex, 'before' ];
}
if ( edge === 'bottom' ) {
return [ rootBlockIndex + 1, 'after' ];
}
}
}

const isRightToLeft = isRTL();

// Allow before/after when dragging over the left/right edges of the drop zone.
if ( dropZoneElement && parentBlockOrientation === 'horizontal' ) {
const rect = dropZoneElement.getBoundingClientRect();
const [ distance, edge ] = getDistanceToNearestEdge( position, rect, [
'left',
'right',
] );

// If dragging over the left or right of the drop zone, insert the block
// before or after the parent block. This only applies to blocks that use
// a drop zone element, typically container blocks such as Group.
if (
rect.width > MINIMUM_WIDTH_FOR_THRESHOLD &&
distance < THRESHOLD_DISTANCE
) {
if (
( isRightToLeft && edge === 'right' ) ||
( ! isRightToLeft && edge === 'left' )
) {
return [ rootBlockIndex, 'before' ];
}
if (
( isRightToLeft && edge === 'left' ) ||
( ! isRightToLeft && edge === 'right' )
) {
return [ rootBlockIndex + 1, 'after' ];
}
}
}

blocksData.forEach(
( { isUnmodifiedDefaultBlock, getBoundingClientRect, blockIndex } ) => {
const rect = getBoundingClientRect();
Expand Down Expand Up @@ -150,19 +216,27 @@ export default function useBlockDropZone( {
operation: 'insert',
} );

const isDisabled = useSelect(
const { isDisabled, parentBlockClientId, rootBlockIndex } = useSelect(
( select ) => {
const {
__unstableIsWithinBlockOverlay,
__unstableHasActiveBlockOverlayActive,
getBlockIndex,
getBlockParents,
getBlockEditingMode,
} = select( blockEditorStore );
const blockEditingMode = getBlockEditingMode( targetRootClientId );
return (
blockEditingMode !== 'default' ||
__unstableHasActiveBlockOverlayActive( targetRootClientId ) ||
__unstableIsWithinBlockOverlay( targetRootClientId )
);
return {
parentBlockClientId:
getBlockParents( targetRootClientId, true )[ 0 ] || '',
rootBlockIndex: getBlockIndex( targetRootClientId ),
isDisabled:
blockEditingMode !== 'default' ||
__unstableHasActiveBlockOverlayActive(
targetRootClientId
) ||
__unstableIsWithinBlockOverlay( targetRootClientId ),
};
},
[ targetRootClientId ]
);
Expand All @@ -172,9 +246,15 @@ export default function useBlockDropZone( {
const { showInsertionPoint, hideInsertionPoint } =
useDispatch( blockEditorStore );

const onBlockDrop = useOnBlockDrop( targetRootClientId, dropTarget.index, {
operation: dropTarget.operation,
} );
const onBlockDrop = useOnBlockDrop(
dropTarget.operation === 'before' || dropTarget.operation === 'after'
? parentBlockClientId
: targetRootClientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this works, but I can't quite figure out how 😅 because at root level, targetRootClientId should be undefined. But I can drop a Paragraph between two Images at root level, with 0 block spacing, very smoothly!

Also I tried logging the operation here while dragging and it always switches to insert before the drop happens. I guess that's expected because it's actually an insertion?

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 this works, but I can't quite figure out how 😅 because at root level, targetRootClientId should be undefined.

I think the comment for this hook is a bit misleading, unfortunately! At the root of the document, targetRootClientId will be undefined / an empty string, however for a container block at the root of the document that calls useBlockDropZone, it'll pass its clientId in as rootClientId here:

gutenberg/packages/block-editor/src/components/inner-blocks/index.js

Line 218 in 7e61771

rootClientId: clientId,

What this means, is that for a container block, by the time you're inside useBlockDropZone, targetRootClientId refers to the client id of the container block itself. So for our purposes, we need to pass in parentBlockClientId (the parent of the container's clientId) in order for it to work properly. In practice, for a Group block at the root of the document, targetRootClientId will be the client id of the Group block, and parentBlockClientId will be an empty string, as we're at the root of the document.

Hope that helps, I know it's quite a mouthful! If / when this PR looks ready to land, I'll see if I can write this up as a comment prior to this line so that it's clearer — and / or so that we can remember what we did here 😄

Also I tried logging the operation here while dragging and it always switches to insert before the drop happens.

Oh strange! I didn't notice that, but the drop should be happening with the previously returned onBlockDrop that had the last state with the "before" or "after" operation. When you perform a drop, I think it'll immediately clear out the dropTarget state back to its default which is insert, so in practice while logging we might be observing the returning to the previous state 🤔

I'm possibly either over thinking that part (or slightly confused about it), but in practice, before, after, and insert should all do the same thing, as the only operation that does anything differently in useOnBlockDrop is replace.

If it's causing any issues though, I can follow-up next week!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh I see, thanks for explaining! Yes a comment would be great.

If it's causing any issues though, I can follow-up next week!

It's not causing any issues that I could notice, so should be fine to leave as-is!

dropTarget.index,
{
operation: dropTarget.operation,
}
);
const throttled = useThrottle(
useCallback(
( event, ownerDocument ) => {
Expand Down Expand Up @@ -211,26 +291,46 @@ export default function useBlockDropZone( {
const [ targetIndex, operation ] = getDropTargetPosition(
blocksData,
{ x: event.clientX, y: event.clientY },
getBlockListSettings( targetRootClientId )?.orientation
getBlockListSettings( targetRootClientId )?.orientation,
{
dropZoneElement,
parentBlockClientId,
parentBlockOrientation: parentBlockClientId
? getBlockListSettings( parentBlockClientId )
?.orientation
: undefined,
rootBlockIndex,
}
);

registry.batch( () => {
setDropTarget( {
index: targetIndex,
operation,
} );
showInsertionPoint( targetRootClientId, targetIndex, {

const insertionPointClientId = [
'before',
'after',
].includes( operation )
? parentBlockClientId
: targetRootClientId;

showInsertionPoint( insertionPointClientId, targetIndex, {
operation,
} );
} );
},
[
dropZoneElement,
getBlocks,
targetRootClientId,
getBlockListSettings,
registry,
showInsertionPoint,
getBlockIndex,
parentBlockClientId,
rootBlockIndex,
]
),
200
Expand Down
3 changes: 2 additions & 1 deletion packages/block-editor/src/components/use-on-block-drop/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,10 @@ export default function useOnBlockDrop(
operation,
getBlockOrder,
getBlocksByClientId,
insertBlocks,
moveBlocksToPosition,
registry,
removeBlocks,
replaceBlocks,
targetBlockIndex,
targetRootClientId,
]
Expand Down
5 changes: 4 additions & 1 deletion packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
store as blockEditorStore,
} from '@wordpress/block-editor';
import { SelectControl } from '@wordpress/components';
import { useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { View } from '@wordpress/primitives';

Expand Down Expand Up @@ -97,7 +98,8 @@ function GroupEdit( { attributes, name, setAttributes, clientId } ) {
themeSupportsLayout || type === 'flex' || type === 'grid';

// Hooks.
const blockProps = useBlockProps();
const ref = useRef();
const blockProps = useBlockProps( { ref } );

const [ showPlaceholder, setShowPlaceholder ] = useShouldShowPlaceHolder( {
attributes,
Expand All @@ -124,6 +126,7 @@ function GroupEdit( { attributes, name, setAttributes, clientId } ) {
? blockProps
: { className: 'wp-block-group__inner-container' },
{
dropZoneElement: ref.current,
Copy link
Contributor

Choose a reason for hiding this comment

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

So in #56312 a similar change was made in Cover so that dragging over the empty area inside the block would make the inner drop zone show, and that caused the drop zone between blocks with no margins to be hidden. But this change is now necessary to make it possible for the in between drop zone to show between Groups?

My main question is how this may affect third party containers - will all containers now need a dropZoneElement ref passed to useInnerBlocksProps?

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!

My main question is how this may affect third party containers - will all containers now need a dropZoneElement ref passed to useInnerBlocksProps?

With this PR (and right now) I think they might need to if they want to allow the threshold dragging behaviour in this PR. It's a tricky one, because we can't really know exactly what a 3rd party block is doing with its UI and where its inner blocks will be placed in relation to anything else displayed within the block itself (a bit like how we have things like Media & Text, which is a non-standard kind of container block).

I think I'd lean slightly toward guarding the behaviour in this PR behind the dropZoneElement behaviour for now (and having it be manually passed to useInnerBlocksProps) while we're still fine-tuning how all the drag and drop behaviour works. Then, perhaps further down the track, we could look at making it all happen automatically within useInnerBlocksProps somehow? The tricky part in thinking about that, though, is that not all blocks might be using a ref attached to their outer wrapper, so it might be hard to make it all automatic. Still, worth thinking about!

Copy link
Contributor

Choose a reason for hiding this comment

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

we can't really know exactly what a 3rd party block is doing with its UI and where its inner blocks will be placed in relation to anything else

Yeah this is a good point. In testing with plain WP 6.4 I notice that it's also not possible to drop between two Columns blocks, but it is between Media & Text blocks. Those behaviours are unaltered in this PR so I guess that means regressions are unlikely?

I'm still not sure I quite grasp why the same thing that made dropping between blocks stop working for Cover in #56312 (passing the ref to useInnerBlocksProps) is making dropping between Group blocks work in this PR 😅

Copy link
Contributor Author

@andrewserong andrewserong Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm still not sure I quite grasp why the same thing that made dropping between blocks stop working for Cover in #56312 (passing the ref to useInnerBlocksProps) is making dropping between Group blocks work in this PR 😅

It's because we're doing it at the same time as the rest of this PR 😄
If we did a standalone PR that passed dropZoneElement to the Group block's useInnerBlocksProps then we'd expose the issue that we can't drag between two adjacent Group blocks, because the two blocks' dropZoneElements would be adjacent to each other, without any threshold for drag-between. But because this PR now enables a drag threshold within the dropZoneElement, the Group block gets to skip the intermediate step where we break the ability to drag between adjacent blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already broken though - I guess the difference is that on trunk a drop indicator appears, apparently between the blocks, but in reality dropping over it just adds the dropped block to one of the Groups.

This happens in WP 6.4 with or without Gutenberg:

drop-between-groups.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! Yep, on trunk, I think you're right, it looks like it's just dropping within 👍

I must admit, I do find it hard to maintain a good mental model for how all this hooks together 😄

Thanks again for giving this a 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 must admit, I do find it hard to maintain a good mental model for how all this hooks together

Same 😭

templateLock,
allowedBlocks,
renderAppender,
Expand Down
Loading