isaacHagoel / svelte-dnd-action

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

Moving very fast result in lags on Edge (chromium) version 113.0.1774.42 #451

Closed Raphael-Bahuau closed 1 year ago

Raphael-Bahuau commented 1 year ago

Hello, great work for your lib it's awesome !

I don't know if it's my PC, but when I use the lib, even on the simplest REPL, the lib seem's lagging when I shake an item fast. It's lagging like 3 seconds after I start to shake.

I don't know how to show you this behavior tho. I wonder if there is not too much event fired ? To much calculations ? Maybe there is a workaround ? At first I was blaming the flip animation from svelte, but the lag appear even without animations.

I'm sure the technical improvement can be done, cause this behavior don't appear on the Draggable lib from Shopify. Also I came from React and I was using Dnd-Kit, and this was well optimised too.

Thoses lib seems to use translate3d style to make the moves, I wonder if we can implement this on your lib ? Maybe instead of letting svelte do it's flip animation magic, whe can provide a custom css application on your consider and finalize functions ?

Let me know if you have any informations about this. thanks.

(sorry for my aproximative sentences, I'm a French developper).

isaacHagoel commented 1 year ago

Hi @Raphael-Bahuau, Thanks for reaching out. Maybe include a video with the issue you're seeing as it doesn't reproduce for me (or maybe I don't understand what you are doing). In this REPL there is no flip but in general:

  1. The lib does its calculation (for whether the element is a zone etc) in every fix interval that's determined by the flip duration (slower flip, longer interval)
  2. The dragged element is moved using translate3D using a standard mouseMove listener, maybe throttling/debouncing it will resolve you issue (though it adds its own calculations so probably not worth it). See here
  3. Svelte flip animates the items as they swap places within the list.
  4. The lib animates the element back to place if flipDurationMS is larger than zero
Raphael-Bahuau commented 1 year ago

``Hi, thanks for the fast response ! Maybe I didn't explain well, the lag isn't about the dragged element but about the element on the list. I will make a video or we can have a visio if you like (on Discord or whatever).

I've make some test, I feel the problem isn't with the lib computation but rather caused by the Svelte #each block processing.

Edit: there is the vidéo https://youtu.be/StjPtzZgWzQ

The code is like the basic REPL with flip animation equal to 100.

<script lang="ts">
    import { dndzone } from 'svelte-dnd-action';
    import { flip } from 'svelte/animate';

    import { zones } from '$lib/store';

    import type { ZONE } from '$models/Deck';

    export let zone: ZONE;
    export let columns: number;
    export let rows: number;
    export let flex: number;

    let items = $zones[zone].map((id) => ({ id }));

    const flipDurationMs = 100;
    function handleSort(e: CustomEvent<DndEvent<{ id: string }>>) {
        items = e.detail.items;
    }
</script>

<div
    use:dndzone={{ items, flipDurationMs }}
    on:consider={handleSort}
    on:finalize={handleSort}
    class="deck-container"
    style:grid-template-columns="repeat({columns}, 1fr)"
    style:grid-template-rows="repeat({rows}, 1fr)"
    style:flex
>
    {#each items as item (item.id)}
        <div class="draggable-card" data-draggableid={item.id} animate:flip={{ duration: flipDurationMs }}>
            {item.id}
        </div>
    {/each}
</div>

<style>
    .deck-container {
        display: grid;
        border: 2px #ccc dashed;
        padding: 0.5rem;
        gap: 0.5rem;
    }
    .draggable-card {
        background-color: rgb(195, 157, 231);
        border-radius: 8px;
        border: 2px solid transparent;
        padding: 0.5rem;
        line-height: 0.8;
        display: flex;
        justify-content: center;
        align-items: center;
    }
</style>
isaacHagoel commented 1 year ago

Thanks. The video really helps. It might actually be a library issue. Maybe there's a memory leak somewhere or something. I tried to paste the code you provided into a REPL but it complains about import type { ZONE } from '$models/Deck';

I remembered you said this happens in the simple REPL too. I played with it a bunch but it doesn't seem like it reproduces for me. Attaching a screen recording. I am happy to investigate this but need a way to reproduce it. I will try to make some time tomorrow to try to make a big example locally and see if I can make the lag happen.

https://github.com/isaacHagoel/svelte-dnd-action/assets/20507787/7c936d62-74d3-45dc-8388-3512cf1472db

Raphael-Bahuau commented 1 year ago

I don't know why I can't save the REPL (the Log in to save button open the connexion page for like 1 second before closing it...)

There is a working code for a REPL.

<script>
    import { dndzone } from 'svelte-dnd-action';
    import { flip } from 'svelte/animate';

    let items = [
        { id: 0 },
        { id: 1 },
        { id: 2 },
        { id: 3 },
        { id: 4 },
        { id: 5 },
        { id: 6 },
        { id: 7 },
        { id: 8 },
        { id: 9 },
        { id: 10 },
        { id: 11 },
        { id: 12 },
        { id: 13 },
        { id: 14 },
    ]

    const flipDurationMs = 100;
    function handleSort(e) {
        items = e.detail.items;
    }
</script>

<div
    use:dndzone={{ items, flipDurationMs }}
    on:consider={handleSort}
    on:finalize={handleSort}
    class="deck-container"
>
    {#each items as item (item.id)}
        <div class="draggable-card" data-draggableid={item.id} animate:flip={{ duration: flipDurationMs }}>
            {item.id}
        </div>
    {/each}
</div>

<style>
    .deck-container {
        height: 90vh;
        display: grid;
        grid-template-columns: repeat(10, 1fr);
        grid-template-rows: repeat(6, 1fr);
        border: 2px #ccc dashed;
        padding: 0.5rem;
        gap: 0.5rem;
    }
    .draggable-card {
        background-color: rgb(195, 157, 231);
        border-radius: 8px;
        border: 2px solid transparent;
        padding: 0.5rem;
        line-height: 0.8;
        display: flex;
        justify-content: center;
        align-items: center;
    }
</style>
isaacHagoel commented 1 year ago

Thanks. Does it reproduce for you on this REPL? It seems to work okay for me which browser, OS, computer are you using?

Raphael-Bahuau commented 1 year ago

There is a video (you respond quicker than the online compressor ahah!)

I'm on Windows 11, Edge (chromium), I didn't try with Chrome tho. With a pretty good computer, like 2000€ beast (I don't think the config matters since other libs works good).

https://github.com/isaacHagoel/svelte-dnd-action/assets/44974825/caa9c20d-4f17-450d-862c-f9697a180aac

Raphael-Bahuau commented 1 year ago

Wow, I've tried on Chrome, there is no lag at all... So apparently there is a bug with this lib on Edge Chromium.

isaacHagoel commented 1 year ago

Edge on Mac works fine too. I just tried. I have version 112.0.1722.48 (Official build) (x86_64) Which one do you run?

Raphael-Bahuau commented 1 year ago

I'm on the 113.0.1774.42 official too (on windows 11 I guess).

I've already tried to search if there is a memory leak with the Memory debug console, but I don't know how to do it properly, I don't understand it quite well...

isaacHagoel commented 1 year ago

Alright. I upgraded my edge to latest (113.0.1774.42) and yey it reproduces. Looks like this version of the browser doesn't like something either the lib or svelte is doing. I will make time to dig deeper into this tomorrow (I am in Australia so it's late evening here).

Raphael-Bahuau commented 1 year ago

Yes it's seems a deeper problem than the lib itself. But now I know this is from the browser or svelte, I can continue to dev, we will fix it sometime.

Anyway thank for the conversation (it's 13h here in France), let me know if you find something about it later !

isaacHagoel commented 1 year ago

Yeah. will do.

isaacHagoel commented 1 year ago

Alright, so I investigated a bit. Firstly, it is not a memory leak as can be seen here:

Screenshot 2023-05-19 at 4 53 24 pm

When I activated debug logging I could see that updates start coming in "slow motion". I tried to run this in the console.

let counter=0, flag = true;
function run(interval=100) {
    if (!flag) return;
    console.log("run", counter, Date.now());
    counter+=interval;
    window.setTimeout(run, interval);
}

When ran after the slowness started (but without the lib or svelte doing anything) it was running every second instead of every 100ms. After a browser refresh it ran in the expected cadence at first, but I killed it (by running flag = false) waited a minute and ran it again and it started running once a second again - that's without interactive with the screen at all. Looks like edge had issues like this in the past, for example: https://github.com/MicrosoftEdge/WebView2Feedback/issues/3024

I will dig some more but would be cool if you could verify that you see the same behaviour with a simple setTimeout regardless of the lib or Svelte

Raphael-Bahuau commented 1 year ago

I've executed your code on my console, same to me, it run every 1 seconds... And yes when I press F5, before the page reload completely, the code run normally at 100ms.

On Chrome the setTimeout works fine.

So what can we do about it ? Maybe instead of running with a setTimeout, you can use requestAnimationFrame on the dragOver ? Like if we don't pass the flipDurationMs, use the max framerate possible to do every cursor calculation, scheduled by requestAnimationFrame instead of setTimeout ?

Maybe this code on the @shopify/draggable/Plugin/SortAnimation (line 129) can help ?

    cancelAnimationFrame(this.lastAnimationFrame);

    // Can be done in a separate frame
    this.lastAnimationFrame = requestAnimationFrame(() => {
      effectedElements.forEach((element) => animate(element, this.options));
    });
  }
isaacHagoel commented 1 year ago

The interval is not for animation but for when to check the position of the dragged element. Checking every frame might result in a performance hit. I will play with it some more. I tried randomizing the interval a bit but it didn't help. We'll see if there is a reasonable workaround.

Raphael-Bahuau commented 1 year ago

Maybe you can expose the position check interval tho ? Let me know if you found something about this setTimout issue, maybe there is something on other libs to see ?

isaacHagoel commented 1 year ago

The interval depends on flipDurationMS (it has a minimum of 100 ms). In other words, the user can already control it. Not being able to run setTimeout or setInterval as specified is a severe browser bug that would affect many web applications (basically anything that uses throttling, debouncing or intervals), therefore I hope they will fix it and release a new version (remember that this is a new bug, it didn't reproduce for me until I upgraded). Let's give them a week or two and if it still exists then maybe there will be something to do on my end.

isaacHagoel commented 1 year ago

@Raphael-Bahuau I just tested it with version 113.0.1774.57 (latest) and it doesn't reproduce for me both the REPL and the run() function that didn't work before. Can you confirm?

Raphael-Bahuau commented 1 year ago

Hello, yes it's seem fixed now ! You can close the ticket, thank for your consideration 👍 .