Disallow drag from Nav list view into block canvas #47615
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
[Type] Bug
getdave
commented
Jan 31, 2023
getdave
commented
Jan 31, 2023
getdave
commented
Jan 31, 2023
packages/block-editor/src/components/off-canvas-editor/block-contents.js
Outdated
Show resolved
Hide resolved
getdave
commented
Jan 31, 2023
packages/block-editor/src/components/use-block-drop-zone/index.js
Outdated
Show resolved
Hide resolved
|
Size Change: +1.99 kB (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
getdave
commented
Jan 31, 2023
packages/block-editor/src/components/use-on-block-drop/index.js
Outdated
Show resolved
Hide resolved
|
If this is a 👍 direction wise, my next step will be unit tests and resulting fixes. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
getdave
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
getdave
force-pushed
the
fix/disallow-drag-drop-into-editor-canvas-from-nav-offcanvas
branch
from
74848af
to
eee42f2
Compare
|
I've been thinking about this PR. Instead of defining 1:1 compatbility between drag origin and drop zone should we instead have some kind of whitelist on the draggable to say which dropzones it is compatible with? Imagine you wanted to be able to define Draggable X thus:
With the current implementation that is not possible. Currently all you can do is tell a given Draggable that it is only allow to drag within itself. That doesn't seem very flexible. Options:
I took a look at some Drag and Drop libraries and they seem to favour the first approach: |
I like the idea. Maybe no list means compatible with everything? The browser drag and drop API is pretty limited so you may have to figure out if its possible. |
Good to know - thank you.
💯 I agree. That's basically how it has to work to avoid backwards compatibility problems. |
@talldan I went with this approach in the end. The new API is:
Why?Because with our use case to retain existing editor behaviour, allowing dropzones to define which draggables they accept would require adding a types whitelist to all existing dropzones. So for example, say I wanted to stop Draggables being moved from the
As a result, I've gone for allowing Draggables to define which dropzones they are compatible with. This means I can simply:
This results in the Nav block list view nodes being droppable within the Nav block list view only. All existing functionality for other drag and drop is retained "as is". I did consider going for a deny list on the dropzones but I wasn't sure about that one. Let me know if you think that could be a better option. |
getdave
added
the
Backport to WP 6.7 Beta/RC
scruffian
reviewed
Feb 3, 2023
| @@ -92,7 +92,8 @@ function OffCanvasEditor( | |||
|
|
|||
| const [ expandedState, setExpandedState ] = useReducer( expanded, {} ); | |||
|
|
|||
| const { ref: dropZoneRef, target: blockDropTarget } = useListViewDropZone(); | |||
| const { ref: dropZoneRef, target: blockDropTarget } = | |||
There was a problem hiding this comment.
Can we make offcanvas a prop that we pass to the component, so that when we combine this with the List View we already have the tools we need?
|
This is looking good to me. Should we add/update some documentation? I'm thinking that in the future people might not understand why their drag/drop operations aren't working so it might be good to explain how the system works. |
Yes 💯 . Which documentation did you have in mind? |
|
|
I'm not fully confident in this implementation. Pinging @youknowriad for his opinion on:
|
|
One thing I just thought is we need to check this hasn't broken file/image drag and drop behaviour. |
|
I think we have a "drag type" in draggables/droppables for this kind of use-cases no? Can we use a dedicated drag type for the list view? |
From what I understand, it uses
There might not be many other ways. Using a data-attribute on the draggable is the only other idea I have, but I don't how feasible that is. |
|
I like this "targets" implementation. I think @talldan has a point that if we highlight the dropzones but then the drop does not work it looks like a bug not like a feature. |
draganescu
reviewed
Feb 7, 2023
|
|
||
| const draggedBlockClientIds = getDraggedBlockClientIds(); | ||
| const throttled = useThrottle( | ||
| useCallback( | ||
| ( event, currentTarget ) => { | ||
| const draggedBlocksTargets = getDraggedBlocksTargets(); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
shouldn't this check be in useDropZone so it applies to all drop zones?
draganescu
reviewed
Feb 7, 2023
| const { | ||
| getBlockRootClientId, | ||
| getBlockIndex, | ||
| getBlockCount, | ||
| getDraggedBlockClientIds, | ||
| canInsertBlocks, | ||
| } = useSelect( blockEditorStore ); | ||
| getDraggedBlocksTargets, | ||
| } = unlock( useSelect( blockEditorStore ) ); | ||
| const [ target, setTarget ] = useState(); | ||
| const { rootClientId: targetRootClientId, blockIndex: targetBlockIndex } = |
There was a problem hiding this comment.
it's a bit confusing the difference between the target variable this hook returns and the target finding done later.
|
This will not be merged in time for 6.2. There are too many changes to how the D&D works under the hood to be safe at this point. |
|
It's not currently clear how to proceed here. My suggestion is that we focus on unifying the OffCanvasEditor with the ListView, and then reconsider when we can see how these components work together. |
scruffian
added
the
[Status] Blocked
Mamaduka
removed
the
Backport to WP 6.7 Beta/RC
|
I know @andrewserong has been working on D&D so I'll close this one out now. |
youknowriad
deleted the
fix/disallow-drag-drop-into-editor-canvas-from-nav-offcanvas
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Disables dragging a block from the Navigation block list view into the editor canvas (or any other context).
Closes #46994
Why?
As shown in #46994 it's possible to drag any block in the Nav block list view into the editor canvas and it will be removed from the Nav block and end up in the editor.
This behaviour is unexpected in this context and should be disallowed.
Note: it has been confirmed that this behaviour is expected in the main List View 🤷
How?
Allows Draggables to define a list of DropZone
targetswith which they are compatible.Allows Dropzones to define a name/identifier for themselves.
When an event occurs over a given Dropzone, it will check the given Draggable for targets. If they exist then it will check for its own name within the Draggables
targets. Only if found will it accept the Draggable. Otherwise the drop will be rejected.Note that it has been necessary to introduce new state to track the drag targets. Why? Because the browser's drag event's
dataTransferis read only during thedragover. Apparently this is by design. As a result we need to store informartion about the dragOrigin in state in order that we can read it during thedragoverevent and avoid showing UI affordances indicating a drop is possible (when it isn't).Currently the drag "chip" (the black icon that follows your cursor as you drag) has a "positive" icon as you drag. It would be ideal if we were able to make that a "X" during the drag if the dropzone is not compatible. However as it's defined on the draggable there's not currently an easy way to do that without adding more state. Therefore this can be tackled in a follow up.
Testing Instructions
The key here is ensuring:
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-02-02.at.11-01-24.mp4