Skip to content

Commit

Permalink
Fix a crash when using dynamic children in <option> tag (facebook#13261)
Browse files Browse the repository at this point in the history
* Make option children a text content by default

fix facebook#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
  • Loading branch information
Slowyn authored and segoddnja committed Aug 1, 2018
1 parent 972b959 commit e979e6b
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 7 deletions.
25 changes: 25 additions & 0 deletions fixtures/dom/src/components/fixtures/selects/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,31 @@ class SelectFixture extends React.Component {
</form>
</div>
</TestCase>

<TestCase
title="An option which contains conditional render fails"
relatedIssues="11911">
<TestCase.Steps>
<li>Select any option</li>
</TestCase.Steps>
<TestCase.ExpectedResult>
Option should be set
</TestCase.ExpectedResult>

<div className="test-fixture">
<select value={this.state.value} onChange={this.onChange}>
<option value="red">
red {this.state.value === 'red' && 'is chosen '} TextNode
</option>
<option value="blue">
blue {this.state.value === 'blue' && 'is chosen '} TextNode
</option>
<option value="green">
green {this.state.value === 'green' && 'is chosen '} TextNode
</option>
</select>
</div>
</TestCase>
</FixtureSet>
);
}
Expand Down
4 changes: 1 addition & 3 deletions packages/react-dom/src/__tests__/ReactDOMOption-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@ describe('ReactDOMOption', () => {
expect(() => {
node = ReactTestUtils.renderIntoDocument(el);
}).toWarnDev(
'<div> cannot appear as a child of <option>.\n' +
' in div (at **)\n' +
' in option (at **)',
'<div> cannot appear as a child of <option>.\n' + ' in option (at **)',
);
expect(node.innerHTML).toBe('1 2');
ReactTestUtils.renderIntoDocument(el);
Expand Down
32 changes: 32 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,38 @@ describe('ReactDOMSelect', () => {
expect(node.options[2].selected).toBe(false); // gorilla
});

it('should support options with dynamic children', () => {
const container = document.createElement('div');

let node;

function App({value}) {
return (
<select value={value} ref={n => (node = n)} onChange={noop}>
<option key="monkey" value="monkey">
A monkey {value === 'monkey' ? 'is chosen' : null}!
</option>
<option key="giraffe" value="giraffe">
A giraffe {value === 'giraffe' && 'is chosen'}!
</option>
<option key="gorilla" value="gorilla">
A gorilla {value === 'gorilla' && 'is chosen'}!
</option>
</select>
);
}

ReactDOM.render(<App value="monkey" />, container);
expect(node.options[0].selected).toBe(true); // monkey
expect(node.options[1].selected).toBe(false); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla

ReactDOM.render(<App value="giraffe" />, container);
expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla
});

it('should warn if value is null', () => {
expect(() =>
ReactTestUtils.renderIntoDocument(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,21 @@ describe('ReactDOMServerIntegration', () => {
expectSelectValue(e, 'bar');
},
);

itRenders('an option with flattened children', async render => {
const e = await render(
<select readOnly={true} value="bar">
<option value="bar">
{['Bar', false, 'Foo', <div key="1" />, 'Baz']}
</option>
</select>,
1,
);
expect(e.getAttribute('value')).toBe(null);
expect(e.getAttribute('defaultValue')).toBe(null);
expect(e.firstChild.innerHTML).toBe('BarFooBaz');
expect(e.firstChild.selected).toBe(true);
});
});

describe('user interaction', function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ module.exports = function(initModules) {
if (console.error.calls.count() > 0) {
console.log(`We saw these warnings:`);
for (let i = 0; i < console.error.calls.count(); i++) {
console.log(console.error.calls.argsFor(i)[0]);
console.log(...console.error.calls.argsFor(i));
}
}
}
Expand Down
30 changes: 27 additions & 3 deletions packages/react-dom/src/client/ReactDOMFiberOption.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import React from 'react';
import warning from 'shared/warning';
import validateDOMNesting from './validateDOMNesting';

let didWarnSelectedSetOnOption = false;

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

// Flatten children and warn if they aren't strings or numbers;
// invalid types are ignored.
// We can silently skip them because invalid DOM nesting warning
// catches these cases in Fiber.
React.Children.forEach(children, function(child) {
if (child == null) {
return;
}
if (typeof child === 'string' || typeof child === 'number') {
content += child;
}
// Note: we don't warn about invalid children here.
// Instead, this is done separately below so that
// it happens during the hydration codepath too.
});

return content;
Expand All @@ -36,8 +38,30 @@ function flattenChildren(children) {
*/

export function validateProps(element: Element, props: Object) {
// TODO (yungsters): Remove support for `selected` in <option>.
if (__DEV__) {
// Warn about invalid children, mirroring the logic above.
if (typeof props.children === 'object' && props.children !== null) {
React.Children.forEach(props.children, function(child) {
if (child == null) {
return;
}
if (typeof child === 'string' || typeof child === 'number') {
return;
}
// This is not real ancestor info but it's close enough
// to produce a useful warning for invalid children.
// We don't have access to the real one because the <option>
// fiber has already been popped, and threading it through
// is needlessly annoying.
const ancestorInfo = validateDOMNesting.updatedAncestorInfo(
null,
'option',
);
validateDOMNesting(child.type, null, ancestorInfo);
});
}

// TODO: Remove support for `selected` in <option>.
if (props.selected != null && !didWarnSelectedSetOnOption) {
warning(
false,
Expand Down
1 change: 1 addition & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ export function prepareUpdate(
export function shouldSetTextContent(type: string, props: Props): boolean {
return (
type === 'textarea' ||
type === 'option' ||
typeof props.children === 'string' ||
typeof props.children === 'number' ||
(typeof props.dangerouslySetInnerHTML === 'object' &&
Expand Down

0 comments on commit e979e6b

Please sign in to comment.