Bug 93544 – Google search query text reverts to original search query after multiple searches

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Brady Eidson]

Google search query text reverts to original search query after multiple searches

1 - Searches google by typing a query into the smart search field of Safari ("WebKit" for example)
2 - On the resulting page of search results, highlight the in page form field and change their search query ("Kittens" for example) and hit enter.
3 - On that resulting page of search results, click the top link.
4 - Then click back.

You're looking at the search results for "Kittens" which is expected, but the form field says "WebKit"

This is because of the following:
-The form field has autocomplete=off
-The html markup for the form field has an original, default value value="WebKit"
-The search results page goes in to the page cache
-When the user clicks "back", we notify the form element and - since it has autocomplete="off", the filled in value "Kittens" is cleared
-Since there is no current filled-in value, WebCore falls back to the default value of "WebKit" which comes from the markup.

From the perspect of "autocomplete=off to avoid form autofill features", the usage makes sense.
But from the perspective of "autocomplete=off to flag a field with sensitive information that should be cleared" - like finanicial institutions do and why we have this rule with the page cache - it seems bizarre to have it applied to a field with a default value.

I proposed that text fields *with* default values should *not* have autocomplete=off force them into clearing the current value when being restored from the page cache.  Patch forthcoming.

In radar as <rdar://problem/10800686>

[Brady Eidson]

Created attachment 157328 [details]
Patch v1 - Fix + layouttest

[Darin Adler]

Comment on attachment 157328 [details]
Patch v1 - Fix + layouttest

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

> Source/WebCore/html/HTMLInputElement.cpp:816
> +    if (m_inputType->storesValueSeparateFromAttribute()) {
> +        // We usually reset values to the empty string to clear out sensitive fields when restoring from the page cache.
> +        // But if the input type is textual with autocomplete=off and the defaultValue is non-empty it doesn't make sense
> +        // to fallback to the default value. So we'll relax the rule in those instances.
> +        if (!m_inputType->isTextType() || getAttribute(autocompleteAttr) != "off" || defaultValue().isEmpty())
> +            setValue(String());
> +    }

This seems like too low a level for this check. I don’t see how this reset function is supposed to know that it’s being called as part of clearing out a sensitive field. It could be used for other purposes. I’d like to see the fix done at another level. Even if there has to be some logic here, it should be driven by what function is being called or by a function argument, not just a guess.

The autocompleteAttr logic here also seems suspect. I’d expect it to be fastGetAttribute rather than getAttribute and I would not expect the string "off" to be hard coded. Does this match how other call sites check the autocomplete attribute?

[Brady Eidson]

Created attachment 157342 [details]
Patch v2 - Much better

[Brady Eidson]

http://trac.webkit.org/changeset/125124

[Kent Tamura]

Comment on attachment 157342 [details]
Patch v2 - Much better

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

> Source/WebCore/html/HTMLInputElement.cpp:1396
> +    bool isSensitive = m_autocomplete == Off && !(m_inputType->isTextType() && !defaultValue().isEmpty());

IMO, this check and InputType::shouldResetOnDocumentActivation() should be integrated to single virtual function of InputType.
Tel, email, url, number, range, date, datetime, datetime-local, month, time, week types should be handled similarly, right?

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 157342 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157342&action=review
> 
> > Source/WebCore/html/HTMLInputElement.cpp:1396
> > +    bool isSensitive = m_autocomplete == Off && !(m_inputType->isTextType() && !defaultValue().isEmpty());
> 
> IMO, this check and InputType::shouldResetOnDocumentActivation() should be integrated to single virtual function of InputType.

That came to mind, and is quite reasonable.

> Tel, email, url, number, range, date, datetime, datetime-local, month, time, week types should be handled similarly, right?

Since this bug represented one specific symptom at one specific site and since this has been a sensitive area in the past I chose to be conservative for this one change set.

I agree with you in principle and think that a separate change set that both moves the check to an InputType virtualized function and expands the scope to many more (all?) input types would be great - It should be in a separate, broader bug.