[Fiber] Attempt to fix ReactDOMTextComponent test in Fiber (#8146)
* Attempt to fix ReactDOMTextComponent test in Fiber Obviously, some tests pass but the reconciliation is still incomplete on this in Fiber. Those changes just ignore reconciliation Comments while asserting (this fixes 2 tests already) but now shows that Fiber lacks some reconciliation features. * Transmit a parentInstance to commitTextUpdate This is done in case a Node#normalize() was performed on the parent instance and that the TextInstance got desolidarized from the parent. We provide the parent known by the parent Fiber to reattach the DOM node in this case. * Added a more complex case around split nodes This test does not assume one split node substitution alone but also some non-split nodes. * Fiber supports split text nodes This is done by suppressing nodes appearing during the split. this is based by comparing against the old value known by the Fiber. * Simplify siblings cleaning in commitTextUpdate No need to assert instanceof Text since the length comparator will work correctly. * Ensure non-text nodes won't be removed * Fix flow for Fiber Text reconciliation additions Mostly define types as optionals. Respects the strict DOM API around Node & Element. * Append at any stack stage only if necessary The direct parent of the TextNode fiber may not contain the host node. We have to go through the hierarchy. However, since this work may be expensive, we only do it if absolutely necessary! * Use tag & ReactTypeOfWork to find an HostComponent * Addfailing scenario when running Node.normalize() It happens when non-text-elements are added in the mix. @sebmarkbage seems to have an idea on how to fix it. This is just a repro of the bug! * Register failing/passing fiber tests
Robin Ricard committed
Nov 7, 2016 at 20:39 UTC
33325ad0097e0689fd415703a1a7c1665629be7e
6 files changed
+94
-23
scripts/fiber/tests-failing.txt
+1
-4
@@ -188,10 +188,7 @@ src/renderers/dom/shared/__tests__/ReactDOMInvalidARIAHook-test.js
188
* should warn for an improperly cased aria-* prop
189
190
src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js
191
-* updates a mounted text component in place
192
-* can be toggled in and out of the markup
193
-* can reconcile text merged by Node.normalize()
194
-* can reconcile text arbitrarily split into multiple nodes
191
+* can reconcile text merged by Node.normalize() alongside other elements
192
193
src/renderers/dom/shared/__tests__/ReactEventIndependence-test.js
194
* does not crash with other react inside
scripts/fiber/tests-passing.txt
+5
@@ -537,7 +537,12 @@ src/renderers/dom/shared/__tests__/ReactDOMSVG-test.js
537
* creates initial namespaced markup
538
539
src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js
540
+* updates a mounted text component in place
541
+* can be toggled in and out of the markup
542
+* can reconcile text merged by Node.normalize()
543
* can reconcile text from pre-rendered markup
544
+* can reconcile text arbitrarily split into multiple nodes
545
+* can reconcile text arbitrarily split into multiple nodes on some substitutions only
546
547
src/renderers/dom/shared/__tests__/ReactEventListener-test.js
548
* should dispatch events from outside React tree
src/renderers/dom/fiber/ReactDOMFiber.js
+28
-1
@@ -13,9 +13,11 @@
13
'use strict';
14
15
import type { HostChildren } from 'ReactFiberReconciler';
16
+import type { Fiber } from 'ReactFiber';
17
18
var ReactFiberReconciler = require('ReactFiberReconciler');
19
var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
20
+var ReactTypeOfWork = require('ReactTypeOfWork');
21
22
var warning = require('warning');
23
@@ -88,8 +90,33 @@ var DOMRenderer = ReactFiberReconciler({
90
return document.createTextNode(text);
91
},
92
91
- commitTextUpdate(textInstance : TextInstance, oldText : string, newText : string) : void {
93
+ commitTextUpdate(textInstance : TextInstance, oldText : string, newText : string, current: ?Fiber) : void {
94
+ let nodeValueLen = textInstance.nodeValue.length;
95
+ let nextSibling : ?Node = textInstance.nextSibling;
96
+ // The Node may have been split in this case we need to clean up some next sibling nodes
97
+ while (nextSibling && nodeValueLen < oldText.length) {
98
+ const currentSibling : Node = nextSibling;
99
+ nextSibling = currentSibling.nextSibling;
100
+ nodeValueLen += currentSibling.nodeValue.length;
101
+ const parentNode = currentSibling.parentElement;
102
+ if (parentNode) {
103
+ parentNode.removeChild(currentSibling);
104
+ }
105
+ }
106
textInstance.nodeValue = newText;
107
+ // After a Node#normalize() on a parent, we need to reattach to the tree
108
+ if (!textInstance.parentNode) {
109
+ // We may need to go back through different types of work (not necessarily an host node)
110
+ // That's why, we're going up the stack testing for an HostComponent type of work
111
+ let parentFiber: ?Fiber = current && current.return;
112
+ while (parentFiber) {
113
+ if (parentFiber.tag === ReactTypeOfWork.HostComponent) {
114
+ (parentFiber.stateNode: Instance).appendChild(textInstance);
115
+ break;
116
+ }
117
+ parentFiber = parentFiber.return;
118
+ }
119
+ }
120
},
121
122
appendChild(parentInstance : Instance, child : Instance | TextInstance) : void {
src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js
+58
-16
@@ -15,6 +15,15 @@ var React;
15
var ReactDOM;
16
var ReactDOMServer;
17
18
+// In standard React, TextComponent keeps track of different Text templates
19
+// using comments. However, in React Fiber, those comments are not outputted due
20
+// to the way Fiber keeps track of the templates.
21
+// This function "Normalizes" childNodes lists to avoid the presence of comments
22
+// and make the child list identical in standard React and Fiber
23
+function filterOutComments(nodeList) {
24
+ return [].slice.call(nodeList).filter(node => !(node instanceof Comment));
25
+}
26
+
27
describe('ReactDOMTextComponent', () => {
28
beforeEach(() => {
29
React = require('React');
@@ -25,17 +34,19 @@ describe('ReactDOMTextComponent', () => {
34
it('updates a mounted text component in place', () => {
35
var el = document.createElement('div');
36
var inst = ReactDOM.render(<div><span />{'foo'}{'bar'}</div>, el);
37
+ let nodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
38
29
- var foo = ReactDOM.findDOMNode(inst).childNodes[2];
30
- var bar = ReactDOM.findDOMNode(inst).childNodes[5];
39
+ let foo = nodes[1];
40
+ let bar = nodes[2];
41
expect(foo.data).toBe('foo');
42
expect(bar.data).toBe('bar');
43
44
inst = ReactDOM.render(<div><span />{'baz'}{'qux'}</div>, el);
45
// After the update, the text nodes should have stayed in place (as opposed
46
// to getting unmounted and remounted)
37
- expect(ReactDOM.findDOMNode(inst).childNodes[2]).toBe(foo);
38
- expect(ReactDOM.findDOMNode(inst).childNodes[5]).toBe(bar);
47
+ nodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
48
+ expect(nodes[1]).toBe(foo);
49
+ expect(nodes[2]).toBe(bar);
50
expect(foo.data).toBe('baz');
51
expect(bar.data).toBe('qux');
52
});
@@ -45,22 +56,34 @@ describe('ReactDOMTextComponent', () => {
56
var inst = ReactDOM.render(<div>{'foo'}<div />{'bar'}</div>, el);
57
58
var container = ReactDOM.findDOMNode(inst);
48
- var childDiv = container.childNodes[3];
49
- var childNodes;
59
+ let childNodes = filterOutComments(container.childNodes);
60
+ let childDiv = childNodes[1];
61
62
inst = ReactDOM.render(<div>{null}<div />{null}</div>, el);
63
container = ReactDOM.findDOMNode(inst);
53
- childNodes = container.childNodes;
64
+ childNodes = filterOutComments(container.childNodes);
65
expect(childNodes.length).toBe(1);
66
expect(childNodes[0]).toBe(childDiv);
67
68
inst = ReactDOM.render(<div>{'foo'}<div />{'bar'}</div>, el);
69
container = ReactDOM.findDOMNode(inst);
59
- childNodes = container.childNodes;
60
- expect(childNodes.length).toBe(7);
61
- expect(childNodes[1].data).toBe('foo');
62
- expect(childNodes[3]).toBe(childDiv);
63
- expect(childNodes[5].data).toBe('bar');
70
+ childNodes = filterOutComments(container.childNodes);
71
+ expect(childNodes.length).toBe(3);
72
+ expect(childNodes[0].data).toBe('foo');
73
+ expect(childNodes[1]).toBe(childDiv);
74
+ expect(childNodes[2].data).toBe('bar');
75
+ });
76
+
77
+ it('can reconcile text merged by Node.normalize() alongside other elements', () => {
78
+ var el = document.createElement('div');
79
+ var inst = ReactDOM.render(<div>{'foo'}{'bar'}{'baz'}<span />{'qux'}</div>, el);
80
+
81
+ var container = ReactDOM.findDOMNode(inst);
82
+ container.normalize();
83
+
84
+ inst = ReactDOM.render(<div>{'bar'}{'baz'}{'qux'}<span />{'foo'}</div>, el);
85
+ container = ReactDOM.findDOMNode(inst);
86
+ expect(container.textContent).toBe('barbazquxfoo');
87
});
88
89
it('can reconcile text merged by Node.normalize()', () => {
@@ -95,14 +118,33 @@ describe('ReactDOMTextComponent', () => {
118
var inst = ReactDOM.render(<div><span />{'foobarbaz'}</div>, el);
119
120
var container = ReactDOM.findDOMNode(inst);
98
- var childNodes = container.childNodes;
99
- var textNode = childNodes[2];
121
+ let childNodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
122
+ let textNode = childNodes[1];
123
textNode.textContent = 'foo';
101
- container.insertBefore(document.createTextNode('bar'), childNodes[3]);
102
- container.insertBefore(document.createTextNode('baz'), childNodes[3]);
124
+ container.insertBefore(document.createTextNode('bar'), childNodes[1].nextSibling);
125
+ container.insertBefore(document.createTextNode('baz'), childNodes[1].nextSibling);
126
127
inst = ReactDOM.render(<div><span />{'barbazqux'}</div>, el);
128
container = ReactDOM.findDOMNode(inst);
129
expect(container.textContent).toBe('barbazqux');
130
});
131
+
132
+ it('can reconcile text arbitrarily split into multiple nodes on some substitutions only', () => {
133
+ var el = document.createElement('div');
134
+ var inst = ReactDOM.render(<div><span />{'bar'}<span />{'foobarbaz'}{'foo'}{'barfoo'}<span/></div>, el);
135
+
136
+ var container = ReactDOM.findDOMNode(inst);
137
+ let childNodes = filterOutComments(ReactDOM.findDOMNode(inst).childNodes);
138
+ let textNode = childNodes[3];
139
+ textNode.textContent = 'foo';
140
+ container.insertBefore(document.createTextNode('bar'), childNodes[3].nextSibling);
141
+ container.insertBefore(document.createTextNode('baz'), childNodes[3].nextSibling);
142
+ let secondTextNode = childNodes[5];
143
+ secondTextNode.textContent = 'bar';
144
+ container.insertBefore(document.createTextNode('foo'), childNodes[5].nextSibling);
145
+
146
+ inst = ReactDOM.render(<div><span />{'baz'}<span />{'barbazqux'}{'bar'}{'bazbar'}<span/></div>, el);
147
+ container = ReactDOM.findDOMNode(inst);
148
+ expect(container.textContent).toBe('bazbarbazquxbarbazbar');
149
+ });
150
});
src/renderers/shared/fiber/ReactFiberCommitWork.js
+1
-1
@@ -288,7 +288,7 @@ module.exports = function<T, P, I, TI, C>(
288
const textInstance : TI = finishedWork.stateNode;
289
const newText : string = finishedWork.memoizedProps;
290
const oldText : string = current.memoizedProps;
291
- commitTextUpdate(textInstance, oldText, newText);
291
+ commitTextUpdate(textInstance, oldText, newText, current);
292
return;
293
}
294
default:
src/renderers/shared/fiber/ReactFiberReconciler.js
+1
-1
@@ -49,7 +49,7 @@ export type HostConfig<T, P, I, TI, C> = {
49
commitUpdate(instance : I, oldProps : P, newProps : P) : void,
50
51
createTextInstance(text : string) : TI,
52
- commitTextUpdate(textInstance : TI, oldText : string, newText : string) : void,
52
+ commitTextUpdate(textInstance : TI, oldText : string, newText : string, parentInstance: ?Fiber) : void,
53
54
appendChild(parentInstance : I, child : I | TI) : void,
55
insertBefore(parentInstance : I, child : I | TI, beforeChild : I | TI) : void,