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

bug fix on input radio #11227

Merged
merged 9 commits into from
Nov 19, 2017
Merged

bug fix on input radio #11227

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4ce1fb0
fixed a bug on radio input
Oct 14, 2017
0e15659
Merge branch 'master' of https://github.com/facebook/react
Oct 22, 2017
6f3a000
roll back previous commit
Oct 27, 2017
aad2f64
moved comment
Oct 27, 2017
03b1b30
merge from upstream/master
Nov 2, 2017
69cbb05
Merge branch 'master' of https://github.com/facebook/react
Nov 7, 2017
25c0fc4
added a fixture for radio bug
Nov 7, 2017
e03b117
fixed prettier error
Nov 7, 2017
9ee15eb
read type from props, not node
Nov 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,45 @@ describe('ReactDOMInput', () => {
expect(cNode.checked).toBe(true);
});

it('should have correct checked value when radio names changed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind rephrasing this to should check the correct radio when the selected name moves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. yours is better

class App extends React.Component {
state = {
updated: false,
};
onClick = () => {
this.setState({updated: true});
};
render() {
const {updated} = this.state;
const radioName = updated ? 'secondName' : 'firstName';
return (
<div>
<button type="button" onClick={this.onClick} />
<input
type="radio"
name={radioName}
onChange={emptyFunction}
checked={updated === true}
/>
<input
type="radio"
name={radioName}
onChange={emptyFunction}
checked={updated === false}
/>
</div>
);
}
}

var stub = ReactTestUtils.renderIntoDocument(<App />);
var buttonNode = ReactDOM.findDOMNode(stub).childNodes[0];
var firstRadioNode = ReactDOM.findDOMNode(stub).childNodes[1];
expect(firstRadioNode.checked).toBe(false);
ReactTestUtils.Simulate.click(buttonNode);
expect(firstRadioNode.checked).toBe(true);
});

it('should control radio buttons if the tree updates during render', () => {
var sharedParent = document.createElement('div');
var container1 = document.createElement('div');
Expand Down
7 changes: 7 additions & 0 deletions packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,13 @@ export function updateProperties(
lastRawProps: Object,
nextRawProps: Object,
): void {
// Update checked *before* name.
// In the middle of an update, it is possible to have multiple checked.
// When a checked radio tries to change name, browser makes another radio's checked false.
if (tag === 'input') {
ReactDOMFiberInput.updateChecked(domElement, nextRawProps, 'radio');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to figure out why this is necessary (but it is definitely effective).

updateNamedCousins in ReactDOMInput should take care of this, but it looks like it doesn't run in the test case you've provided.

@jquense updateNamedCousins gets called in restoreControlledState, but for what ever reason it only fires on the button, which I guess is clicked and responsible for the last trigger. The radio input never gets targeted by restoreControlledState.

It feels like there's something deeper here that we need to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think updateNamedCousins is called only on the button because of this code ReactTestUtils.Simulate.click(buttonNode);
I checked that updateNamedCousins is not called when I run your fixture.


var wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
var isCustomComponentTag = isCustomComponent(tag, nextRawProps);
// Apply the diff.
Expand Down
27 changes: 19 additions & 8 deletions packages/react-dom/src/client/ReactDOMFiberInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,24 @@ export function initWrapperState(element: Element, props: Object) {
};
}

export function updateChecked(
element: Element,
props: Object,
targetType: ?string,
) {
var node = ((element: any): InputWithWrapperState);
if (!targetType || targetType === node.type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In updateNamedCousins, we check for radio buttons like:

function updateNamedCousins(rootNode, props) {
  var name = props.name;
  if (props.type === 'radio' && name != null) {
    // ...
  }
  // ...
}

Can you use that check here? That would allow us to remove the targetType argument and reading from the DOM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the code in updateProperties because updateWrapper also calls updateChecked

var checked = props.checked;
if (checked != null) {
DOMPropertyOperations.setValueForProperty(
node,
'checked',
checked || false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of a bummer that we have to || false here. It can't hurt, but I wonder if this is necessary (just with a quick test via https://codepen.io/nhunzaker/pen/qVadLo).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think it's unnecessary.

);
}
}
}

export function updateWrapper(element: Element, props: Object) {
var node = ((element: any): InputWithWrapperState);
if (__DEV__) {
Expand Down Expand Up @@ -183,14 +201,7 @@ export function updateWrapper(element: Element, props: Object) {
}
}

var checked = props.checked;
if (checked != null) {
DOMPropertyOperations.setValueForProperty(
node,
'checked',
checked || false,
);
}
updateChecked(element, props);

var value = props.value;
if (value != null) {
Expand Down