Sibling Shows unexpectedly trigger blur on sibling

alex-stanescu commented 1 year ago

Describe the bug

When a (focusable) element is flanked by a Show component on either side, a signal triggering both Show components to true will cause the focusable element to lose focus.

Proximally this is caused by the reconcileArray logic using replaceChild rather than an insertion method (replacing the focusable element with one of the newly visible Shows, before putting the focusable element back in the DOM). This results in a blur event being fired at the focusable element (and no subsequent .focus event for it to regain focus).

Your Example Website or App

https://playground.solidjs.com/anonymous/6a550f0d-b8bd-41cd-840f-491c94b2ec36

Steps to Reproduce the Bug or Issue

Go to the playground link
Open the in-playground console
Click the Do Show button
observe the focusable div gets focused and then immediately blurred

Expected behavior

I expect that causing (seemingly) unrelated divs to Show will not result in a blur event being fired on the element that has focus. If that is not possible, I expect the element to at least regain focus after the reconciliation is complete

Screenshots or Videos

No response

Platform

Reproduces on multiple browsers, but mine ismacOS, Chrome Version 113.0.5672.92 (Official Build) (x86_64), using latest Solid

Additional context

No response

ryansolid commented 1 year ago

This is interesting. Yeah the udomdiff algorithm we moved to in march 2020, will produce the blur event. https://github.com/WebReflection/udomdiff/issues/7

I will need to see which ones don't. Keeping track of focus and focus restoration seems way more brittle than looking at the reconciliation algorithm. I don't see us ever doing that because the library is not really DOM aware and focus state isn't really represented declaratively. But I will need to audit solutions. It is quite likely that they all have edge cases in this regard.

Because there are performance implications this might take a while.

titoBouzout commented 2 months ago

I have run into this issue. When an Element is moved (that be append, insertBefore, or any other moving operation) it loses focus, there's no way around that.

This is visualizable by defining a custom element, focus it and logging the connectedCallback/disconnectedCallback. Element gets removed/added back to the dom. With a bit of CSS you can see also that the element its moved when the condition of the Show turns true https://playground.solidjs.com/anonymous/564b92ab-90bc-4c47-83a6-3ff17f53ac0f

For the diffing algorithm to keep the focused element focused. The focused element should become the center of gravity. Everything moves around this focused element. This is not workable, I mean, it will be too expensive that is not worthy. For example: If element 1 is focused and you move it to the bottom of a list with 10 items, then you will have to keep 1 on its place and move the other 9 items, to be able to not "touch" the focused element.

There's also the issue that when you move stuff around you could be moving parents, so you will have to check the subtree movable.contains(document.activeElement).

Then, there's the problem if you move the focused element out of its parent (instead of changing position on the same parent). Then it will lose focus no matter what.

I'm saying this because looking for a different algorithm won't be the solution and wanted to document my findings.

2. Comes to mind

const active = document.activeElement
doDiff()
active && active.focus()

I don't know if this has some performance issues, but I know of some situations on where this is not as simple. There's a need to restore scroll position because when running .focus() the browser puts the element in the middle of the screen and stuff moves which could be undesirable when not expected.

Wanting to throttle that snippet, If you have a table with 10 rows, with 10 inputs and 10 select elements with options, you will have at least 11 For and that code will run 11 times. Trying to throttle that logic is challenging because you cannot simple put it in a microtask, it has to be perfectly timed (fe: run before refs/onMount), let's say someone adds an onMount/ref and they change focus to something else, then restoring the focus with that snippet ill mean that they lose the focus that you have just added via ref/onMount.

Maybe that snippet could be used if doesnt impact performance,

A wrapper for the For component as a workaround https://playground.solidjs.com/anonymous/633194b1-6483-4f01-813a-c72c5356d7ef

  function createMemoKeepFocus(source) {
    return createMemo(() => {
      const active = document.activeElement;
      active && queueMicrotask(() => active.focus());
      return source();
    });
  }

  const elementsKeepFocus = createMemoKeepFocus(elements)
<For each={elementsKeepFocus()}>{props.children}</For>

4. Browsers would probably add new APIs that consider this problem among others (frames reloading, animations restarting also affected by the same issue) https://github.com/whatwg/dom/issues/1255

if someone has other ideas welcome

ryansolid commented 2 months ago

Thanks for the input @titoBouzout. Yeah I remember not checking all of them but most diffing algorithms will fall down in some scenario. It isn't avoidable, and as you said focus resetting is imperfect and has other side effects. I can just picture unrelated changes on inputs suddenly moving the cursor position. I don't think there is winning here. I don't think any framework as remotely solved this unfortunately. So yeah I don't know.

solidjs / solid