Fix a crash when using dynamic children in <option> tag (#13261)
* Make option children a text content by default fix #11911 * Apply requested changes - Remove meaningless comments - revert scripts/rollup/results.json * remove empty row * Update comment * Add a simple unit-test * [WIP: no flow] Pass through hostContext * [WIP: no flow] Give better description for test * Fixes * Don't pass hostContext through It ended up being more complicated than I thought. * Also warn on hydration
Konstantin Yakushin committed
Aug 1, 2018 at 18:16 UTC
0182a74632b66a226921ca13dcfe359c67e15c6f
7 files changed
+102
-7
fixtures/dom/src/components/fixtures/selects/index.js
+25
@@ -158,6 +158,31 @@ class SelectFixture extends React.Component {
158
</form>
159
</div>
160
</TestCase>
161
+
162
+ <TestCase
163
+ title="An option which contains conditional render fails"
164
+ relatedIssues="11911">
165
+ <TestCase.Steps>
166
+ <li>Select any option</li>
167
+ </TestCase.Steps>
168
+ <TestCase.ExpectedResult>
169
+ Option should be set
170
+ </TestCase.ExpectedResult>
171
+
172
+ <div className="test-fixture">
173
+ <select value={this.state.value} onChange={this.onChange}>
174
+ <option value="red">
175
+ red {this.state.value === 'red' && 'is chosen '} TextNode
176
+ </option>
177
+ <option value="blue">
178
+ blue {this.state.value === 'blue' && 'is chosen '} TextNode
179
+ </option>
180
+ <option value="green">
181
+ green {this.state.value === 'green' && 'is chosen '} TextNode
182
+ </option>
183
+ </select>
184
+ </div>
185
+ </TestCase>
186
</FixtureSet>
187
);
188
}
packages/react-dom/src/__tests__/ReactDOMOption-test.js
+1
-3
@@ -41,9 +41,7 @@ describe('ReactDOMOption', () => {
41
expect(() => {
42
node = ReactTestUtils.renderIntoDocument(el);
43
}).toWarnDev(
44
- '<div> cannot appear as a child of <option>.\n' +
45
- ' in div (at **)\n' +
46
- ' in option (at **)',
44
+ '<div> cannot appear as a child of <option>.\n' + ' in option (at **)',
45
);
46
expect(node.innerHTML).toBe('1 2');
47
ReactTestUtils.renderIntoDocument(el);
packages/react-dom/src/__tests__/ReactDOMSelect-test.js
+32
@@ -545,6 +545,38 @@ describe('ReactDOMSelect', () => {
545
expect(node.options[2].selected).toBe(false); // gorilla
546
});
547
548
+ it('should support options with dynamic children', () => {
549
+ const container = document.createElement('div');
550
+
551
+ let node;
552
+
553
+ function App({value}) {
554
+ return (
555
+ <select value={value} ref={n => (node = n)} onChange={noop}>
556
+ <option key="monkey" value="monkey">
557
+ A monkey {value === 'monkey' ? 'is chosen' : null}!
558
+ </option>
559
+ <option key="giraffe" value="giraffe">
560
+ A giraffe {value === 'giraffe' && 'is chosen'}!
561
+ </option>
562
+ <option key="gorilla" value="gorilla">
563
+ A gorilla {value === 'gorilla' && 'is chosen'}!
564
+ </option>
565
+ </select>
566
+ );
567
+ }
568
+
569
+ ReactDOM.render(<App value="monkey" />, container);
570
+ expect(node.options[0].selected).toBe(true); // monkey
571
+ expect(node.options[1].selected).toBe(false); // giraffe
572
+ expect(node.options[2].selected).toBe(false); // gorilla
573
+
574
+ ReactDOM.render(<App value="giraffe" />, container);
575
+ expect(node.options[0].selected).toBe(false); // monkey
576
+ expect(node.options[1].selected).toBe(true); // giraffe
577
+ expect(node.options[2].selected).toBe(false); // gorilla
578
+ });
579
+
580
it('should warn if value is null', () => {
581
expect(() =>
582
ReactTestUtils.renderIntoDocument(
packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js
+15
@@ -413,6 +413,21 @@ describe('ReactDOMServerIntegration', () => {
413
expectSelectValue(e, 'bar');
414
},
415
);
416
+
417
+ itRenders('an option with flattened children', async render => {
418
+ const e = await render(
419
+ <select readOnly={true} value="bar">
420
+ <option value="bar">
421
+ {['Bar', false, 'Foo', <div key="1" />, 'Baz']}
422
+ </option>
423
+ </select>,
424
+ 1,
425
+ );
426
+ expect(e.getAttribute('value')).toBe(null);
427
+ expect(e.getAttribute('defaultValue')).toBe(null);
428
+ expect(e.firstChild.innerHTML).toBe('BarFooBaz');
429
+ expect(e.firstChild.selected).toBe(true);
430
+ });
431
});
432
433
describe('user interaction', function() {
packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js
+1
-1
@@ -58,7 +58,7 @@ module.exports = function(initModules) {
58
if (console.error.calls.count() > 0) {
59
console.log(`We saw these warnings:`);
60
for (let i = 0; i < console.error.calls.count(); i++) {
61
- console.log(console.error.calls.argsFor(i)[0]);
61
+ console.log(...console.error.calls.argsFor(i));
62
}
63
}
64
}
packages/react-dom/src/client/ReactDOMFiberOption.js
+27
-3
@@ -9,6 +9,7 @@
9
10
import React from 'react';
11
import warning from 'shared/warning';
12
+import validateDOMNesting from './validateDOMNesting';
13
14
let didWarnSelectedSetOnOption = false;
15
@@ -17,8 +18,6 @@ function flattenChildren(children) {
18
19
// Flatten children and warn if they aren't strings or numbers;
20
// invalid types are ignored.
20
- // We can silently skip them because invalid DOM nesting warning
21
- // catches these cases in Fiber.
21
React.Children.forEach(children, function(child) {
22
if (child == null) {
23
return;
@@ -26,6 +25,9 @@ function flattenChildren(children) {
25
if (typeof child === 'string' || typeof child === 'number') {
26
content += child;
27
}
28
+ // Note: we don't warn about invalid children here.
29
+ // Instead, this is done separately below so that
30
+ // it happens during the hydration codepath too.
31
});
32
33
return content;
@@ -36,8 +38,30 @@ function flattenChildren(children) {
38
*/
39
40
export function validateProps(element: Element, props: Object) {
39
- // TODO (yungsters): Remove support for `selected` in <option>.
41
if (__DEV__) {
42
+ // Warn about invalid children, mirroring the logic above.
43
+ if (typeof props.children === 'object' && props.children !== null) {
44
+ React.Children.forEach(props.children, function(child) {
45
+ if (child == null) {
46
+ return;
47
+ }
48
+ if (typeof child === 'string' || typeof child === 'number') {
49
+ return;
50
+ }
51
+ // This is not real ancestor info but it's close enough
52
+ // to produce a useful warning for invalid children.
53
+ // We don't have access to the real one because the <option>
54
+ // fiber has already been popped, and threading it through
55
+ // is needlessly annoying.
56
+ const ancestorInfo = validateDOMNesting.updatedAncestorInfo(
57
+ null,
58
+ 'option',
59
+ );
60
+ validateDOMNesting(child.type, null, ancestorInfo);
61
+ });
62
+ }
63
+
64
+ // TODO: Remove support for `selected` in <option>.
65
if (props.selected != null && !didWarnSelectedSetOnOption) {
66
warning(
67
false,
packages/react-dom/src/client/ReactDOMHostConfig.js
+1
@@ -248,6 +248,7 @@ export function prepareUpdate(
248
export function shouldSetTextContent(type: string, props: Props): boolean {
249
return (
250
type === 'textarea' ||
251
+ type === 'option' ||
252
typeof props.children === 'string' ||
253
typeof props.children === 'number' ||
254
(typeof props.dangerouslySetInnerHTML === 'object' &&