-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IE11: Fix incorrect node order with conditionals and text nodes #4650
base: main
Are you sure you want to change the base?
Conversation
📊 Tachometer Benchmark ResultsSummaryduration
usedJSHeapSize
Resultscreate10kduration
usedJSHeapSize
filter-listduration
usedJSHeapSize
hydrate1kduration
usedJSHeapSize
many-updatesduration
usedJSHeapSize
replace1kduration
usedJSHeapSize
run-warmup-0
run-warmup-1
run-warmup-2
run-warmup-3
run-warmup-4
run-final
text-updateduration
usedJSHeapSize
tododuration
usedJSHeapSize
update10th1kduration
usedJSHeapSize
|
Wow! There is already a hack: Line 42 in 3ff5f50
|
@@ -339,7 +339,7 @@ function insert(parentVNode, oldDom, parentDom) { | |||
|
|||
return oldDom; | |||
} else if (parentVNode._dom != oldDom) { | |||
if (oldDom && parentVNode.type && !parentDom.contains(oldDom)) { | |||
if (oldDom && parentVNode.type && !contains(parentDom, oldDom)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could solve this a lot simpler, similar to #4666 - alternatively, we could check whether oldDom
is in fact a DOM-node rather than a text-node.
@@ -39,7 +39,7 @@ function Portal(props) { | |||
nodeType: 1, | |||
parentNode: container, | |||
childNodes: [], | |||
contains: () => true, | |||
isPortal: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unsafe change as other faux roots could be using this, also contains
is not a hack, it's a DOM built-in
There is an issue with using Element.contains in IE11. IE11 only partially supports contains: it works for Element nodes but does not support Text nodes. This can cause incorrect node ordering when conditionally rendering elements that include text nodes.
Minimal Repro:
Expected output:
Actual output in IE11: