Bug 36762 – Forms with autocomplete=off should not consume saved state

• Component: Forms
• Status: RESOLVED FIXED

This was reported in bug 23346 first, and Kent had patched in and attached a patch in bug 23346. But since 23346 describes multiple bugs, I am creating this new bug to keep track of this particular item.

The bug: autocomplete forms currently consume saved state, when WebKit is re-filling forms with previous values (on user back button action). They did not produce saved states earlier.

[Kent Tamura]

Created attachment 52132 [details]
Patch

[Darin Adler]

Comment on attachment 52132 [details]
Patch

> -    // Assuming we're still in a Form, respect the Form's setting
> -    if (HTMLFormElement* form = this->form())
> -        return form->autoComplete();
> -    
> -    // The default is true
> -    return true;
> +    return HTMLFormControlElementWithState::autoComplete();

The change log does not mention this change.

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 52132 [details])
> > -    // Assuming we're still in a Form, respect the Form's setting
> > -    if (HTMLFormElement* form = this->form())
> > -        return form->autoComplete();
> > -    
> > -    // The default is true
> > -    return true;
> > +    return HTMLFormControlElementWithState::autoComplete();
> 
> The change log does not mention this change.

I think ChangeLog mentions it.  The behavior of this function won't be changed.
>         * html/HTMLInputElement.cpp:
>        (WebCore::HTMLInputElement::autoComplete):
>          Use HTMLFormControlElementWithState::autoComplete().

[Darin Adler]

Comment on attachment 52132 [details]
Patch

>  void HTMLFormControlElementWithState::finishParsingChildren()
>  {
>      HTMLFormControlElement::finishParsingChildren();
> +    if (!autoComplete())
> +        return;
>      Document* doc = document();
>      if (doc->hasStateForNewFormElements()) {
>          String state;

This code change needs a comment. It's not at all clear why autoComplete has anything to do with restoring state; people should not have to look at change history to find out why. The comment will be a "why" comment.

>  {
>      if (m_autocomplete != Uninitialized)
>          return m_autocomplete == On;
> -    
> -    // Assuming we're still in a Form, respect the Form's setting
> -    if (HTMLFormElement* form = this->form())
> -        return form->autoComplete();
> -    
> -    // The default is true
> -    return true;
> +    return HTMLFormControlElementWithState::autoComplete();
>  }

The normal idiom when calling through to the base class is to name the immediate base. So you should write this as "return HTMLTextFormControlElement::autoComplete()".

>  bool HTMLSelectElement::saveFormControlState(String& value) const
>  {
> +    if (!autoComplete())
> +        return false;
>      return SelectElement::saveFormControlState(m_data, this, value);
>  }

Needs a "why" comment. But also, I suggest refactoring so you can put this logic into HTMLFormControlElementWithState. We don't want the implementers of each type of form control to have to get this right. And we also don't want the rules about saving to be in a different class than the rules about restoring.

A good way to do that would be to change the type of the argument to registerFormElementWithState to HTMLFormControlElementWithState*. Then change that function to call a non-virtual HTMLFormControlElementWithState function that checks autoComplete and then turns around and calls saveFormControlState. And you could move the saveFormControlState function from Element down into HTMLFormControlElementWithState.

>  bool HTMLTextAreaElement::saveFormControlState(String& result) const
>  {
> +    if (!autoComplete())
> +        return false;
>      result = value();
>      return true;
>  }

Needs a "why" comment.

review- because of the comments, but I think the change with a bit of cleanup would be more foolproof. Lets put all the responsibility for deciding how to handle state into a single place

[Kent Tamura]

Created attachment 52382 [details]
Proposed patch (rev.2)

[Kent Tamura]

Thank you for reviewing.

(In reply to comment #4)
> (From update of attachment 52132 [details])
> >  void HTMLFormControlElementWithState::finishParsingChildren()
> >  {
> >      HTMLFormControlElement::finishParsingChildren();
> > +    if (!autoComplete())
> > +        return;
> 
> This code change needs a comment. It's not at all clear why autoComplete has
> anything to do with restoring state; people should not have to look at change
> history to find out why. The comment will be a "why" comment.

Added a comment.

> The normal idiom when calling through to the base class is to name the
> immediate base. So you should write this as "return
> HTMLTextFormControlElement::autoComplete()".

It was just a mistake.  Fixed.

> 
> >  bool HTMLSelectElement::saveFormControlState(String& value) const
> >  {
> > +    if (!autoComplete())
> > +        return false;
> >      return SelectElement::saveFormControlState(m_data, this, value);
> >  }
> 
> Needs a "why" comment. But also, I suggest refactoring so you can put this
> logic into HTMLFormControlElementWithState. We don't want the implementers of
> each type of form control to have to get this right. And we also don't want the
> rules about saving to be in a different class than the rules about restoring.
> 
> A good way to do that would be to change the type of the argument to
> registerFormElementWithState to HTMLFormControlElementWithState*. Then change
> that function to call a non-virtual HTMLFormControlElementWithState function
> that checks autoComplete and then turns around and calls saveFormControlState.
> And you could move the saveFormControlState function from Element down into
> HTMLFormControlElementWithState.

I understand your advice.  But we can't do it because WML Form also uses this save/restore infrastructure, and we don't have a common parent class of HTMLFormControlElement and WMLFormControlElement. If you think we should introduce such common parent, I'll make another patch.

So, I introduced Element::shouldSaveAndRestoreFormControlState().

[WebKit Commit Bot]

Comment on attachment 52382 [details]
Proposed patch (rev.2)

Clearing flags on attachment: 52382

Committed r57012: <http://trac.webkit.org/changeset/57012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.