sveltejs / svelte

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

Keyed each does not preserve element (re-create) when reordered #3973

Open kutlugsahin opened 4 years ago

kutlugsahin commented 4 years ago

Describe the bug

I try to render a list of input element using keyed each block so that when enter key is pressed when an input is focused it will be swapped with the next input element. Likewise when shift+enter is pressed it will be swapped with the previous input element. Forward swapping works as expected and input keeps the focus. But backwards swap (shift+enter) causes the previous input to be re-created and the focus is lost.

I share a sample app written with both Svelte and React. React version work as expected.

Codesandbox Repo svelte demo react demo

Expected behavior Keyed list should respect the keys and matching elements should be reordered by getting detached and attached to preserve element instance.

Information about your Svelte project: Svelte 3

Conduitry commented 4 years ago

Even if I remove the bind:value={item.name} on each <input>, I'm seeing the values swap correctly (if I'd previously entered some), which is leading me to believe that the inputs are actually not being re-created, and that this is only a matter of where the focus ends up afterwards, which I suppose I don't have as strong opinions about.

kutlugsahin commented 4 years ago

You can confirm that the input element is re-created by inspecting the elements. The re-created input is flashing when you swap them. only the first one though others are preserved. Since focused input does not exist anymore the focus is also lost.

rixo commented 4 years ago

I've been bitten by this problem too, when doing a invoice editor of sort.

The element is indeed removed from the DOM by insertBefore by this code (confirmed by debugger in Chrome):

function insert(target, node, anchor) {
    target.insertBefore(node, anchor || null);
}

But so it is by React that runs this code:

function insertBefore(parentInstance, child, beforeChild) {
  parentInstance.insertBefore(child, beforeChild);
}

But then React actively restores focus:

    if (typeof priorFocusedElem.focus === 'function') {
      priorFocusedElem.focus();
    }

I would appreciate Svelte doing the same so that code of this kind just works as intended when you write it the first time.

Otherwise, or meanwhile, could the same solution as React be implemented in user's code? With an action + MutationObserver or something?

rixo commented 4 years ago

Re-reading the conversation above... The element is indeed not recreated, allowing it to keep its internal value, but the mere act of removing it (temporarily) from the DOM steals focus from it.

Now, the interesting question is: why doesn't it lose focus when moved forward in Svelte? Chrome's "break on node removal" doesn't trigger in this case either.

There is apparently a difference in implementation in Svelte when moving forward or backward. Maybe it is possible to achieve the same effect when moving forward as when moving backward, and that would make restoring the focus unneeded. Or maybe that's some browser quirks and the forceful focus fix would be needed to ensure proper behaviour...

rixo commented 4 years ago

The difference is that when moving forward, it's the following node that is moved before the focused node. The focused node is only used as the anchor, but it stays in the DOM. That explains everything.

Maybe Svelte could ensure that it doesn't remove the focused element from the DOM? I still fail to appreciate if that would be practical.

Note: React apparently relies on document.activeElement to know if the moved element needs to have focus restored. My understanding is that the focus would be lost on browsers that don't support that.

Note 2: It's not just the focus that is restored by React, it also restores the selection. It might be worth considering.

Conduitry commented 4 years ago

Perhaps better than attempting to restore the focus and selection afterwards might be to just avoid removing from the DOM the {#each} entry containing the active element. I'm not too familiar with the keyed each code and don't know how practical that would be.

kutlugsahin commented 4 years ago

@rixo yes indeed the elements are not re-created. it seems like the algorithm for reordering causes the active element detached and attached before the previous node when moving backwards. Instead the previous node should be attached after the current node keeping the active element in the dom if possible. So React seems to rely on an algorithm that keeps the active element in the dom always and move the others, so that the focus and selection will be preserved.

kutlugsahin commented 4 years ago

I created a similar issue in vanillajs. Removing the focused element causes it's focus and selection to get lost. That might be the reason why Svelte is not able to preserve it. So the possible solution would be to update the diff algorithm to take the active element into account and keep it in the dom while editing the element array. vanilla-demo

rixo commented 4 years ago

So React seems to rely on an algorithm that keeps the active element in the dom always and move the others, so that the focus and selection will be preserved.

That's not what I have seen. React always (forward or backward) removes the element from the DOM, but it saves focus & selection beforehand, and reapply them after the operation.

But indeed, tweaking the reorder algorithm in Svelte to avoid removing the activeElement from the DOM seems like it could be a more appealing solution.

kutlugsahin commented 4 years ago

Yes my assumption was wrong about how react handles this. It restores the focus, the selection and also the scroll position if the focus() causes an undesirable scrolls in the page. So keeping the active element in the dom while reordering the list might seem a good solution but in a scenario that you move the last element to the first place you actually need to move whole list after the last one and I'm not sure how costly it is compared to just moving the last element to the beginning of the list. Maybe some sort of best of both approach could be applied in this case.

danny-andrews commented 3 years ago

Just ran into this myself. Trying to implement drag-and-drop keyboard functionality. Moving things up in the list removes focus from the selected item. Anyone found a workaround in the meantime?

rixo commented 3 years ago

@danny-andrews Play it like React?

import { beforeUpdate, afterUpdate } from 'svelte'  

let activeElement

beforeUpdate(() => {
  activeElement = document.activeElement
})

afterUpdate(() => {
  if (activeElement) activeElement.focus()
})  
danny-andrews commented 3 years ago

Thanks @rixo. I I ended up doing something similar.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rixo commented 2 years ago

I think that should stay open. It's pretty annoying when you run into this (and it can take some time before realizing there's a problem, so you may have built on it...). Maybe not a high priority, but it would be great that this gets addressed eventually.

sanderjson commented 5 months ago

I have the same issue. How to maintain focus on an 'up' element?

Demo (Svelte5) here

Edit: Work around here