isaacHagoel / svelte-dnd-action

An action based drag and drop container for Svelte
MIT License
1.79k stars 104 forks source link

Registering a new drop zone while dragging an element #479

Open m4nnke opened 1 year ago

m4nnke commented 1 year ago

Hi, first of all great work with your library.

I have a use case, where I display the nodes of a simplified abstract syntax tree of a latex document using dropZones as inner Nodes of the graph. It's a LOT of nesting. Inner Nodes can be collapsed so parts of the tree aren't visible. Everything works quite smoothly :).

Now to the point: While dragging an element, a new dropzone is being mounted (its part of a collapsed inner node that opens after hovering over it). The problem is, that it's not possible to drop the dragged element in the newly created drop zone. (Probably because it is not yet registered in the typeToDropZones Map or something). Is there a way to register a new drop zone while dragging an element?

Thanks in advance

isaacHagoel commented 1 year ago

Hi, Do you mind making a simple REPL that illustrates the issue? Based on your description I would expect it to work (so maybe there's a bug)

On Tue, Aug 29, 2023, 23:34 m4nnke @.***> wrote:

Hi, first of all great work with your library.

I have a use case, where I display the nodes of a simplified abstract syntax tree of a latex document using dropZones as inner Nodes of the graph. It's a LOT of nesting. Inner Nodes can be collapsed so parts of the tree aren't visible. Everything works quite smoothly :).

Now to the point: While dragging an element, a new dropzone is being mounted (its part of a collapsed inner node that opens after hovering over it). The problem is, that it's not possible to drop the dragged element in the newly created drop zone. (Probably because it is not yet registered in the typeToDropZones Map or something). Is there a way to register a new drop zone while dragging an element?

Thanks in advance

— Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/479, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC4UWSORMFFHMTR4JNDXXXVVBANCNFSM6AAAAAA4C7BQYI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

m4nnke commented 1 year ago

Thanks for the fast reply. Maybe my description was not detailed enough. I hope this illustrates my problem: https://svelte.dev/repl/9af9c4f11c2f4d888438b8b29d9e6c52?version=4.2.0

isaacHagoel commented 1 year ago

Yeah I see the issue. You could probably work around it if you hide the collapsed zone with css rather than not rendering it. With that said, from looking at the code, what needs to be done in order to fix it is to modify the watch function to be a bit smarter (e.g get the list of zones as a parameter and default to what it's using now) and call it when registering a new zone if isWorkingOnPreviousDrag is true. If you have time to make a pull request it would be great, otherwise I can probably get to it over the next few days.

isaacHagoel commented 1 year ago

(cont) the key is to call observe on the new zone and add the event listeners. the event listener on the window shouldn't be added again.

isaacHagoel commented 1 year ago

@m4nnke i merged that PR but now that i am thinking about it I reckon that the opposite case is probably also an issue... as in if the user removes a zone mid drag and it unregisters, the lib won't stop observing it and I wonder if that would cause any errors.

m4nnke commented 1 year ago

Maybe this is linked with the potential bug I mentioned in #480 first message all the way down (occurs when removing the source zone on drag). You can try it out in the test repo I created. I'll think about that tomorrow. I'm from Europe, it's quiet late here ;)

isaacHagoel commented 1 year ago

@m4nnke thanks. Now it is very late for me (Australia) but looks like this change somehow introduced issue #482. I am not sure how (the problem i was worried about should exist in older versions as well so that's not it) I asked for more details and will give it a look tomorrow.

m4nnke commented 1 year ago

I had a similar issue earlier today. I tried my old implementation and it seems, my issue isn't showing up there.

m4nnke commented 1 year ago

@isaacHagoel I think the problem is the fact that rewatching the dragged element leads to reobservation, of the zones, wich leads to a reinitialisation of the variables inside the observe function. The next time the dragged element enters a dropzone things go wrong (I'm not sure what exactly). In my first impementation the observe loop keeps running the whole time so the variables stay in tact. A quick fix in the current implementation: making the 4 variables in the observe function global and cleaning up the variables after a drop has occurred. That seems to fix the issues I encountered. Maybe that gives you an idea what is wrong. I can't tell if it fixes #482.

m4nnke commented 1 year ago

The quickfix semi-solves #482, but my old implementation seems to solve #482 too.

isaacHagoel commented 1 year ago

@m4nnke I reverted the change for now (it broke the crazy nesting example it turns out) and re-opening the ticket after spending a long time re-diving into the relevant parts of the lib and old tickets such as #362 and the fix for it (scheduling the dz for removal after drop rather than removing in real time). I discovered that in nested scenarios the lower level dropzones (that are items in higher level zones) register and unregister as part of a normal drag operation at the parent level (because the shadow element is created etc). Initially I tried adding code to deal with that (which also means doing a better version of the fix to #362 because we need to also be able to remove dynamically "for real" otherwise nested scenarios break) but then some questions started creeping into my mind. If we allow adding and removing zones mid drag and expect the lib to adjust within the same drag operation it raises edge-case questions such as: What should happen if I remove the origin zone and then after a second add "it" back and then drop outside of any zone. Is it still the origin zone? In other words, I need some more time to think about it. In the meantime will hiding it with css when it is collapsed be a proper workaround for you?

m4nnke commented 1 year ago

Well, thanks for the effort, using css with display: none does fix the issue, sadly it slows down the startup, because all the elements need to get in the dom (in large documents there are > 500 nodes = draggable elements and up to 7 nestng levels). The leafs of the tree are mostly iframes with latexjs library inside. Display:none seems to break my latexjs inside the iframes. I probably take the first implementation of the issue and use that combined with not being able to close elements while dragging. That seems to work for me.

isaacHagoel commented 1 year ago

I see. Alright. I'll keep you posted

isaacHagoel commented 1 year ago

Just a short update. I am working on a PR that would address some of the core issues that makes this difficult. Specifically it adds much tighter control over nested zones life-cycle and eliminates the noise they currently create by registering and being destroyed as the parent element is dragged. You can watch #483 to stay in the loop about that. After that's merge I will re-examine supporting dynamically added and removed zones. Notes to self: