Radio button group state is not restored correctly

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Radio button group state is not restored correctly if non-first radio button has "checked" attribute.
e.g.
A page has
<input type=radio name=foo value=1>
<input type=radio name=foo value=2 checkd>

A user clicks the first radio button, then remove the tab, and load the page again.
Expected: the first radio button is checked, and the second is not checked.
Actual: Both are not checked.

* What happened?
Document::m_stateForNewFormElements has correct values and HTMLInputElement::restoreFormControlState() is called with correct values.
However, It doesn't work well for radio buttons during parsing the document.

Steps during parsing and restoring:
1. The first input tag is parsed.
  It has no "checked" attribute.
2. HTMLFormControlElement::finishParsingChildren() is called for the first input.
 The state is restored. The radio button becomes "on".
3. The second input tag is parsed.
  It has "checked" attribute. So, this radio button becomes "on", and the first radio button becomes "off". ******
4. HTMLFormControlEement::finishParsingChildren() is called for the second input.
  The state is restored.  The radio button becomes "off".

I have no idea how to fix this bug at this moment.

[Darin Adler]

(In reply to comment #0)
> 3. The second input tag is parsed.
>   It has "checked" attribute. So, this radio button becomes "on", and the first radio button becomes "off". ******
> 4. HTMLFormControlEement::finishParsingChildren() is called for the second input.
>   The state is restored.  The radio button becomes "off".
> 
> I have no idea how to fix this bug at this moment.

During step (3), the control is in a position to know that it’s being created by the parser and has not yet finished parsing its children. We can set constructorNeedsCreatedByParser in HTMLTagNames.in and set a flag. If that flag is set, we can defer whatever work we need to until finishParsingChildren time.

[Kent Tamura]

(In reply to comment #1)
> > I have no idea how to fix this bug at this moment.
> 
> During step (3), the control is in a position to know that it’s being created by the parser and has not yet finished parsing its children. We can set constructorNeedsCreatedByParser in HTMLTagNames.in and set a flag. If that flag is set, we can defer whatever work we need to until finishParsingChildren time.

Thank you for the advice!  constructorNeedsCreatedByParser worked well.

[Kent Tamura]

Created attachment 76493 [details]
Patch

[Darin Adler]

Comment on attachment 76493 [details]
Patch

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

Looks great; a few comments.

> LayoutTests/fast/forms/state-restore-radio-group.html:31
> +        // Submit form in a timeout to make sure that we create a new back/forward list item.            

Disappointing that it has to work like this. It really would be better to have another way to make this kind of test work.

> WebCore/html/HTMLFormControlElement.h:175
> +    bool stateRestored() const { return m_stateRestored; }

Given that this function and data member are only used in HTMLInputElement, it is unpleasant we had to add them to HTMLFormControlElement. Since the restoreFormControlState function is virtual, I see no reason this couldn’t have been kept entirely within the HTMLInputElement class; I suggest you move it there.

> WebCore/html/HTMLInputElement.cpp:714
> +        // If m_createdByParser, delay the setChecked() call in order to avoid
> +        // breaking resotred state of other radio buttons in the group.

This comment is a bit too mechanical; saying “if createdByParser” is unclear. It says that delaying the call will “avoid breaking” restored state of other radio buttons. But delay until then.

We do not want to call setChecked until we have restored saved form state, right. I think that’s the real issue. So the comment needs to explain why this logic helps that happen.

> WebCore/html/HTMLInputElement.cpp:798
> +    m_createdByParser = false;

Since we are resetting this to false once we are done parsing, it’s not a “created by parser” flag at all. It’s a “parsing in progress” flag, and so needs a name that means that. I’d also like to know if we can keep the number of new boolean data members down to one rather than needing a flag for this and another for “state was restored”. Maybe that’s not possible; probably not worth wasting a lot of time on that.

> WebCore/html/HTMLInputElement.cpp:803
> +    if (!stateRestored()) {
> +        setChecked(m_defaultChecked);
> +        m_useDefaultChecked = true;
> +    }

It seems a little strange to do this unconditionally for all input elements even if they don’t have a checked attribute. Not great for performance either. Maybe we should do this only if m_defaultChecked?

As a side note, the m_defaultChecked data member is not helpful at all! It’s just a cached copy of hasAttribute(checkedAttr). We should get rid of it and just call hasAttribute(checkedAttr) as needed.

> WebCore/html/HTMLInputElement.cpp:926
> +    setNeedsValidityCheck();

The change log has no explanation of this fix and no mention other than listing this function name. Is this a separate bug fix? Is it covered by the test? Does it need a separate test?

> WebCore/html/HTMLInputElement.h:214
> -    HTMLInputElement(const QualifiedName&, Document*, HTMLFormElement* = 0);
> +    HTMLInputElement(const QualifiedName&, Document*, HTMLFormElement* = 0, bool createdByParser = false);

This seems wrong. Classes derived from HTMLInputElement are going to set createdByParser to false, even in cases where they are created by the parser. Having the wrong value for createdByParser might be harmless now, but it’s not a good situation.

[Kent Tamura]

Created attachment 78098 [details]
Patch 2

[Kent Tamura]

Comment on attachment 76493 [details]
Patch

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

Thank you for reviewing!

>> WebCore/html/HTMLFormControlElement.h:175
>> +    bool stateRestored() const { return m_stateRestored; }
> 
> Given that this function and data member are only used in HTMLInputElement, it is unpleasant we had to add them to HTMLFormControlElement. Since the restoreFormControlState function is virtual, I see no reason this couldn’t have been kept entirely within the HTMLInputElement class; I suggest you move it there.

ok, I moved the flag to HTMLInputElement.

>> WebCore/html/HTMLInputElement.cpp:714
>> +        // breaking resotred state of other radio buttons in the group.
> 
> This comment is a bit too mechanical; saying “if createdByParser” is unclear. It says that delaying the call will “avoid breaking” restored state of other radio buttons. But delay until then.
> 
> We do not want to call setChecked until we have restored saved form state, right. I think that’s the real issue. So the comment needs to explain why this logic helps that happen.

I improved the comment.

>> WebCore/html/HTMLInputElement.cpp:798
>> +    m_createdByParser = false;
> 
> Since we are resetting this to false once we are done parsing, it’s not a “created by parser” flag at all. It’s a “parsing in progress” flag, and so needs a name that means that. I’d also like to know if we can keep the number of new boolean data members down to one rather than needing a flag for this and another for “state was restored”. Maybe that’s not possible; probably not worth wasting a lot of time on that.

Renamed it to m_parsingInProgress.

>> WebCore/html/HTMLInputElement.cpp:803
>> +    }
> 
> It seems a little strange to do this unconditionally for all input elements even if they don’t have a checked attribute. Not great for performance either. Maybe we should do this only if m_defaultChecked?
> 
> As a side note, the m_defaultChecked data member is not helpful at all! It’s just a cached copy of hasAttribute(checkedAttr). We should get rid of it and just call hasAttribute(checkedAttr) as needed.

Done.

>> WebCore/html/HTMLInputElement.cpp:926
>> +    setNeedsValidityCheck();
> 
> The change log has no explanation of this fix and no mention other than listing this function name. Is this a separate bug fix? Is it covered by the test? Does it need a separate test?

This was moved from parseMappedAttribute().
We need to call setNeedsValidityCheck() whenever the value is changed. So we had to add it in HTMLInputElement::finishParsingChildren().  Then, I found adding setNeedsValidityCheck() to setChecked() was better than calling setNeedsValidityCheck() after every setChecked().
If we don't add setNeedsValidityCheck(), an existing test crashes by an assertion failure.

>> WebCore/html/HTMLInputElement.h:214
>> +    HTMLInputElement(const QualifiedName&, Document*, HTMLFormElement* = 0, bool createdByParser = false);
> 
> This seems wrong. Classes derived from HTMLInputElement are going to set createdByParser to false, even in cases where they are created by the parser. Having the wrong value for createdByParser might be harmless now, but it’s not a good situation.

ok.
I used the default parameter because we can't add new parameter after HTMLFormElement*=0 without the default parameter. I removed the default parameter of HTMLFormElement*.

[Dimitri Glazkov (Google)]

Comment on attachment 78098 [details]
Patch 2

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

ok with a nit.

> WebCore/html/HTMLInputElement.cpp:652
> +    if (!m_stateRestored) {

could be just an early return here.

[Kent Tamura]

Landed with the following fix: http://trac.webkit.org/changeset/76664

(In reply to comment #7)
> > WebCore/html/HTMLInputElement.cpp:652
> > +    if (!m_stateRestored) {
> 
> could be just an early return here.

Fixed.

[Darin Adler]

Kent, I found some ways to simplify the patch. I’ll submit a new patch soon that streamlines things a bit.