input.checked is incorrect while we're parsing its children

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

input.checked is incorrect while we're parsing its children.

[Chris Dumez]

Created attachment 351457 [details]
Patch

[Chris Dumez]

Created attachment 351458 [details]
Patch

[Darin Adler]

Comment on attachment 351458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351458&action=review

> Source/WebCore/ChangeLog:13
> +        In this patch, we update the checked state as soon as the 'checked' attribute is set, when we know
> +        that no form state to restore.

What about in the case where there is form state to restore? Do we have a test covering that? Is our behavior already what we would like it to be in that case?

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 351458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351458&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        In this patch, we update the checked state as soon as the 'checked' attribute is set, when we know
> > +        that no form state to restore.
> 
> What about in the case where there is form state to restore? Do we have a
> test covering that? Is our behavior already what we would like it to be in
> that case?

Yes, as I stated in my change log, we have already a test case covering that and it is still passing.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Darin Adler from comment #3)
> > Comment on attachment 351458 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=351458&action=review
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        In this patch, we update the checked state as soon as the 'checked' attribute is set, when we know
> > > +        that no form state to restore.
> > 
> > What about in the case where there is form state to restore? Do we have a
> > test covering that? Is our behavior already what we would like it to be in
> > that case?
> 
> Yes, as I stated in my change log, we have already a test case covering that
> and it is still passing.

And to answer your second question: No, I do not think our behavior is what we would like it to be in the less common state restoration case because it is non-standard behavior. That said, making our behavior standard in the state restoration case while not breaking state restoration is going to be more work and I did not have time to look into this part further. Also note that AFAICT, Blink also does not seem to reflect the correct checked state in the state restoration case, which reduces the risk of Web breakage.

[WebKit Commit Bot]

Comment on attachment 351458 [details]
Patch

Clearing flags on attachment: 351458

Committed r236795: <https://trac.webkit.org/changeset/236795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44975225>

[Chris Dumez]

Comment on attachment 351458 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351458&action=review

> Source/WebCore/html/HTMLInputElement.cpp:-749
> -            m_reflectsCheckedAttribute = true;

I mistakenly dropped this because it looked like a no-op given the if check above. However, the call to setChecked() sets the attribute to false so we do need to reset the flag to true here.
Fixing via https://bugs.webkit.org/show_bug.cgi?id=190651.