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.

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

Conversation

dilidili
Copy link

Fix issue #12765

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

Thanks for sending this in!

I want to see if we can simplify the DOM operations by directly assigning our pre-computed initial checked value to node.defaultValue. Is this something you could look into?

// Set the "checked" attribute initially.
if (props.hasOwnProperty('defaultChecked') && !!props.defaultChecked) {
node.setAttribute('checked', 'checked');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if lines 245-250 could be replaced with:

node.defaultChecked = node._wrapperState.initialChecked

Copy link
Author

@dilidili dilidili Jun 28, 2018

Choose a reason for hiding this comment

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

You are right. The node._wrapperState.initialChecked will simplify the DOM operations.

@@ -242,7 +242,7 @@ export function postMountWrapper(
if (name !== '') {
node.name = '';
}
node.defaultChecked = !node.defaultChecked;
node.defaultChecked = !node._wrapperState.initialChecked;
node.defaultChecked = !node.defaultChecked;
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about reversing these lines? I have a hard time tracking lots of boolean flipping. Do you think this would work?

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.

Remind me again why we set defaultChecked twice, is it related to value detachment? It'd be great if we could add a comment here explaining the reasoning.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aweary Actually I don't know either, but I aired on the side of avoiding speculation, haha. I can do some testing to see if we really need this.

A lot of the value detachment stuff was added when defaultValue was assigned as an attribute through DOMPropertyOperations. I think those edge cases went away when we moved everything into ReactDOMInput.

The thing to figure out is if assignment detaches, or if the value truly must be different. It would be easy to run a few tests.

Copy link
Author

Choose a reason for hiding this comment

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

I think we set defaultChecked twice for solving Checkbox checked state is erroneously influenced by value of defaultChecked in Chrome.

test:

+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+</head>
+<body>
+<script>
+test (function() {
+    var el = document.createElement("input");
+    el.setAttribute("type", "checkbox");
+    el.defaultChecked = true;
+    el.checked = true;
+    el.defaultChecked = false;
+    assert_true(el.checked);
+}, "This test check that checked state of checkbox once explicitly set is not affected by default state.");
+</script>
+</body>
+</html>

diff:

void HTMLInputElement::setChecked(bool nowChecked, TextFieldEventBehavior eventBehavior)
 {
+    m_reflectsCheckedAttribute = false;
     if (checked() == nowChecked)
         return;
 
-    m_reflectsCheckedAttribute = false;
     m_isChecked = nowChecked;
 
     if (RadioButtonGroupScope* scope = radioButtonGroupScope())

The purpose of the first assignment is set the m_reflectsCheckedAttribute to false. So, both of our codes work.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good enough answer for me, 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Still, just to keep the boolean flipping in check, is it possible to do this:

node.defaultChecked = !node.defaultChecked;
node.defaultChecked = node._wrapperState.initialChecked;

?

Copy link
Author

Choose a reason for hiding this comment

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

@nhunzaker updated.

And i changed test case because of which i found:
if a <input /> is neither checkbox nor radio, input.defaultChecked = true will call input.setAttribute('checked', '') but input.getAttribute('checked') still be null. Thereafter, input.defaultChecked = false will not call input.setAttribute on the uncheckable input.

@dilidili dilidili force-pushed the defaultCheckedProps branch 3 times, most recently from b56f29d to 146bb96 Compare June 29, 2018 02:58
Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

This looks good to me. I just left a final change requesting the use of hasAttribute over comparing getAttribute.

Thanks for sending this in!

expect(aNode.getAttribute('checked')).toBe('');
expect(bNode.getAttribute('checked')).toBe(null);
expect(cNode.getAttribute('checked')).toBe('');

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I think one last change. This looks good overall.

Can you change this test to use hasAttribute instead of doing a value check on getAttribute? Like:

expect(aNode.hasAttribute('checked')).toBe(true);

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense, Updated.

@@ -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

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.

@@ -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!

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

This looks great from my end. I'll do some manual QA just to feel things out across a few browsers, but this is good as far as I'm concerned.

@aweary any thoughts?

@nhunzaker
Copy link
Contributor

Did some testing. This looks great.

Thanks @dilidili!

@nhunzaker nhunzaker merged commit 85fe4dd into facebook:master Jul 4, 2018
@gaearon gaearon mentioned this pull request Aug 7, 2018
@gaearon gaearon mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants