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

Block editor: try direct drag (outside text editable) #67305

Merged
merged 29 commits into from
Nov 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
e833f64
block editor: try direct drag if no text selection
ellatrix Nov 26, 2024
7df944f
Fix Safari issue
ellatrix Nov 26, 2024
7e48854
simplify condition
ellatrix Nov 27, 2024
60218be
Disable for multi-selection
ellatrix Nov 27, 2024
910a9bc
Add drap chip
ellatrix Nov 27, 2024
9ea2fe1
Disable when CE and draggable are the same element
ellatrix Nov 27, 2024
d08aabe
Set drag item in store
ellatrix Nov 27, 2024
dda0ad3
Fix spacer
ellatrix Nov 27, 2024
f076e3d
Disable drag when child is selected
ellatrix Nov 27, 2024
7b7871a
fix multi selection issue
ellatrix Nov 27, 2024
fd1d795
Hide toolbar when dragging
ellatrix Nov 27, 2024
1914df1
make resize handles focusable
ellatrix Nov 27, 2024
40b99ab
Hide toolbar differently
ellatrix Nov 27, 2024
31f04d6
Add grabbing for ff
ellatrix Nov 27, 2024
a7f0617
Add comment
ellatrix Nov 28, 2024
6012f63
Add changelog
ellatrix Nov 28, 2024
589fd6e
fix changelog
ellatrix Nov 28, 2024
b7ad95f
Fix firefox drag image
ellatrix Nov 28, 2024
a9cd7be
Remove drag if block can't be moved
ellatrix Nov 28, 2024
77b8603
Fix iframe drag
ellatrix Nov 28, 2024
40574f1
fix custom sources tests
ellatrix Nov 28, 2024
a0c1a6b
Improve image drag rule
ellatrix Nov 28, 2024
81266c6
disable drag on normal svgs
ellatrix Nov 28, 2024
3af4d30
ff compat
ellatrix Nov 28, 2024
a3071ae
Restore hasChildSelected condition
ellatrix Nov 28, 2024
720b9d5
Add e2e test
ellatrix Nov 28, 2024
fbcf496
Improve comment
ellatrix Nov 28, 2024
766e34e
oopsie
ellatrix Nov 28, 2024
76f61df
add ff comment
ellatrix Nov 29, 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
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
// This creates a "slot" where the block you're dragging appeared.
// We use !important as one of the rules are meant to be overridden.
.block-editor-block-list__layout .is-dragging {
background-color: currentColor !important;
opacity: 0.05 !important;
opacity: 0.1 !important;
border-radius: $radius-small !important;

// Disabling pointer events during the drag event is necessary,
// lest the block might affect your drag operation.
pointer-events: none !important;
Copy link
Member Author

Choose a reason for hiding this comment

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

@jasmussen You added this a while back in #33950, but not sure why. I'm guessing to not allow dragging within the dragged block's inner blocks. I've fixed this in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't do this anymore because pointer-events will end drag immediately.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you drag a block from A to B, you don't actually move that block. A copy of the block is made, and shown near your cursor, the block you're intending to move becomes a "gray slot", and only when you release the mouse button, is the actual block moved, and the copy you were dragging destroyed.

I'm pretty sure that's the reason I set pointer-events to none in that PR: if the block you are moving has other inner blocks, or other interactive elements (I guess ondragover related ones), then even though it shows up as a gray slot, it's still I guess a bit interactive.

If you've solved that, then certainly go forward. Pointer events is always a poor CSS propety to tinker with.

Copy link
Member Author

Choose a reason for hiding this comment

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

has other inner blocks, or other interactive elements

Yes, I added a different solution for inner blocks. Also left it for frames, which would completely capture pointer event into another document.

iframe {
pointer-events: none;
}

// Hide the multi selection indicator when dragging.
&::selection {
Expand All @@ -18,3 +17,10 @@
content: none !important;
}
}

// Images are draggable by default, so disable drag for them if not explicitly
// set. This is done so that the block can capture the drag event instead.
.wp-block img:not([draggable]),
.wp-block svg:not([draggable]) {
pointer-events: none;
}
1 change: 1 addition & 0 deletions packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -797,6 +797,7 @@ function BlockListBlockProvider( props ) {
mayDisplayParentControls,
originalBlockClientId,
themeSupportsLayout,
canMove,
};

// Here we separate between the props passed to BlockListBlock and any other
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,3 +427,9 @@ _::-webkit-full-page-media, _:future, :root [data-has-multi-selection="true"] .b
// Additional -1px is required to avoid sub pixel rounding errors allowing background to show.
margin: 0 calc(-1 * var(--wp--style--root--padding-right) - 1px) 0 calc(-1 * var(--wp--style--root--padding-left) - 1px) !important;
}

// This only works in Firefox, Chrome and Safari don't accept a custom cursor
// during drag.
.is-dragging {
cursor: grabbing;
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { useIntersectionObserver } from './use-intersection-observer';
import { useScrollIntoView } from './use-scroll-into-view';
import { useFlashEditableBlocks } from '../../use-flash-editable-blocks';
import { canBindBlock } from '../../../hooks/use-bindings-attributes';
import { useFirefoxDraggableCompatibility } from './use-firefox-draggable-compatibility';

/**
* This hook is used to lightly mark an element as a block element. The element
Expand Down Expand Up @@ -100,11 +101,15 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
isTemporarilyEditingAsBlocks,
defaultClassName,
isSectionBlock,
canMove,
} = useContext( PrivateBlockContext );

const canDrag = canMove && ! hasChildSelected;

// translators: %s: Type of block (i.e. Text, Image etc)
const blockLabel = sprintf( __( 'Block: %s' ), blockTitle );
const htmlSuffix = mode === 'html' && ! __unstableIsHtml ? '-visual' : '';
const ffDragRef = useFirefoxDraggableCompatibility();
const mergedRefs = useMergeRefs( [
props.ref,
useFocusFirstElement( { clientId, initialPosition } ),
Expand All @@ -120,6 +125,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {
isEnabled: isSectionBlock,
} ),
useScrollIntoView( { isSelected } ),
canDrag ? ffDragRef : undefined,
] );

const blockEditContext = useBlockEditContext();
Expand Down Expand Up @@ -152,6 +158,7 @@ export function useBlockProps( props = {}, { __unstableIsHtml } = {} ) {

return {
tabIndex: blockEditingMode === 'disabled' ? -1 : 0,
draggable: canDrag ? true : undefined,
...wrapperProps,
...props,
ref: mergedRefs,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* WordPress dependencies
*/
import { useRefEffect } from '@wordpress/compose';

/**
* In Firefox, the `draggable` and `contenteditable` attributes don't play well
* together. When `contenteditable` is within a `draggable` element, selection
* doesn't get set in the right place. The only solution is to temporarily
* remove the `draggable` attribute clicking inside `contenteditable` elements.
*
* @return {Function} Cleanup function.
*/
export function useFirefoxDraggableCompatibility() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment explaining what this fixes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in 76f61df and rebased this PR too to get the latest drag and drop fixes.

return useRefEffect( ( node ) => {
function onDown( event ) {
node.draggable = ! event.target.isContentEditable;
}
const { ownerDocument } = node;
ownerDocument.addEventListener( 'pointerdown', onDown );
return () => {
ownerDocument.removeEventListener( 'pointerdown', onDown );
};
}, [] );
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import { isTextField } from '@wordpress/dom';
import { ENTER, BACKSPACE, DELETE } from '@wordpress/keycodes';
import { useSelect, useDispatch } from '@wordpress/data';
import { useRefEffect } from '@wordpress/compose';
import { createRoot } from '@wordpress/element';
import { store as blocksStore } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../../store';
import { unlock } from '../../../lock-unlock';
import BlockDraggableChip from '../../../components/block-draggable/draggable-chip';

/**
* Adds block behaviour:
Expand All @@ -21,12 +24,16 @@ import { unlock } from '../../../lock-unlock';
* @param {string} clientId Block client ID.
*/
export function useEventHandlers( { clientId, isSelected } ) {
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 is becoming a bit of a dump every even hook. We should consider a better organization here (maybe a follow-up)

Copy link
Member 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 too bad right now, the hook handles keydown and drag so far, but sure we can split it.

const { getBlockRootClientId, getBlockIndex, isZoomOut } = unlock(
useSelect( blockEditorStore )
);
const { insertAfterBlock, removeBlock, resetZoomLevel } = unlock(
useDispatch( blockEditorStore )
);
const { getBlockType } = useSelect( blocksStore );
const { getBlockRootClientId, isZoomOut, hasMultiSelection, getBlockName } =
unlock( useSelect( blockEditorStore ) );
const {
insertAfterBlock,
removeBlock,
resetZoomLevel,
startDraggingBlocks,
stopDraggingBlocks,
} = unlock( useDispatch( blockEditorStore ) );

return useRefEffect(
( node ) => {
Expand Down Expand Up @@ -76,7 +83,102 @@ export function useEventHandlers( { clientId, isSelected } ) {
* @param {DragEvent} event Drag event.
*/
function onDragStart( event ) {
event.preventDefault();
if (
node !== event.target ||
node.isContentEditable ||
node.ownerDocument.activeElement !== node ||
hasMultiSelection()
) {
event.preventDefault();
return;
}
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
const data = JSON.stringify( {
type: 'block',
srcClientIds: [ clientId ],
srcRootClientId: getBlockRootClientId( clientId ),
} );
event.dataTransfer.effectAllowed = 'move'; // remove "+" cursor
event.dataTransfer.clearData();
event.dataTransfer.setData( 'wp-blocks', data );
const { ownerDocument } = node;
const { defaultView } = ownerDocument;
const selection = defaultView.getSelection();
selection.removeAllRanges();

const domNode = document.createElement( 'div' );
const root = createRoot( domNode );
root.render(
<BlockDraggableChip
icon={ getBlockType( getBlockName( clientId ) ).icon }
/>
);
document.body.appendChild( domNode );
domNode.style.position = 'absolute';
domNode.style.top = '0';
domNode.style.left = '0';
domNode.style.zIndex = '1000';
domNode.style.pointerEvents = 'none';

// Setting the drag chip as the drag image actually works, but
// the behaviour is slightly different in every browser. In
// Safari, it animates, in Firefox it's slightly transparent...
// So we set a fake drag image and have to reposition it
// ourselves.
const dragElement = ownerDocument.createElement( 'div' );
// Chrome will show a globe icon if the drag element does not
// have dimensions.
dragElement.style.width = '1px';
dragElement.style.height = '1px';
dragElement.style.position = 'fixed';
dragElement.style.visibility = 'hidden';
ownerDocument.body.appendChild( dragElement );
event.dataTransfer.setDragImage( dragElement, 0, 0 );

let offset = { x: 0, y: 0 };

if ( document !== ownerDocument ) {
const frame = defaultView.frameElement;
if ( frame ) {
const rect = frame.getBoundingClientRect();
offset = { x: rect.left, y: rect.top };
}
}

// chip handle offset
offset.x -= 58;

function over( e ) {
domNode.style.transform = `translate( ${
e.clientX + offset.x
}px, ${ e.clientY + offset.y }px )`;
}

over( event );

function end() {
ownerDocument.removeEventListener( 'dragover', over );
ownerDocument.removeEventListener( 'dragend', end );
domNode.remove();
dragElement.remove();
stopDraggingBlocks();
document.body.classList.remove(
'is-dragging-components-draggable'
);
ownerDocument.documentElement.classList.remove(
'is-dragging'
);
}

ownerDocument.addEventListener( 'dragover', over );
ownerDocument.addEventListener( 'dragend', end );
ownerDocument.addEventListener( 'drop', end );
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 not really clear to me why we have all these events here.
The other thing is I wonder if some of this code can be shared between the drag handle in the toolbar and here. like a useBlockDragClone or things like that. I don't know enough to tell what's the right abstraction though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan to refactor all that after this. The block drag component can just be a hook. There's some differences that we can't do here such as rendering the drag chip in place. I rendered it a separate React root for this hook, we should do the same in the other place cause it's much simpler. But also, if we want to use block previews at some point the approach will be very different again.


startDraggingBlocks( [ clientId ] );
// Important because it hides the block toolbar.
document.body.classList.add(
'is-dragging-components-draggable'
);
ownerDocument.documentElement.classList.add( 'is-dragging' );
}

node.addEventListener( 'keydown', onKeyDown );
Expand All @@ -91,11 +193,13 @@ export function useEventHandlers( { clientId, isSelected } ) {
clientId,
isSelected,
getBlockRootClientId,
getBlockIndex,
insertAfterBlock,
removeBlock,
isZoomOut,
resetZoomLevel,
hasMultiSelection,
startDraggingBlocks,
stopDraggingBlocks,
]
);
}
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/iframe/content.scss
Original file line number Diff line number Diff line change
Expand Up @@ -60,5 +60,9 @@
}
}
}

.wp-block[draggable] {
cursor: grab;
}
}
}
5 changes: 5 additions & 0 deletions packages/block-editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,11 @@ export function RichTextWrapper(
aria-multiline={ ! disableLineBreaks }
aria-readonly={ shouldDisableEditing }
{ ...props }
// Unset draggable (coming from block props) for contentEditable
// elements because it will interfere with multi block selection
// when the contentEditable and draggable elements are the same
// element.
draggable={ undefined }
Copy link
Contributor

Choose a reason for hiding this comment

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

Which draggable we're unsetting, the one set by the block wrapper I guess. Maybe it's a good thing to mention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a mention

aria-label={
bindingsLabel || props[ 'aria-label' ] || placeholder
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ export default function useBlockDropZone( {
isGroupable,
isZoomOut,
getSectionRootClientId,
getBlockParents,
} = unlock( useSelect( blockEditorStore ) );
const {
showInsertionPoint,
Expand All @@ -358,13 +359,29 @@ export default function useBlockDropZone( {
// So, ensure that the drag state is set when the user drags over a drop zone.
startDragging();
}

const draggedBlockClientIds = getDraggedBlockClientIds();
const targetParents = [
targetRootClientId,
...getBlockParents( targetRootClientId, true ),
];

// Check if the target is within any of the dragged blocks.
const isTargetWithinDraggedBlocks = draggedBlockClientIds.some(
( clientId ) => targetParents.includes( clientId )
);

if ( isTargetWithinDraggedBlocks ) {
return;
}

youknowriad marked this conversation as resolved.
Show resolved Hide resolved
const allowedBlocks = getAllowedBlocks( targetRootClientId );
const targetBlockName = getBlockNamesByClientId( [
targetRootClientId,
] )[ 0 ];

const draggedBlockNames = getBlockNamesByClientId(
getDraggedBlockClientIds()
draggedBlockClientIds
);
const isBlockDroppingAllowed = isDropTargetValid(
getBlockType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,17 @@ export default function useDragSelection() {
} );
}

let lastMouseDownTarget;

function onMouseDown( { target } ) {
lastMouseDownTarget = target;
}

function onMouseLeave( { buttons, target, relatedTarget } ) {
if ( ! target.contains( lastMouseDownTarget ) ) {
return;
}

// If we're moving into a child element, ignore. We're tracking
// the mouse leaving the element to a parent, no a child.
if ( target.contains( relatedTarget ) ) {
Expand Down Expand Up @@ -141,6 +151,7 @@ export default function useDragSelection() {
}

node.addEventListener( 'mouseout', onMouseLeave );
node.addEventListener( 'mousedown', onMouseDown );

return () => {
node.removeEventListener( 'mouseout', onMouseLeave );
Expand Down
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
- Upgraded `@ariakit/react` (v0.4.13) and `@ariakit/test` (v0.4.5) ([#65907](https://github.com/WordPress/gutenberg/pull/65907)).
- Upgraded `@ariakit/react` (v0.4.15) and `@ariakit/test` (v0.4.7) ([#67404](https://github.com/WordPress/gutenberg/pull/67404)).

### Bug Fixes

- `ResizableBox`: Make drag handles focusable ([#67305](https://github.com/WordPress/gutenberg/pull/67305)).

## 28.13.0 (2024-11-27)

### Deprecations
Expand Down
10 changes: 10 additions & 0 deletions packages/components/src/resizable-box/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,16 @@ function UnforwardedResizableBox(
showHandle && 'has-show-handle',
className
) }
// Add a focusable element within the drag handle. Unfortunately,
// `re-resizable` does not make them properly focusable by default,
// causing focus to move the the block wrapper which triggers block
// drag.
handleComponent={ Object.fromEntries(
Object.keys( HANDLE_CLASSES ).map( ( key ) => [
key,
<div key={ key } tabIndex={ -1 } />,
] )
) }
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
handleClasses={ HANDLE_CLASSES }
handleStyles={ HANDLE_STYLES }
ref={ ref }
Expand Down
Loading
Loading