Skip to content

Commit 0182a74

Browse files
Slowyngaearon
authored andcommitted
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
1 parent 2a2ef7e commit 0182a74

7 files changed

Lines changed: 102 additions & 7 deletions

File tree

fixtures/dom/src/components/fixtures/selects/index.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,31 @@ class SelectFixture extends React.Component {
158158
</form>
159159
</div>
160160
</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>
161186
</FixtureSet>
162187
);
163188
}

packages/react-dom/src/__tests__/ReactDOMOption-test.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,7 @@ describe('ReactDOMOption', () => {
4141
expect(() => {
4242
node = ReactTestUtils.renderIntoDocument(el);
4343
}).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 **)',
4745
);
4846
expect(node.innerHTML).toBe('1 2');
4947
ReactTestUtils.renderIntoDocument(el);

packages/react-dom/src/__tests__/ReactDOMSelect-test.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,38 @@ describe('ReactDOMSelect', () => {
545545
expect(node.options[2].selected).toBe(false); // gorilla
546546
});
547547

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+
548580
it('should warn if value is null', () => {
549581
expect(() =>
550582
ReactTestUtils.renderIntoDocument(

packages/react-dom/src/__tests__/ReactDOMServerIntegrationForms-test.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -413,6 +413,21 @@ describe('ReactDOMServerIntegration', () => {
413413
expectSelectValue(e, 'bar');
414414
},
415415
);
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+
});
416431
});
417432

418433
describe('user interaction', function() {

packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ module.exports = function(initModules) {
5858
if (console.error.calls.count() > 0) {
5959
console.log(`We saw these warnings:`);
6060
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));
6262
}
6363
}
6464
}

packages/react-dom/src/client/ReactDOMFiberOption.js

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
import React from 'react';
1111
import warning from 'shared/warning';
12+
import validateDOMNesting from './validateDOMNesting';
1213

1314
let didWarnSelectedSetOnOption = false;
1415

@@ -17,15 +18,16 @@ function flattenChildren(children) {
1718

1819
// Flatten children and warn if they aren't strings or numbers;
1920
// invalid types are ignored.
20-
// We can silently skip them because invalid DOM nesting warning
21-
// catches these cases in Fiber.
2221
React.Children.forEach(children, function(child) {
2322
if (child == null) {
2423
return;
2524
}
2625
if (typeof child === 'string' || typeof child === 'number') {
2726
content += child;
2827
}
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.
2931
});
3032

3133
return content;
@@ -36,8 +38,30 @@ function flattenChildren(children) {
3638
*/
3739

3840
export function validateProps(element: Element, props: Object) {
39-
// TODO (yungsters): Remove support for `selected` in <option>.
4041
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>.
4165
if (props.selected != null && !didWarnSelectedSetOnOption) {
4266
warning(
4367
false,

packages/react-dom/src/client/ReactDOMHostConfig.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,7 @@ export function prepareUpdate(
248248
export function shouldSetTextContent(type: string, props: Props): boolean {
249249
return (
250250
type === 'textarea' ||
251+
type === 'option' ||
251252
typeof props.children === 'string' ||
252253
typeof props.children === 'number' ||
253254
(typeof props.dangerouslySetInnerHTML === 'object' &&

0 commit comments

Comments
 (0)