Handle the radio button case completely in the asap case (#8170)
Instead of scheduling individual callbacks to asap, we schedule one and then do all the work in that one. I'm doing this for architectural refactoring reasons. Nevertheless, I'm adding a contrived unit test that this fixes. :)
Sebastian Markbåge committed
Nov 1, 2016 at 18:54 UTC
576542d0cfa61f22ddf390c44015990128564637
3 files changed
+89
-9
scripts/fiber/tests-failing.txt
+1
@@ -281,6 +281,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
281
* should properly control a value of number `0`
282
* should have the correct target value
283
* should control radio buttons
284
+* should control radio buttons if the tree updates during render
285
* should warn with value and no onChange handler and readOnly specified
286
* should warn with checked and no onChange handler with readOnly specified
287
* should warn if value is null
src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
+75
-3
@@ -347,9 +347,9 @@ describe('ReactDOMInput', () => {
347
}
348
349
var stub = ReactTestUtils.renderIntoDocument(<RadioGroup />);
350
- var aNode = ReactDOM.findDOMNode(stub.refs.a);
351
- var bNode = ReactDOM.findDOMNode(stub.refs.b);
352
- var cNode = ReactDOM.findDOMNode(stub.refs.c);
350
+ var aNode = stub.refs.a;
351
+ var bNode = stub.refs.b;
352
+ var cNode = stub.refs.c;
353
354
expect(aNode.checked).toBe(true);
355
expect(bNode.checked).toBe(false);
@@ -371,6 +371,78 @@ describe('ReactDOMInput', () => {
371
expect(cNode.checked).toBe(true);
372
});
373
374
+ it('should control radio buttons if the tree updates during render', () => {
375
+ var sharedParent = document.createElement('div');
376
+ var container1 = document.createElement('div');
377
+ var container2 = document.createElement('div');
378
+
379
+ sharedParent.appendChild(container1);
380
+
381
+ var aNode;
382
+ var bNode;
383
+ class ComponentA extends React.Component {
384
+ componentDidMount() {
385
+ ReactDOM.render(<ComponentB />, container2);
386
+ }
387
+ render() {
388
+ return (
389
+ <div>
390
+ <input
391
+ ref={n => aNode = n}
392
+ type="radio"
393
+ name="fruit"
394
+ checked={true}
395
+ onChange={emptyFunction}
396
+ />A
397
+ </div>
398
+ );
399
+ }
400
+ }
401
+
402
+ class ComponentB extends React.Component {
403
+ state = { changed: false };
404
+ handleChange = () => {
405
+ this.setState({
406
+ changed: true,
407
+ })
408
+ }
409
+ componentDidUpdate() {
410
+ sharedParent.appendChild(container2);
411
+ }
412
+ render() {
413
+ return (
414
+ <div>
415
+ <input
416
+ ref={n => bNode = n}
417
+ type="radio"
418
+ name="fruit"
419
+ checked={false}
420
+ onChange={this.handleChange}
421
+ />B
422
+ </div>
423
+ );
424
+ }
425
+ }
426
+
427
+ ReactDOM.render(<ComponentA />, container1);
428
+
429
+ expect(aNode.checked).toBe(true);
430
+ expect(bNode.checked).toBe(false);
431
+
432
+ bNode.checked = true;
433
+ // This next line isn't necessary in a proper browser environment, but
434
+ // jsdom doesn't uncheck the others in a group (which makes this whole test
435
+ // a little less effective)
436
+ aNode.checked = false;
437
+
438
+ // Now let's run the actual ReactDOMInput change event handler
439
+ ReactTestUtils.Simulate.change(bNode);
440
+
441
+ // The original state should have been restored
442
+ expect(aNode.checked).toBe(true);
443
+ expect(bNode.checked).toBe(false);
444
+ });
445
+
446
it('should warn with value and no onChange handler and readOnly specified', () => {
447
ReactTestUtils.renderIntoDocument(
448
<input type="text" value="zoink" readOnly={true} />
src/renderers/dom/stack/client/wrappers/ReactDOMInput.js
+13
-6
@@ -29,6 +29,8 @@ function forceUpdateIfMounted() {
29
// DOM component is still mounted; update
30
ReactDOMInput.updateWrapper(this);
31
}
32
+ var props = this._currentElement.props;
33
+ updateNamedCousins(this, props);
34
}
35
36
function isControlled(props) {
@@ -278,9 +280,13 @@ function _handleChange(event) {
280
// https://github.com/facebook/react/issues/1698
281
ReactUpdates.asap(forceUpdateIfMounted, this);
282
283
+ return returnValue;
284
+}
285
+
286
+function updateNamedCousins(thisInstance, props) {
287
var name = props.name;
288
if (props.type === 'radio' && name != null) {
283
- var rootNode = ReactDOMComponentTree.getNodeFromInstance(this);
289
+ var rootNode = ReactDOMComponentTree.getNodeFromInstance(thisInstance);
290
var queryRoot = rootNode;
291
292
while (queryRoot.parentNode) {
@@ -291,8 +297,9 @@ function _handleChange(event) {
297
// but that sometimes behaves strangely in IE8. We could also try using
298
// `form.getElementsByName`, but that will only return direct children
299
// and won't include inputs that use the HTML5 `form=` attribute. Since
294
- // the input might not even be in a form, let's just use the global
295
- // `querySelectorAll` to ensure we don't miss anything.
300
+ // the input might not even be in a form. It might not even be in the
301
+ // document. Let's just use the local `querySelectorAll` to ensure we don't
302
+ // miss anything.
303
var group = queryRoot.querySelectorAll(
304
'input[name=' + JSON.stringify('' + name) + '][type="radio"]');
305
@@ -315,11 +322,11 @@ function _handleChange(event) {
322
// If this is a controlled radio button group, forcing the input that
323
// was previously checked to update will cause it to be come re-checked
324
// as appropriate.
318
- ReactUpdates.asap(forceUpdateIfMounted, otherInstance);
325
+ if (otherInstance._rootNodeID) {
326
+ ReactDOMInput.updateWrapper(otherInstance);
327
+ }
328
}
329
}
321
-
322
- return returnValue;
330
}
331
332
module.exports = ReactDOMInput;