canDrop = false not working on tree root element

[buzzie-bee]

I am trying to implement a 'tree' style drag and drop interface which will have different types of elements. For logical reasons, certain element types can only exist as children of other element types.

I have tried to implement a basic example using the 'section' and 'field' types. sections can be children of the root or of other sections, fields can only be children of sections.

I have added the dropTargetForElements adaptor to the root element, and configured it's 'canDrop' to only return true for 'section' type items, but 'field' type items can still be successfully dropped there.

I have slightly adjusted the tree example to include the type on the TreeItem, and to only return canDrop: true on the root element if the item.type === 'section'.

If you check the console.logs, the 'section' items return canDrop: true, and the field items return canDrop: false, yet the field items can still be dropped.

https://codesandbox.io/p/sandbox/tree-forked-83sfc7?file=%2Fexample.tsx%3A251%2C26

I assume this is a bug, as this behaviour does not match the typedoc for canDrop:

Used to conditionally block dropping. By default a drop target can be dropped on.

Return false if you want to block a drop.

If this is not a bug and is the intended behaviour of canDrop on the root elements, is there another way to achieve my goal?

[alexreardon]

canDrop only blocks dragging on the current drop target, and will not block dropping on child or parent drop targets. If you want to block dragging on all children drop targets too, they too will need to return false from their canDrop(). Hopefully this understanding unblocks you. If you are leveraging something like react context, then you could pass a boolean down to all children (or pass down function that returns whether dragging is allowed).

---

I think we could improve the clarity of our documentation on this point.

Here is our current guidance about canDrop()

canDrop?: (args: GetFeedbackArgs) => boolean is used to conditionally block dropping. When looking for valid drop targets, @atlaskit/pragmatic-drag-and-drop starts at the deepest part of the DOM tree the user is currently over and searches upwards for valid targets. If a drop target blocks dragging (canDrop() returns false), then that drop target is ignored and the search upwards continues. canDrop() is called repeatedly while a drop target is being dragged over to allow you to dynamically change your mind as to whether a drop target can be dropped on. canDrop() being called repeatedly allows you to change your mind about whether a drop target can be dropped on after it has been entered into. This could be helpful in a situation where you are waiting on some permission information from a backend service.

• [ ] I think we could make these docs even clearer that a drop target blocking dropping with canDrop() does not impact the ability of any child / parent drop target to accept a drop
• [ ] Improve clarity of jsdoc

[alexreardon]

Work in progress new docs:

canDrop?: (args: GetFeedbackArgs) => boolean is used to conditionally block dropping on a drop target. Returning false from canDrop() will not block dropping on parent or child drop targets. All drop targets that want to not allow dropping, need to return false from their canDrop() function.

When looking for valid drop targets, a lookup starts from the deepest part of the DOM tree the user is currently over and searches upwards for valid targets. If a drop target blocks dragging (canDrop() returns false), then that drop target is ignored and the search upwards continues.

canDrop() is called repeatedly while a drop target is being dragged over to allow you to dynamically change your mind as to whether a drop target can be dropped on (including after it has already been entered into). This could be helpful in a situation where you are waiting on some permission information from a backend service.

Work in progress jsdoc

/**
   * Used to conditionally block dropping.
   * By default a drop target can be dropped on.
   *
   * Return `false` if you want to block a drop.
   * 
+  * Blocking dropping on a drop target will not block
+  * dropping on child or parent drop targets.
+  * If you want child or parent drop targets to block dropping,
+  * then they will need to return `false` from their `canDrop()`
   *
   * `canDrop()` is called _repeatedly_ while a drop target
   * is being dragged over to allow you to dynamically
   * change your mind as to whether a drop target can be
   * dropped 

Do you think that clears things up @buzzie-bee?

[buzzie-bee]

@alexreardon the jsdoc changes certainly help with understanding how the can drop check searches upwards so I think it's a worthwhile change.

I'm still confused though as to why I can drop a 'field' type at the root level, when the root 'canDrop' is returning false.

Is it because the canDrop of the first 'section' is being triggered and returning true, even though the instruction is 'reorder-above' which implies it's being dropped on the parent of the 'section' which is the root?

I would think that a 'reorder-above' instruction would check the canDrop of the parent of the target element for that instruction.

Edit: Your comment definitely helped me figure out what exactly was going on and get a fix working.

I added this to the items dropTargetForElements.getData function:

if(item.type === 'section' && source?.data?.type === 'field' && level === 0) {
  block.push('reorder-above', 'reorder-below');
}

Which means that the section items will block 'reorder-above/below' when they are at the root level. I still feel that the 'reorder-above' instruction not checking the canDrop of the parent is potentially problematic behaviour.

[alexreardon]

@alexreardon the jsdoc changes certainly help with understanding how the can drop check searches upwards so I think it's a worthwhile change.

👍. I'll ship them asap.

I'm still confused though as to why I can drop a 'field' type at the root level, when the root 'canDrop' is returning false.

I have been thinking about this. It think it does make sense for most cases that a disabled parent also disables all children (eg like how a disabled <fieldset> element will also disable children). However, it is nice to have the flexibility for each drop target to be able to decide whether it should be enabled or not.

A few potential options:

• Make it so that a disabled parent always disables all children
• Make it so a disabled parent disables children, unless a child states it wants to be disabled. This is problematic as the default canDrop() behaviour is true. I don't think we should distinguish between explicit canDrop: () => true and a default canDrop: () => true as that adds extra hidden meaning to the API
• Keep the current behaviour (most flexible, but perhaps a bit of friction in that folks might think that a disabled parent should disable children).

I want to think about this more and gather more feedback. Let's move this to a Github "discussion" #45.

---

I think the tree case of "reorder-above" is a special case. "reorder-above" is simply a string returned from the tree hitbox.

If you want to not allow "reorder-above" you have some options:

• use the block argument for attachInstruction
• locally remap the string "reorder-above" to "make-child"
• use your own hitbox function (ours is not heaps of code) which more accurately reflects your tree.

---

I'll close this issue for now and we can continue the broader discussion on #45.

Thanks @buzzie-bee

[alexreardon]

We are currently merging these docs and jsdoc improvements. They should be released within ~12 hours