-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Conversation
There was a problem hiding this 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'); | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
136bea6
to
3e8ca2f
Compare
@@ -242,7 +242,7 @@ export function postMountWrapper( | |||
if (name !== '') { | |||
node.name = ''; | |||
} | |||
node.defaultChecked = !node.defaultChecked; | |||
node.defaultChecked = !node._wrapperState.initialChecked; | |||
node.defaultChecked = !node.defaultChecked; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, 👍
There was a problem hiding this comment.
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;
?
There was a problem hiding this comment.
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
.
b56f29d
to
146bb96
Compare
There was a problem hiding this 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(''); | ||
|
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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", "")', |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
…et on the input
146bb96
to
a3b6d44
Compare
There was a problem hiding this 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?
Did some testing. This looks great. Thanks @dilidili! |
Fix issue #12765