search

sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
77.94k stars 4.07k forks source link

Svelte 5: Reordering array with dnd-action causes flickering #10115

Closed Freeskier closed 7 months ago

Freeskier commented 7 months ago

Describe the bug

When dragging an element and changing its position the dragged element (the shadow element in the list) flickers. I assume its a $state rune problem, because in Svelte 4 everything works fine.

Reproduction

Svelte4 Svelte5

Logs

No response

System Info

(Svelte v5.0.0-next.30)

Severity

annoyance

brunnerh commented 7 months ago

This is a flip issue, drag and drop is not required to run into this. Reordering example

FLIP works by first calculating the target layout, reverting layout and then transitioning to the target by calculating a delta animation. Currently a render can (but does not have to) happen while the DOM is in the final state which should not be the case.

Freeskier commented 7 months ago

@brunnerh is there any possibility for you to fix this issue in the near feature?

trueadm commented 7 months ago

I'm not even fully sure how to go about solving this one. Any help in debugging it and figuring out the correct fix would be appreciated. :)

brunnerh commented 7 months ago

@trueadm The main problem appears to be the use of setTimeout here: https://github.com/sveltejs/svelte/blob/fe8a9ce31d5fb662b3953b318621ea364992e014/packages/svelte/src/internal/client/runtime.js#L491-L497

The function is used by each_item_animate: https://github.com/sveltejs/svelte/blob/b3d185da298c83c4799d92d3e4f372e4a776bd34/packages/svelte/src/internal/client/transitions.js#L674-L676

setTimeout lets the event loop end and the browser potentially render. Changing this to something like queueMicrotask (or using a promise for backward compatibility like here) seems to fix the problem.

Though schedule_task is used in hydration. I don't know if the event loop has to be ended there or why setTimeout was used here in the first place.

Edit: You actually added that here, do you remember why this is done async and on the next loop?

trueadm commented 7 months ago

@brunnerh Nice digging. I'll make that change :)

brunnerh commented 7 months ago

@trueadm If it's safe to change the internal implementation, consider renaming the function to schedule_microtask/schedule_micro_task.