preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.71k stars 1.95k forks source link

Focus is lost during list re-order #3242

Open mattorchard opened 3 years ago

mattorchard commented 3 years ago

Describe the bug Focus is lost during list re-order. When a list re-renders, if the focused item is moved to later in the source order, it loses focus.

To Reproduce As shown in this Code Sandbox If a list is re-ordered: focus is properly preserved when moving up in the source order, but not down in the source order. (All items have keys that are stable and not indexed based) preact-reorder-focus-bug

Steps to reproduce the behavior:

  1. Go to the Code Sandbox
  2. Click on an item in the list
  3. Move the item up with the up arrow key, and observe the focus remains on the correct item.
  4. Move the item down with the down arrow key, and observe the focus is lost.

Expected behavior The item should maintain focus no matter which direction in the source order it moves.

Similar issue This is similar to: Input loses focus when conditionally rerendering other divs #540. However, since this issue is not about conditional rendering, only list rendering, and because that issue is closed whereas this issue effects the latest tested version (10.5.14), I believe this warrants it's own ticket.

nmain commented 3 years ago

Even when keys match and DOM elements are reused, in order to keep focus you either need to 1) avoid removing the focused element from the DOM, or 2) manually refocus it after removing+reinserting it. Preact reuses the DOM element correctly, but takes no care about what is removed+reinserted versus what stays in.

The given example works in React, so somewhere in the depths of that massive codebase they're doing 1) or 2).

mattorchard commented 3 years ago

Maybe I've missed something but in this vanilla JS code sandbox the focus is lost when the source order changes in either direction.

This at least makes it seem like Preact is attempting to preserve focus, and the case where it moves later in the source order is broken.

vanilla-rorder-focus

nmain commented 3 years ago

As I outlined in my previous response, the focus will automatically remain if you do not remove the element from the DOM. The vanilla example can easily be tweaked to do that: https://codesandbox.io/s/source-order-focus-lost-vanilla-forked-pf3pr.

Preact is losing focus in some cases and not others simply because of which elements the reconciler chooses to remove and reinsert versus which elements stay attached.

mattorchard commented 3 years ago

Ahh, I see. I hadn't clued in that it was just a matter of swapping the other element for the focused one to remain selected.

Given that this does work in React (as you mentioned above, and shown in this React CodeSandbox): Should this issue still be considered a bug for Preact? Either to fix, or to add to the "Differences to React" docs?

developit commented 3 years ago

Interesting - this is an unfortunate effect of an optimization Preact does: we "detect" small swap or shift movements within a list of keyed items and only invoke a single insertBefore() to move one of the items because we know the other affected item will end up in the correct place as a result. In order for this to work, we always move the first out-of-place item.

In your demo, hitting the up arrow key shifts the item up, which means it's the first out-of-place item in the newly sorted list of items. Conversely, the down arrow key shifts the item down, which means the item it was shifted "around" (the next item in the list) will be the first out-of-place item in the new set of children.

The result is that pressing "up" moves the previous item to after the selected item, whereas pressing "down" moves the selected item to after the next item, which means you lose focus.

We removed manual focus management code from Preact, because it's really difficult to justify the performance impact. I do think this is an interesting case to explore fixes for (rather than documenting as a difference to React), but we'd be looking for ways of taking focus state into account during diffing rather than any sort of manual focus restoration.

johnemau commented 1 year ago

Using the DevTools I found that React is doing the same insertBefore behavior as Preact.

React appears to track if the element about to move is the document.activeElement and then calls focus on the element after the move to reset focus.

It appears that focus is not kept but just getting reset, this may have some consequence for screen readers or apps that have focus management logic.

Haroenv commented 2 days ago

Long discussion and I don't think it came to a solution, but in whatwg/html an atomic move operation has been suggested and rejected https://github.com/whatwg/html/issues/5484#issuecomment-620247769. Is there some solution we can imagine for this that's not just restoring the focus or trying to find the interacting element and moving the others (as you can't really know what's active)

rschristian commented 2 days ago

in whatwg/html an atomic move operation has been suggested and rejected

Hasn't been rejected for good fwiw. Chrome has picked up the effort again but it's still very early: https://github.com/whatwg/dom/issues/1255