Clear existing text content before inserting children (#8331)
Fixes a bug when updating from a single text child (or dangerouslySetInnerHTML) to regular children, where the previous text content never gets deleted.
Andrew Clark committed
Nov 30, 2016 at 18:08 UTC
aa6279c41abdb992a7f82503c250af7627cd948e
9 files changed
+96
-18
scripts/fiber/tests-failing.txt
-3
@@ -32,8 +32,6 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js
32
* throws in render() if the update callback is not a function
33
34
src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
35
-* should empty element when removing innerHTML
36
-* should transition from innerHTML to children in nested el
35
* should warn for children on void elements
36
* should report component containing invalid styles
37
* should clean up input value tracking
@@ -104,7 +102,6 @@ src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js
102
* throws when rendering null at the top level
103
104
src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js
107
-* should correctly handle all possible children for render and update
105
* should reorder keyed text nodes
106
107
src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
scripts/fiber/tests-passing.txt
+3
@@ -601,8 +601,10 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
601
* should update arbitrary attributes for tags containing dashes
602
* should clear all the styles when removing `style`
603
* should update styles when `style` changes from null to object
604
+* should empty element when removing innerHTML
605
* should transition from string content to innerHTML
606
* should transition from innerHTML to string content
607
+* should transition from innerHTML to children in nested el
608
* should transition from children to innerHTML in nested el
609
* should not incur unnecessary DOM mutations for attributes
610
* should not incur unnecessary DOM mutations for string properties
@@ -1397,6 +1399,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js
1399
* should insert non-empty children in middle where nulls were
1400
1401
src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js
1402
+* should correctly handle all possible children for render and update
1403
* should throw if rendering both HTML and children
1404
* should render between nested components and inline children
1405
src/renderers/dom/fiber/ReactDOMFiber.js
+16
@@ -117,6 +117,22 @@ var DOMRenderer = ReactFiberReconciler({
117
updateProperties(domElement, type, oldProps, newProps, root);
118
},
119
120
+ shouldSetTextContent(props : Props) : boolean {
121
+ return (
122
+ typeof props.children === 'string' ||
123
+ typeof props.children === 'number' ||
124
+ (
125
+ typeof props.dangerouslySetInnerHTML === 'object' &&
126
+ props.dangerouslySetInnerHTML !== null &&
127
+ typeof props.dangerouslySetInnerHTML.__html === 'string'
128
+ )
129
+ );
130
+ },
131
+
132
+ resetTextContent(domElement : Instance) : void {
133
+ domElement.textContent = '';
134
+ },
135
+
136
createTextInstance(text : string, internalInstanceHandle : Object) : TextInstance {
137
var textNode : TextInstance = document.createTextNode(text);
138
precacheFiberNode(internalInstanceHandle, textNode);
src/renderers/noop/ReactNoop.js
+9
@@ -68,6 +68,15 @@ var NoopRenderer = ReactFiberReconciler({
68
instance.prop = newProps.prop;
69
},
70
71
+ shouldSetTextContent(props : Props) : boolean {
72
+ return (
73
+ typeof props.children === 'string' ||
74
+ typeof props.children === 'number'
75
+ );
76
+ },
77
+
78
+ resetTextContent(instance : Instance) : void {},
79
+
80
createTextInstance(text : string) : TextInstance {
81
var inst = { text : text, id: instanceCounter++ };
82
// Hide from unit tests
src/renderers/shared/fiber/ReactFiberBeginWork.js
+15
-1
@@ -53,6 +53,7 @@ var {
53
} = require('ReactPriorityLevel');
54
var {
55
Placement,
56
+ ContentReset,
57
} = require('ReactTypeOfSideEffect');
58
var ReactCurrentOwner = require('ReactCurrentOwner');
59
var ReactFiberClassComponent = require('ReactFiberClassComponent');
@@ -210,14 +211,27 @@ module.exports = function<T, P, I, TI, C>(
211
}
212
213
function updateHostComponent(current, workInProgress) {
214
+ const nextProps = workInProgress.pendingProps;
215
+ const prevProps = current ? current.memoizedProps : null;
216
let nextChildren = workInProgress.pendingProps.children;
214
- if (typeof nextChildren === 'string' || typeof nextChildren === 'number') {
217
+ const isDirectTextChild = config.shouldSetTextContent(nextProps);
218
+ if (isDirectTextChild) {
219
// We special case a direct text child of a host node. This is a common
220
// case. We won't handle it as a reified child. We will instead handle
221
// this in the host environment that also have access to this prop. That
222
// avoids allocating another HostText fiber and traversing it.
223
nextChildren = null;
224
+ } else if (prevProps && (
225
+ config.shouldSetTextContent(prevProps) ||
226
+ prevProps.children === null ||
227
+ typeof prevProps.children === 'undefined' ||
228
+ typeof prevProps.children === 'boolean'
229
+ )) {
230
+ // If we're switching from a direct text child to a normal child, or to
231
+ // empty, we need to schedule the text content to be reset.
232
+ workInProgress.effectTag |= ContentReset;
233
}
234
+
235
if (workInProgress.pendingProps.hidden &&
236
workInProgress.pendingWorkPriority !== OffscreenPriority) {
237
// If this host component is hidden, we can bail out on the children.
src/renderers/shared/fiber/ReactFiberCommitWork.js
+35
-5
@@ -30,6 +30,7 @@ var {
30
Placement,
31
Update,
32
Callback,
33
+ ContentReset,
34
} = require('ReactTypeOfSideEffect');
35
36
module.exports = function<T, P, I, TI, C>(
@@ -38,6 +39,7 @@ module.exports = function<T, P, I, TI, C>(
39
) {
40
41
const commitUpdate = config.commitUpdate;
42
+ const resetTextContent = config.resetTextContent;
43
const commitTextUpdate = config.commitTextUpdate;
44
45
const appendChild = config.appendChild;
@@ -83,6 +85,17 @@ module.exports = function<T, P, I, TI, C>(
85
throw new Error('Expected to find a host parent.');
86
}
87
88
+ function getHostParentFiber(fiber : Fiber) : Fiber {
89
+ let parent = fiber.return;
90
+ while (parent) {
91
+ if (isHostParent(parent)) {
92
+ return parent;
93
+ }
94
+ parent = parent.return;
95
+ }
96
+ throw new Error('Expected to find a host parent.');
97
+ }
98
+
99
function isHostParent(fiber : Fiber) : boolean {
100
return (
101
fiber.tag === HostComponent ||
@@ -133,12 +146,29 @@ module.exports = function<T, P, I, TI, C>(
146
}
147
148
function commitPlacement(finishedWork : Fiber) : void {
136
- // Clear effect from effect tag before any errors can be thrown, so that
137
- // we don't attempt to do this again
138
- finishedWork.effectTag &= ~Placement;
139
-
149
// Recursively insert all host nodes into the parent.
141
- const parent = getHostParent(finishedWork);
150
+ const parentFiber = getHostParentFiber(finishedWork);
151
+ let parent;
152
+ switch (parentFiber.tag) {
153
+ case HostComponent:
154
+ parent = parentFiber.stateNode;
155
+ break;
156
+ case HostContainer:
157
+ parent = parentFiber.stateNode.containerInfo;
158
+ break;
159
+ case Portal:
160
+ parent = parentFiber.stateNode.containerInfo;
161
+ break;
162
+ default:
163
+ throw new Error('Invalid host parent fiber.');
164
+ }
165
+ if (parentFiber.effectTag & ContentReset) {
166
+ // Reset the text content of the parent before doing any insertions
167
+ resetTextContent(parent);
168
+ // Clear ContentReset from the effect tag
169
+ parentFiber.effectTag &= ~ContentReset;
170
+ }
171
+
172
const before = getHostSibling(finishedWork);
173
// We only have the top Fiber that was inserted but we need recurse down its
174
// children to find all the terminal nodes.
src/renderers/shared/fiber/ReactFiberReconciler.js
+3
@@ -49,6 +49,9 @@ export type HostConfig<T, P, I, TI, C> = {
49
prepareUpdate(instance : I, oldProps : P, newProps : P) : boolean,
50
commitUpdate(instance : I, oldProps : P, newProps : P, internalInstanceHandle : OpaqueNode) : void,
51
52
+ shouldSetTextContent(props : P) : boolean,
53
+ resetTextContent(instance : I) : void,
54
+
55
createTextInstance(text : string, internalInstanceHandle : OpaqueNode) : TI,
56
commitTextUpdate(textInstance : TI, oldText : string, newText : string) : void,
57
src/renderers/shared/fiber/ReactFiberScheduler.js
+6
-1
@@ -40,6 +40,7 @@ var {
40
Update,
41
PlacementAndUpdate,
42
Deletion,
43
+ ContentReset,
44
Callback,
45
Err,
46
} = require('ReactTypeOfSideEffect');
@@ -168,11 +169,15 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
169
pass1: while (true) {
170
try {
171
while (effectfulFiber) {
172
+ if (effectfulFiber.effectTag & ContentReset) {
173
+ config.resetTextContent(effectfulFiber.stateNode);
174
+ }
175
+
176
// The following switch statement is only concerned about placement,
177
// updates, and deletions. To avoid needing to add a case for every
178
// possible bitmap value, we remove the secondary effects from the
179
// effect tag and switch on that value.
175
- let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err);
180
+ let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err | ContentReset);
181
switch (primaryEffectTag) {
182
case Placement: {
183
commitPlacement(effectfulFiber);
src/renderers/shared/fiber/ReactTypeOfSideEffect.js
+9
-8
@@ -12,14 +12,15 @@
12
13
'use strict';
14
15
-export type TypeOfSideEffect = 0 | 1 | 2 | 3 | 4 | 8 | 16;
15
+export type TypeOfSideEffect = 0 | 1 | 2 | 3 | 4 | 8 | 16 | 32;
16
17
module.exports = {
18
- NoEffect: 0, // 0b00000
19
- Placement: 1, // 0b00001
20
- Update: 2, // 0b00010
21
- PlacementAndUpdate: 3, // 0b00011
22
- Deletion: 4, // 0b00100
23
- Callback: 8, // 0b01000
24
- Err: 16, // 0b10000
18
+ NoEffect: 0, // 0b000000
19
+ Placement: 1, // 0b000001
20
+ Update: 2, // 0b000010
21
+ PlacementAndUpdate: 3, // 0b000011
22
+ Deletion: 4, // 0b000100
23
+ ContentReset: 8, // 0b001000
24
+ Callback: 16, // 0b010000
25
+ Err: 32, // 0b100000
26
};