Change in v10.11.1 causes inputs to lose focus on DOM changes #3814
Comments
|
I ran a git bisect and looks like the commit that caused this bug is: I reverted this change on top of the current master (d4089df) and I can confirm that reverting this change fixes the bug. Since I don't have enough knowledge of preact's internals to tell @JoviDeCroock 's intention, I won't submit this revert as the solution. |
|
Thanks for doing the bisect @jcestibariz, that's super helpful. My guess is that this is triggering a remount that isn't covered by our tests (which sometimes just assert final tree structure). |
|
Yes, I'm pretty sure the input is removed from the DOM and added back. One of the unit tests (test/browser/fragments.test.js line 240) seems to be checking for the DOM operations queued, in the diff for that commit you can see an additional |
|
Reverting it would not be the right course of action as the PR solves an issue with unmounting fragments, dissecting the issue though points at a bigger one 😅 so first off, the reproduction isn't using keys for a changing list of elements, which does hint that these remounts are intended. Going further, when we add the keys, in this reproduction, we can see that the focus is lost when we remove the first element but not the second time around which does point that we are dealing with a matching issue of or dom or vnodes. PR for reference #3738 - we might need to tweak this to better account for the two cases. |
|
I had missed the key in each "tag" span, sorry about that. I added that and added a third tag. The behaviour didn't change after adding the key, the bug is still reproducible. Notice that removing each of "bar" and "baz" tags the focus is lost. As @JoviDeCroock observed only when the last tag ("foo") is removed the focus stays on the input. |
|
Re: our test suite - we have quite a few tests around maintaining focus on inputs when adding/removing elements around it, but it does not cover removing the element directly before the input. Thanks for finding and reporting this! Fix incoming, hang tight :) |
…#3875) ## Why does #3814 fail on master? When transitioning from the tree ```jsx <div> <Fragment> <span>0</span> <span>1</span> </Fragment> <input /> </div> ``` to the tree (note `<span>1</span>` was unmounted) ```jsx <div> <Fragment> <span>0</span> </Fragment> <input /> </div> ``` the `master` branch has a bug where it will re-append all the children after the `Fragment`. This re-appending of the `<input>` causes it to lose focus. The reason for this re-appending is that when exiting `diffChildren` for the Fragment, it's `_nextDom` pointer is set to `<span>1</span>`. Since `<span>1</span>` is unmounted it's `parentNode` is `null`. When diffing the `input` element, we hit the `oldDom.parentNode !== parentDom` condition in `placeChild` which re-appends the `<input>` tag and sets oldDom to `null` causing all siblings of `<input>` to re-append. When diffing components/Fragments, the `_nextDom` pointer should point to where in the DOM tree the diff should continue. So when unmounting `<span>1</span>`, the Fragment's `_nextDom` should point to the `<input>` tag. The previous code in `diffChildren` removed in #3738 was intended to fix this by detecting when `_nextDom` pointed to a node that we were unmounting and reset it to it's sibling. However, that code (copied below) had a correctness bug prompting its removal (#3737). Let's look at this bug. ```js // Remove remaining oldChildren if there are any. for (i = oldChildrenLength; i--; ) { if (oldChildren[i] != null) { if ( typeof newParentVNode.type == 'function' && oldChildren[i]._dom != null && oldChildren[i]._dom == newParentVNode._nextDom ) { // If the newParentVNode.__nextDom points to a dom node that is about to // be unmounted, then get the next sibling of that vnode and set // _nextDom to it newParentVNode._nextDom = getDomSibling(oldParentVNode, i + 1); } unmount(oldChildren[i], oldChildren[i]); } } ``` ## What caused #3737? Here is a simplified repro of the issue from #3737: ```jsx function App() { const [condition] = useState(true); return this.state.condition ? ( // Initial render <> <div>1</div> <Fragment> <div>A</div> <div>B</div> </Fragment> <div>2</div> </> ) : ( // Second render: unmount <div>B and move Fragment up <> <Fragment> <div>A</div> </Fragment> <div>1</div> <div>2</div> </> ); } ``` The first render creates the DOM `1 A B 2` (each wrapped in divs) and when changing the `condition` state, it should rerender to produce `A 1 2` but instead we get `1 A 2`. Why? When rerendering `App` we unmount `<div>B</div>`, which is a child of a Fragment. This unmounting triggers the call to `getDomSibling` in the code above. `getDomSibling` has a line of code in it that looks like `vnode._parent._children.indexOf(vnode)`. This line of code doesn't work in this situation because when rerendering a component, we only make a shallow copy of the old VNode tree. So when a child of `oldParentVNode` (the `App` component in this case) tries to access it's parent through the `_parent` pointer (e.g. the `Fragment` parent of `<div>A</div>`), it's `_parent` pointer points to the new VNode tree which we are in progress of diffing and modifying. Ultimately, this tree mismatch (trying to access the old VNode tree but getting the in-progress new tree) causes us to get the wrong DOM pointer and leads to incorrect DOM ordering. In summary, when unmounting `<div>B</div>`, we need `getDomSibling` to return `<div>2</div>` since that is the next DOM sibling in the old VNode tree. Instead, because our VNode pointers are mixed at this stage of diffing, `getDomSibling` doesn't work correctly and we get back the wrong DOM element. ## Why didn't other tests catch this? Other tests only do top-level render calls (e.g. `render(<App condition={true} />)` then `render(<App condition={false} />)`) which generate brand-new VNode trees with no shared pointers. They did not test renders originating from `setState` calls which go through a different code path and reuse VNode trees which share pointers across the old and new trees. ## The fix The initial fix for this is to replace `getDomSibling` with a call to `dom.nextSibling` to get the actual next DOM sibling. (I found a situation in which this doesn't work optimally. I'll open a separate PR for that.) ## Final thoughts One additional thought I have here is that walking through this has given me more confidence in our approach for v11. First, we do unmounts before insertions so we don't have to do this additional DOM pointer checking. Also, by diffing backwards, we ensure that our `_next` pointers are correct when we go to search what DOM element to insert an element before. Fixes #3814, #3737
Describe the bug
Up to preact v10.11.0 changes in the DOM did not cause a focused input to lose focus. Starting with v10.11.1 the input loses focus.
To Reproduce
Steps to reproduce the behavior:
npm iand thennpm startnpm i preact@10.11.1and thennpm startExpected behavior
The input should not lose focus when DOM changes.
The text was updated successfully, but these errors were encountered: