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

Fix the "checked" attribute is not initially set on the input #13114

Merged
merged 1 commit into from
Jul 4, 2018
Merged

Fix the "checked" attribute is not initially set on the input #13114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 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 @@ -739,9 +739,12 @@ describe('ReactDOMInput', () => {
const cNode = stub.refs.c;

expect(aNode.checked).toBe(true);
expect(aNode.hasAttribute('checked')).toBe(true);
expect(bNode.checked).toBe(false);
expect(bNode.hasAttribute('checked')).toBe(false);
// c is in a separate form and shouldn't be affected at all here
expect(cNode.checked).toBe(true);
expect(cNode.hasAttribute('checked')).toBe(true);

bNode.checked = true;
// This next line isn't necessary in a proper browser environment, but
Expand All @@ -750,6 +753,11 @@ describe('ReactDOMInput', () => {
aNode.checked = false;
expect(cNode.checked).toBe(true);

// The original 'checked' attribute should be unchanged
expect(aNode.hasAttribute('checked')).toBe(true);
expect(bNode.hasAttribute('checked')).toBe(false);
expect(cNode.hasAttribute('checked')).toBe(true);

// Now let's run the actual ReactDOMInput change event handler
ReactTestUtils.Simulate.change(bNode);

Expand Down Expand Up @@ -1324,7 +1332,6 @@ describe('ReactDOMInput', () => {
'set property value',
'set attribute value',
'set attribute checked',
'set attribute checked',
]);
});

Expand Down Expand Up @@ -1389,7 +1396,6 @@ describe('ReactDOMInput', () => {
'node.value = "1980-01-01"',
'node.setAttribute("value", "1980-01-01")',
'node.setAttribute("checked", "")',
'node.setAttribute("checked", "")',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so I can better understand this change, is the reduction of this setAttribute call due to the following case?

checked or defaultChecked are true

// <input defaultChecked=true} />

node.defaultChecked = true // node.defaultChecked = !node.defaultChecked
node.defaultChecked = true // node.defaultChecked = !!true

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. Didn't catch your comment earlier. This makes sense!

]);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMFiberInput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ export function postMountWrapper(
node.name = '';
}
node.defaultChecked = !node.defaultChecked;
node.defaultChecked = !node.defaultChecked;
node.defaultChecked = !!node._wrapperState.initialChecked;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just trying to think through all of the cases here:

checked or defaultChecked are true

node.defaultChecked = true // node.defaultChecked = !node.defaultChecked
node.defaultChecked = true // node.defaultChecked = !!false

checked or defaultChecked are false

node.defaultChecked = true  // node.defaultChecked = !node.defaultChecked
node.defaultChecked = false // node.defaultChecked = !!false

checked and defaultChecked are undefined

This can happen if you just declare a regular text input, ex: <input value="test" />

In this case, value detachment still needs to occur. An input might switch from text to radio later, and React doesn't fire the post mount hook behavior when that happens.

node.defaultChecked = true  // node.defaultChecked = !node.defaultChecked
node.defaultChecked = false // node.defaultChecked = !!undefined

So I think this is good to go. Are there any other cases you can think of?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separately, part of me wonders if node._wrapperState.initialChecked should always be a boolean. Like we should do the !!node._wrapperState.initialChecked part when initialChecked is assigned.

However I don't think that's related to this issue, and I'm not sure what existing code relies on that value to be loosely null.

Copy link
Author

@dilidili dilidili Jun 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Haha, i tried and got a Flow type check error.

Cannot assign node._wrapperState.initialChecked to node.defaultChecked because property defaultChecked is missing in object type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. One thing at a time. I think this is a great follow-up, but I'd love to get this check attribute fix in first.

if (name !== '') {
node.name = name;
}
Expand Down