Skip to content

Commit

Permalink
Use the virtual target for change events to avoid restoring controlle…
Browse files Browse the repository at this point in the history
…d state on the real target (#10444)

* Add test for nested controlled selects

The failure repros only when two events are fired sequentially, where the second
event is what updates the value.

* Add failure case to DOM select fixture

* Use virtual event target for change event instead of native target

Ensures that we use the same node to make decisions instead of two like
we do now.

This will cause restore to be called or not called consistently.

* Pass native event target to the event

We normally pass the native event target to the event object. This ensures
that we do the same thing here.

We still want to schedule a "restore" on the virtual target though since
those are the only nodes known to React.
  • Loading branch information
sebmarkbage authored Aug 12, 2017
1 parent 2cd0ecd commit 3bc6432
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 4 deletions.
34 changes: 33 additions & 1 deletion fixtures/dom/src/components/fixtures/selects/index.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,34 @@
const React = window.React;
const ReactDOM = window.ReactDOM;

class SelectFixture extends React.Component {
state = {value: ''};
_nestedDOMNode = null;

onChange = event => {
this.setState({value: event.target.value});
};

componentDidMount() {
this._renderNestedSelect();
}

componentDidUpdate() {
this._renderNestedSelect();
}

_renderNestedSelect() {
ReactDOM.render(
<select value={this.state.value} onChange={this.onChange}>
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="green">Green</option>
</select>,
this._nestedDOMNode
);
}

render() {
return (
<form>
Expand All @@ -24,8 +48,16 @@ class SelectFixture extends React.Component {
<option value="">Select a color</option>
<option value="red">Red</option>
<option value="blue">Blue</option>
<option value="gree">Green</option>
<option value="green">Green</option>
</select>
<span className="hint" />
</fieldset>
<fieldset>
<legend>Controlled in nested subtree</legend>
<div ref={node => (this._nestedDOMNode = node)} />
<span className="hint">
This should synchronize in both direction with the one above.
</span>
</fieldset>
</form>
);
Expand Down
12 changes: 9 additions & 3 deletions src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,21 @@ function shouldUseChangeEvent(elem) {
);
}

function createAndAccumulateChangeEvent(inst, nativeEvent, target) {
function createAndAccumulateChangeEvent(
inst,
nativeEvent,
nativeEventTarget,
virtualTarget,
) {
var event = SyntheticEvent.getPooled(
eventTypes.change,
inst,
nativeEvent,
target,
nativeEventTarget,
);
event.type = 'change';
// Flag this event loop as needing state restore.
ReactControlledComponent.enqueueStateRestore(target);
ReactControlledComponent.enqueueStateRestore(virtualTarget);
EventPropagators.accumulateTwoPhaseDispatches(event);
return event;
}
Expand Down Expand Up @@ -175,6 +180,7 @@ var ChangeEventPlugin = {
inst,
nativeEvent,
nativeEventTarget,
targetNode,
);
return event;
}
Expand Down
64 changes: 64 additions & 0 deletions src/renderers/dom/shared/wrappers/__tests__/ReactDOMSelect-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -589,4 +589,68 @@ describe('ReactDOMSelect', () => {
expect(node.options[1].selected).toBe(true); // b
expect(node.options[2].selected).toBe(false); // c
});

it('should allow controlling `value` in a nested render', () => {
var selectNode;

class Parent extends React.Component {
state = {
value: 'giraffe',
};

componentDidMount() {
this._renderNested();
}

componentDidUpdate() {
this._renderNested();
}

_handleChange(event) {
this.setState({value: event.target.value});
}

_renderNested() {
ReactDOM.render(
<select
onChange={this._handleChange.bind(this)}
ref={n => (selectNode = n)}
value={this.state.value}>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
<option value="gorilla">A gorilla!</option>
</select>,
this._nestingContainer,
);
}

render() {
return <div ref={n => (this._nestingContainer = n)} />;
}
}

var container = document.createElement('div');

document.body.appendChild(container);

ReactDOM.render(<Parent />, container);

expect(selectNode.value).toBe('giraffe');

selectNode.value = 'gorilla';

let nativeEvent = document.createEvent('Event');
nativeEvent.initEvent('input', true, true);
selectNode.dispatchEvent(nativeEvent);

expect(selectNode.value).toEqual('gorilla');

nativeEvent = document.createEvent('Event');
nativeEvent.initEvent('change', true, true);
selectNode.dispatchEvent(nativeEvent);

expect(selectNode.value).toEqual('gorilla');

document.body.removeChild(container);
});
});

0 comments on commit 3bc6432

Please sign in to comment.