-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
e833f64
7df944f
7e48854
60218be
910a9bc
9ea2fe1
d08aabe
dda0ad3
f076e3d
7b7871a
fd1d795
1914df1
40b99ab
31f04d6
a7f0617
6012f63
589fd6e
b7ad95f
a9cd7be
77b8603
40574f1
a0c1a6b
81266c6
3af4d30
a3071ae
720b9d5
fbcf496
766e34e
76f61df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment explaining what this fixes? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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: | ||
|
@@ -21,12 +24,16 @@ import { unlock } from '../../../lock-unlock'; | |
* @param {string} clientId Block client ID. | ||
*/ | ||
export function useEventHandlers( { clientId, isSelected } ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) => { | ||
|
@@ -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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
|
@@ -91,11 +193,13 @@ export function useEventHandlers( { clientId, isSelected } ) { | |
clientId, | ||
isSelected, | ||
getBlockRootClientId, | ||
getBlockIndex, | ||
insertAfterBlock, | ||
removeBlock, | ||
isZoomOut, | ||
resetZoomLevel, | ||
hasMultiSelection, | ||
startDraggingBlocks, | ||
stopDraggingBlocks, | ||
] | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,5 +60,9 @@ | |
} | ||
} | ||
} | ||
|
||
.wp-block[draggable] { | ||
cursor: grab; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a mention |
||
aria-label={ | ||
bindingsLabel || props[ 'aria-label' ] || placeholder | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I added a different solution for inner blocks. Also left it for frames, which would completely capture pointer event into another document.