Skip to content
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

Fixes wrongful element decoration #600

Closed
wants to merge 1 commit into from
Closed

Fixes wrongful element decoration #600

wants to merge 1 commit into from

Conversation

vini-brito
Copy link

@vini-brito vini-brito commented Sep 13, 2024

This fixes #596, where the wrong element in the list is decorated as a shadow element.

What happens is that the shadow element is not yet present when a decoration is triggered due to, for some still unknown reason, the splice at line 159 items.splice(shadowElIdx, 0, shadowElData); not running.

That splice is responsible for adding to the list of items a copy with the following properties:

"id": "id:dnd-shadow-placeholder-0000",
"isDndShadowItem": true 

Specifically, the ID is modified to that value and the isDndShadowItem property is added.
When that shadow copy is missing, we have one less item in the list and when the decorateShadowEl(draggableEl) function runs, it hits the correct index position but in that index position the wrong HTML element will be.

Immediately after, this internal routine runs again, properly adds the shadow copy, then hits the correct element.
The thing is, in a healthy drag, the internal routine runs just once, in the problematic version it runs twice, first without adding the shadow copy and automatically it triggers again and properly adds the shadow copy.

Here is this quick video I first show a healthy drag, then a problematic drag:

Screencast.from.12-09-2024.21.04.17.webm

This fix is inelegant because it doesn't get to the root cause of the issue, but apparently it works, it uses a side effect that I noticed, in the first, problematic internal routine run, one of the items object will have the ID as "id": "id:dnd-shadow-placeholder-0000", and in a healthy internal routine run, the ID of the item will be the correct ID as defined by the app, so the logic here is to check if one of the items has the shadow ID, which is indicative of a problematic run, and then not to decorate the element.

Ideally, we should figure out why the problematic run happens, so any suggestion on this is appreciated.

I had a couple ideas for fixes:
1 - Undecorate the element after a problematic run.
2 - The one suggested in this PR, which is to not decorate if the side effect is noticed.

But of course I would prefer to do 3:

3 - Eventually reach the source of this problem and not even have the problematic run.

I have tons of logs I can share that allowed me to arrive at this conclusion and solution.
Example of my logs about the items, using console log stringify items, right before the decoration, showing a problematic run where this runs twice:

items right before decoration:
{
  "content": "Text node",
  "id": "id:dnd-shadow-placeholder-0000",
  "isDndShadowItem": true
}

{
  "content": "Text node",
  "id": "text_dOHCW0NJRU8-mqerMESzgfGCw"
}

{
  "content": "Text node",
  "id": "text_xSZJVOxc9NthNP68l2rxVP93f"
}

------ Another automatic run, this time healthy ---------

items right before decoration:
{
  "content": "Text node",
  "id": "text_AbuZBBG2dZiHu6tICxBV3YIFN",
  "isDndShadowItem": true
}

{
  "content": "Text node",
  "id": "text_dOHCW0NJRU8-mqerMESzgfGCw"
}

{
  "content": "Text node",
  "id": "text_xSZJVOxc9NthNP68l2rxVP93f"
}

Also, this bug was introduced in the version 42. Before that however, the last working version for my setup was 28.
From 29 to 41, all of them freeze when I try to drop with a "getBoudingEtc..." error.
In my setup this change doesn't seem to interfere with anything else, but I'm not sure about the wide range of use cases out there.

If it is better to discuss or further work on this before merging I'm open to it!

@isaacHagoel
Copy link
Owner

Thanks for this.
Can we reproduce this issue using a minimal REPL? I'd like to try to find the root cause

@vini-brito
Copy link
Author

Sure, here is the minimal example, just one file, I removed everything I could:

https://svelte.dev/repl/3620096a09e74af698c2633fdf5b6d00?version=4.2.18

@isaacHagoel
Copy link
Owner

remind me why we can't just remove the filtering of the shadow item like I did in this REPL?
https://svelte.dev/repl/42158c219b174627980f8f6a3f0d174b?version=4.2.18

@vini-brito
Copy link
Author

It breaks nested containers. Also applying the filter is the recommendation in the docs, when I tested without, nested stuff broke.

@isaacHagoel
Copy link
Owner

isaacHagoel commented Sep 16, 2024 via email

@vini-brito
Copy link
Author

vini-brito commented Oct 4, 2024

I removed the filter and made several changes in userland lately and apparently this issue is gone, so the PR is unnecessary. However I cant isolate what change solved it, else I'd share it.

@vini-brito vini-brito closed this Oct 4, 2024
@isaacHagoel
Copy link
Owner

isaacHagoel commented Oct 4, 2024 via email

@vini-brito vini-brito deleted the fix/fix-odd-shadow-decoration branch October 15, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not removing 'data-is-dnd-shadow-item-internal="true"' property so elements get hidden permanently
2 participants