Improve Fragment unmounting while correctly swapping nested fragments #3875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Results02_replace1k
duration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
03_update10th1k_x16
duration
usedJSHeapSize
07_create10k
duration
usedJSHeapSize
filter_list
duration
usedJSHeapSize
hydrate1k
duration
usedJSHeapSize
many_updates
duration
usedJSHeapSize
text_update
duration
usedJSHeapSize
todo
duration
usedJSHeapSize
|
|
Size Change: +142 B (0%) Total Size: 53.1 kB
ℹ️ View Unchanged
|
andrewiggins
commented
Feb 1, 2023
| @@ -377,6 +383,22 @@ describe('focus', () => { | |||
| validateFocus(input, 'remove sibling before 2'); | |||
| }); | |||
|
|
|||
| it('should maintain focus when removing element directly before input', () => { | |||
There was a problem hiding this comment.
This test cases mimics what the repro in #3814 does. I can also add that specific repro if you'd like
There was a problem hiding this comment.
Fine be me the way you did it in the PR 👍
marvinhagemeister
approved these changes
Feb 1, 2023
JoviDeCroock
approved these changes
Feb 2, 2023
andrewiggins
added a commit
that referenced
this pull request
Feb 3, 2023
This PR is a follow up to #3875. I realized that we can't do oldChildren[i]._dom.nextSibling cuz oldChildren[i] itself could be Fragment representing a bunch of DOM nodes. We want the last dom node of oldChildren[i] and to get it's last sibling.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why does #3814 fail on master?
When transitioning from the tree
to the tree (note
<span>1</span>was unmounted)the
masterbranch has a bug where it will re-append all the children after theFragment. This re-appending of the<input>causes it to lose focus. The reason for this re-appending is that when exitingdiffChildrenfor the Fragment, it's_nextDompointer is set to<span>1</span>. Since<span>1</span>is unmounted it'sparentNodeisnull. When diffing theinputelement, we hit theoldDom.parentNode !== parentDomcondition inplaceChildwhich re-appends the<input>tag and sets oldDom tonullcausing all siblings of<input>to re-append.When diffing components/Fragments, the
_nextDompointer should point to where in the DOM tree the diff should continue. So when unmounting<span>1</span>, the Fragment's_nextDomshould point to the<input>tag. The previous code indiffChildrenremoved in #3738 was intended to fix this by detecting when_nextDompointed 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.What caused #3737?
Here is a simplified repro of the issue from #3737:
The first render creates the DOM
1 A B 2(each wrapped in divs) and when changing theconditionstate, it should rerender to produceA 1 2but instead we get1 A 2. Why?When rerendering
Appwe unmount<div>B</div>, which is a child of a Fragment. This unmounting triggers the call togetDomSiblingin the code above.getDomSiblinghas a line of code in it that looks likevnode._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 ofoldParentVNode(theAppcomponent in this case) tries to access it's parent through the_parentpointer (e.g. theFragmentparent of<div>A</div>), it's_parentpointer 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 needgetDomSiblingto 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,getDomSiblingdoesn'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} />)thenrender(<App condition={false} />)) which generate brand-new VNode trees with no shared pointers. They did not test renders originating fromsetStatecalls 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
getDomSiblingwith a call todom.nextSiblingto 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
_nextpointers are correct when we go to search what DOM element to insert an element before.Fixes #3814, #3737