isaacHagoel / svelte-dnd-action

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

Inconsistencies on dropping zones (Cannot read property 'getBoundingClientRect' of undefined ) #280

Closed IoannisLafiotis closed 3 years ago

IoannisLafiotis commented 3 years ago
Bildschirmfoto 2021-04-22 um 19 57 40

After taking a lot of time checking the library I can not understand why there are these inconsistencies. I would really like to help resolve this isssue not only for me but for other people using the lib as well.

Bildschirmfoto 2021-04-22 um 20 02 36
isaacHagoel commented 3 years ago

Please provide a REPL that reproduces the problem and i will have a look. Are you updating the items array in the event handlers?

On Fri, Apr 23, 2021, 04:03 Ioannis Lafiotis @.***> wrote:

[image: Bildschirmfoto 2021-04-22 um 19 57 40] https://user-images.githubusercontent.com/46591718/115763836-288a0f80-a3a5-11eb-97ba-c32ea9eca35c.png

After taking a lot of time checking the library I can not understand why there are these inconsistencies. I would really like to help resolve this isssue not only for me but for other people using the lib as well.

[image: Bildschirmfoto 2021-04-22 um 20 02 36] https://user-images.githubusercontent.com/46591718/115764316-b49c3700-a3a5-11eb-9d84-774a28d68a24.png

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC7BSPND5A67A5S7IGDTKBQGTANCNFSM43NAEPNQ .

IoannisLafiotis commented 3 years ago

It will take me some time to reproduce the repl. It is quite similar though to the example that already exists there. Left side dynamically created cards that only can take one item inside and the previous is popped out (that is why I remove the consider functionilty on them). Right side a ul with items that I drag and drop to the cards on the left! When I hang on to the items for too long over different cards then I get almost always the error!

isaacHagoel commented 3 years ago

Many others (including myself) are using the library with no such issues so it is more likely that the issue is in your code (maybe the readme or examples misled you and should be improved). I do agree that adding defensive code with clearer error messages is a good idea. In any case, without you providing an example that reproduces the issue i have nothing to debug for you.

On Fri, Apr 23, 2021, 07:18 Ioannis Lafiotis @.***> wrote:

It will take me some time to reproduce the repl. It is quite similar though to the example that already exists there. Left side dynamically created cards that only can take one item inside and the previous is popped out (that is why I remove the consider functionilty on them). Right side a ul with items that I drag and drop to the cards on the left! When I hang on to the items for too long over different cards then I get almost always the error!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/280#issuecomment-825191716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC2W7XVGETGKLGSCFR3TKCHAZANCNFSM43NAEPNQ .

IoannisLafiotis commented 3 years ago

Let me get back to you tomorrow when I go to the office. I will let you know If I can not find the solution!

isaacHagoel commented 3 years ago

Cool. Would be good if u let me know either way so i gain some insight into common gatchas and can improve the docs and/ or error messages

On Fri, Apr 23, 2021, 07:29 Ioannis Lafiotis @.***> wrote:

Let me get back to you tomorrow when I go to the office. I will let you know If I can not find the solution!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/280#issuecomment-825198133, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC6KAPQKQWUHBNA4WEDTKCILTANCNFSM43NAEPNQ .

quentin-fox commented 3 years ago

I'm having a similar issue when dropping, in my case there are a lot of drag zones. This is the error message generated in Safari 14.0:

image

And the error message generated in Firefox 88.0

image

For reference, I'm trying to use this library to recreate scrabble in svelte. The rack is one single dndzone, and each square on the board is its own dndzone.

I'll try to do a minimal recreation and upload the REPP later today, this is quite blocking for me on this side project. I assume that there's some sort of gotcha that we're running into, would be great to sort it out!

quentin-fox commented 3 years ago

Here's a link to the REPL - I've been able to replicate a few issues - sometimes, the tile just fully disappears when you drag it onto a square, and other times (especially when there are a few tiles placed on the board already, and you drop it in-between two board squares), it'll get stuck, rather than snapping to a DND zone, and it makes all other draggable items unresponsive to further drag and drop motions.

https://svelte.dev/repl/06758d1dc5c54416957b2a9baf386bfb?version=3

Hopefully it's something I've just done wrong or some anti pattern that causes an error!

isaacHagoel commented 3 years ago

@quentin-fox okay so good news and bad news. The good news is that there was an issue with your code. The Square didn't handle the consider event at all, which does break the library. I have fixed it (and the logic to decide on whether there is a tile) and there are no exceptions. https://svelte.dev/repl/ed2e138417094281be6db1aef23d7859?version=3.37.0

The bad news: there does seem to be a bug in the library, the draggedLeft event is not fired for the squares, resulting in them disappearing as you drag a letter over them (the shadow item is being left behind, hiding the square because of the missing event). I will need to debug it further. hopefully later today.

P.S: this is a very interesting usecase (scrabble) - first time I see someone attempting it :)

isaacHagoel commented 3 years ago

(cont) note to self: it might have to do with the dnd zone in square not having a #each block which is the normal pattern for using the lib

quentin-fox commented 3 years ago

Thanks for looking into it! I also ran into the bug in my local codebase where it was causing the squares to disappear as the letter was dragged over them, so I think that's why I removed the on:consider, but evidently that caused other problems.

I also had the same hunch with the #each block, so I slightly changed the code in Square.svelte so that the tiles are rendered in an each block - this shouldn't lead to any UI bugs with multiple tiles in each square, as the code prevents there from ever being more than 1 item in the items array.

https://svelte.dev/repl/aceee6a84842496e9723b28f7eef2cd7?version=3.37.0

However, that didn't seem to solve the issue we're running into, at least not in terms of UI.

And thanks! The facebook scrabble app that my family used for years and years got discontinued, and the replacement is riddled with micro transactions and other gimmicks, so as the family software dev I was tasked with building a new one. It's been a really interesting project so far!

isaacHagoel commented 3 years ago

@quentin-fox I fixed it. See this REPL.

As the README states, the library assume a 1:1 relationship between the items that are passed in and the children of the dndzone in the DOM. The conditionally rendered "square" element was a foreign child that was breaking this assumption.

In addition I fixed some other small issues. Most notably the nested styles weren't copied from the shadow item to the dragged element when dragging from the board to the rack. There are two ways to address it: disable morph (which i did) or placing the styles/class on the direct child of the dnd zone (line 50 in App.svelte).

i think i should add clear error messages when:

  1. the number of items and the number of children don't match
  2. there are missing event listeners on the dnd zone (consider or finalize) thoughts?
daogilvie commented 3 years ago

Funnily enough I'm attempting exactly the same thing as @quentin-fox for very similar reasons. I was running into the same issue and came here to see what I was doing wrong! It's great to find a fix already — thank you for making such an excellent library @isaacHagoel, it's fantastic. I'm new to svelte, and finding it pretty darn neat; I've already seen some things I could do svelte-ier from looking at your REPLs (thank you 👍 ), and now I have the drag-and-drop of tiles working on the board itself.

i think i should add clear error messages when:

  1. the number of items and the number of children don't match
  2. there are missing event listeners on the dnd zone (consider or finalize) thoughts?

I think case 1 would have made it clearer to me what was happening earlier, that definitely gets my vote.

TL;DR thanks very much, I'm really enjoying using this code

quentin-fox commented 3 years ago

@isaacHagoel Amazing! Really appreciate it.

I think those error messages would be really great additions - it seems like they're obviously hard requirements for the library to work, so failing loudly seems ideal.

Unless I'm missing it, the 1:1 relationship isn't stated as such in the README? In the "Rules/ assumptions to keep in mind" section, the closest line I could find was this:

The items in the list that is passed-in are in the same order as the children of the container (i.e the items are rendered in an #each block).

If this is the line you meant, it might be worth clarifying or adding a new point (potentially with a small example) explaining that's it's not just about the "same order", but about the 1:1 relationship between items/DOM elements as well, and that conditional rendering breaks this assumption.

EDIT: We could also add the most recent REPL you sent to the list of examples in the README?

IoannisLafiotis commented 3 years ago

Nice to see that other people had similar issues. Anyway the bugs are fixed and we can use the lib. @isaacHagoel I will let you know if there are more issues like this.

isaacHagoel commented 3 years ago

@daogilvie thanks! @quentin-fox fair enough. it needs to be stated more explicitly. I've created #281 to track the needed improvements to the REDME/ error messages. @IoannisLafiotis the fix was to the REPL not the library (not that the library can't have bugs). We both have @quentin-fox to thank for taking the time to create that REPL that got us all on the same page. Thanks for reporting the issue.

IoannisLafiotis commented 3 years ago

https://svelte.dev/repl/ed2e138417094281be6db1aef23d7859?version=3.37.0

I trimmed down your Repl and managed to reproduce the issue that I had.

Bildschirmfoto 2021-04-28 um 14 39 13

isaacHagoel commented 3 years ago

@IoannisLafiotis the link you provided seems to be pointing to my REPL that is working fine. Link to the REPL with the issue please, assuming you didn't trim the handlers or anything we know to be essential. thx

weberjm commented 3 years ago

Thanks @isaacHagoel for your help here, and your work on the library. I think that @IoannisLafiotis and our team worked out most of the problems we were getting. I did want to mention that we do still get some problems with shadow elements when we drag around between multiple dndzones. And I saw that the same behavior is occuring in the latest REPL you shared above.

If you drag a tile around the board, when you drop it, there are sometimes shadow elements left behind that are not removed on drop. Possibly it is trying to process too many exit events from all of the separate zones? This does not cause errors in the Scrabble scenario (although it does cause tiles to not be able to be dropped on squares which still have a shadow element), but was causing us problems when we were trying to work with the data in each item list.

isaacHagoel commented 3 years ago

Which repl and which browser? It doesn't reproduce for me on my latest

On Sat, May 1, 2021, 06:10 James @.***> wrote:

Thanks @isaacHagoel https://github.com/isaacHagoel for your help here, and your work on the library. I think that @IoannisLafiotis https://github.com/IoannisLafiotis and our team worked out most of the problems we were getting. I did want to mention that we do still get some problems with shadow elements when we drag around between multiple dndzones. And I saw that the same behavior is occuring in the latest REPL you shared above.

If you drag a tile around the board, when you drop it, there are sometimes shadow elements left behind that are not removed on drop. Possibly it is trying to process too many exit events from all of the separate zones? This does not cause errors in the Scrabble scenario (although it does cause tiles to not be able to be dropped on squares which still have a shadow element), but was causing us problems when we were trying to work with the data in each item list.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/280#issuecomment-830353149, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC4VR6IYPKWWIQW6EUDTLMFB3ANCNFSM43NAEPNQ .

weberjm commented 3 years ago

I have been able to reproduce on both Firefox and Vivaldi (Chromium), using this link which you shared in this comment.

To reproduce, I simply drag (for example) Tile B onto a square. I then grab the same tile and move it around the board and drop on another square. This results, using Dev Tools, in multiple shadow items remaining on some of the squares the dragged item passed over. These shadow items prohibit other Tiles from later being dropped on those squares. Would you like me to record a short video or GIF of this?

isaacHagoel commented 3 years ago

I was able to reproduce it. interesting. Maybe it is a race condition that has to do with the dndzones being so small and in such close proximity to one another (which allows swooshing over them real quick). I will check.

On Mon, May 3, 2021 at 5:21 AM James @.***> wrote:

I have been able to reproduce on both Firefox and Vivaldi (Chromium), using this link https://svelte.dev/repl/ed2e138417094281be6db1aef23d7859?version=3.37.0 which you shared in this comment https://github.com/isaacHagoel/svelte-dnd-action/issues/280#issuecomment-826268173 .

To reproduce, I simply drag (for example) Tile B onto a square. I then grab the same tile and move it around the board and drop on another square. This results, using Dev Tools, in multiple shadow items remaining on some of the squares the dragged item passed over. These shadow items prohibit other Tiles from later being dropped on those squares. Would you like me to record a short video or GIF of this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/isaacHagoel/svelte-dnd-action/issues/280#issuecomment-830858229, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE4OZC3B5BEWTX3KG272HR3TLWQ4LANCNFSM43NAEPNQ .

isaacHagoel commented 3 years ago

@weberjm @IoannisLafiotis please try the REPL again. I enhanced it to show where the shadow element is at all times, added a workaround for the issue you were facing and created issue #289 that will address it permanently (should be a tiny fix)

isaacHagoel commented 3 years ago

Okay. #289 is fixed now and the scrabble example works perfectly with no workarounds as far as I can tell. Closing this issue. Feel free to reopen if I missed something. Thanks for all of your input and examples.