Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix uncontrolled radios #10186

Merged
merged 3 commits into from Jul 24, 2017
Merged

Fix uncontrolled radios #10186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a320eb
Fix uncontrolled radios
jquense Jul 14, 2017
ecba3d6
cherry-pick regression test from master
jquense Jul 19, 2017
f89c6b2
prettier
jquense Jul 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 29 additions & 6 deletions src/renderers/dom/client/__tests__/inputValueTracking-test.js
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@
'use strict';

var React = require('React');
var ReactDOM = require('ReactDOM');
var ReactTestUtils = require('ReactTestUtils');
var inputValueTracking = require('inputValueTracking');

Expand Down Expand Up @@ -143,16 +144,38 @@ describe('inputValueTracking', function() {
it('should stop tracking', function() {
inputValueTracking.track(mockComponent);

expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
true,
);
expect(mockComponent._wrapperState.valueTracker).not.toEqual(null);

inputValueTracking.stopTracking(mockComponent);

expect(mockComponent._wrapperState.hasOwnProperty('valueTracker')).toBe(
false,
);
expect(mockComponent._wrapperState.valueTracker).toEqual(null);

expect(input.hasOwnProperty('value')).toBe(false);
});

it('does not crash for nodes with custom value property', () => {
// https://github.com/facebook/react/issues/10196
try {
var originalCreateElement = document.createElement;
document.createElement = function() {
var node = originalCreateElement.apply(this, arguments);
Object.defineProperty(node, 'value', {
get() {},
set() {},
});
return node;
};
var div = document.createElement('div');
// Mount
var node = ReactDOM.render(<input type="text" />, div);
// Update
ReactDOM.render(<input type="text" />, div);
// Change
ReactTestUtils.SimulateNative.change(node);
// Unmount
ReactDOM.unmountComponentAtNode(div);
} finally {
document.createElement = originalCreateElement;
}
});
});
2 changes: 1 addition & 1 deletion src/renderers/dom/client/inputValueTracking.js
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ function attachTracker(inst, tracker) {
}

function detachTracker(inst) {
delete inst._wrapperState.valueTracker;
inst._wrapperState.valueTracker = null;
}

function getValueFromNode(node) {
Expand Down
4 changes: 4 additions & 0 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Expand Up @@ -909,6 +909,10 @@ ReactDOMComponent.Mixin = {
// happen after `_updateDOMProperties`. Otherwise HTML5 input validations
// raise warnings and prevent the new value from being assigned.
ReactDOMInput.updateWrapper(this);

// We also check that we haven't missed a value update, such as a
// Radio group shifting the checked value to another named radio input.
inputValueTracking.updateValueIfChanged(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in 15.6.2 this fix is no longer working...broadly when an radio changes it triggers an update in related (same name) radio inputs manually. Originally (unless i'm going crazy) those other updates hit this branch, .e.g radio1 triggered updateComponent and the other radios reset their inputTracking. Now it seems like this branch is missed entirely and ReactDOMInput.updateWrapper is called directly...I could totally be going crazy and this never worked originally but i'm certain we all tested it...

CC @aweary @nhunzaker

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we all tested this. The fixture in it's current state on 15.6.1 is completely broken on older builds of the fixtures (http://react-dom-test-fixtures.surge.sh/). Can we attribute this to a mixup pulling this code on to 15.x?

break;
case 'textarea':
ReactDOMTextarea.updateWrapper(this);
Expand Down