Bug 150731 – Implement support for the autocomplete attribute

• Component: Forms
• Status: RESOLVED FIXED

[Jon Honeycutt]

We should implement support for the autocomplete attribute, <https://html.spec.whatwg.org/multipage/forms.html#autofill>.

<rdar://problem/21078968>

[Jon Honeycutt]

Created attachment 264416 [details]
Patch

[Build Bot]

Comment on attachment 264416 [details]
Patch

Attachment 264416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/360015

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-autocomplete.html
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 264422 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264416 [details]
Patch

Attachment 264416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/360012

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-autocomplete.html
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 264423 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Jon Honeycutt]

Looking into the test failures. One appears to be a test error in a w3c imported test. I'm going to e-mail the test author.

[Build Bot]

Comment on attachment 264416 [details]
Patch

Attachment 264416 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/360009

New failing tests:
imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-autocomplete.html

[Build Bot]

Created attachment 264424 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 264416 [details]
Patch

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

Code looks good.

Since WebKit itself doesn’t do any autocomplete, I don’t fully understand what adding this accomplishes. Web browsers that embed WebKit such as Safari do autocomplete. But building in a particular set of values in WebKit itself might not do what we want. I’m assuming the patch is a good idea, though.

Need to upload a new patch that passes EWS. Looks like Windows and EFL builds failed.

These tests seem to be failing too:

  imported/w3c/web-platform-tests/html/dom/interfaces.html [ Failure ]
  imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-autocomplete.html [ Failure ]

> Source/WebCore/html/HTMLFormControlElement.cpp:702
> +    if (!attributeValue.length())
> +        return String();

I don’t think we need this. We’ll exit when SpaceSplitString is empty below.

> Source/WebCore/html/HTMLFormControlElement.h:117
> +    void setAutocomplete(const String& value);

Should omit the name “value” here.

[Jon Honeycutt]

Created attachment 264638 [details]
Patch

[Jon Honeycutt]

(In reply to comment #9)
> Comment on attachment 264416 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264416&action=review
> 
> Code looks good.
> 
> Since WebKit itself doesn’t do any autocomplete, I don’t fully understand
> what adding this accomplishes. Web browsers that embed WebKit such as Safari
> do autocomplete. But building in a particular set of values in WebKit itself
> might not do what we want. I’m assuming the patch is a good idea, though.

One intent here is to allow site authors to test whether their sites are spec compliant in WebKit browsers. The other is that by enforcing spec compliance, we can potentially allow WebKit browsers to avoid using heuristic algorithms altogether when a site author has provided a well-formed autocomplete value.

> 
> Need to upload a new patch that passes EWS. Looks like Windows and EFL
> builds failed.

Attempted a build fix.

> 
> These tests seem to be failing too:
> 
>   imported/w3c/web-platform-tests/html/dom/interfaces.html [ Failure ]
>  
> imported/w3c/web-platform-tests/html/semantics/forms/the-form-element/form-
> autocomplete.html [ Failure ]

I rebased these.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:702
> > +    if (!attributeValue.length())
> > +        return String();
> 
> I don’t think we need this. We’ll exit when SpaceSplitString is empty below.

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.h:117
> > +    void setAutocomplete(const String& value);
> 
> Should omit the name “value” here.

Fixed.

Thanks for the review!

[Brent Fulgham]

Comment on attachment 264638 [details]
Patch

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

r=me.

> Source/WebCore/html/HTMLFormControlElement.cpp:706
> +    if (!tokens.size())

This should be "if (tokens.isEmpty())"

> Source/WebCore/html/HTMLFormControlElement.cpp:707
> +        return String();

Should we be using "emptyString()" for all of these blank string returns?

> Source/WebCore/html/HTMLFormControlElement.cpp:713
> +        return String();

Ditto. (etc. for rest of method).

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

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

> Source/WebCore/html/HTMLFormControlElement.cpp:614
> +        map.get().add("off", AutofillCategory::Off);

We could use ASCIILiteral("off") for the first parameter. ditto below.

> Source/WebCore/html/HTMLFormControlElement.cpp:673
> +    if (map.get().contains(token))

It would be nice to factor this so we don't 1 hash lookup instead of 2 (probably by using find()).

> Source/WebCore/html/HTMLFormControlElement.cpp:679
> +static inline unsigned maxTokensForAutofillFieldCategory(enum AutofillCategory category)

We don't need the "enum"

> Source/WebCore/html/HTMLFormControlElement.cpp:695
> +    default:

We don't need the default here. This way we get a build error if we forget to handle a value, instead of a runtime assertion hit.

> Source/WebCore/html/HTMLFormControlElement.cpp:696
> +        ASSERT_NOT_REACHED();

We can just move these 2 lines after the switch.

> Source/WebCore/html/HTMLFormControlElement.cpp:723
> +        return "off";

ASCIILiteral("off")

> Source/WebCore/html/HTMLFormControlElement.cpp:726
> +        return "on";

ASCIILiteral("on")

> Source/WebCore/html/HTMLFormControlElement.cpp:728
> +    String result = fieldToken;

Looks like we could use a StringBuilder for result.

> LayoutTests/fast/forms/autocomplete-tokens.html:26
> +                shouldBe('input.autocomplete', '"' + keyword + '"');

This test should probably be using shouldBeEqualToString() more.

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

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

>> Source/WebCore/html/HTMLFormControlElement.cpp:673
>> +    if (map.get().contains(token))
> 
> It would be nice to factor this so we don't 1 hash lookup instead of 2 (probably by using find()).

Actually, I am pretty sure that "return map.get().contains(token);" would do the right thing and return AutofillCategory::Invalid (0) when the key is not in the hash map.

> Source/WebCore/html/HTMLTextAreaElement.h:65
> +    String autocomplete() const { return parseAutocompleteAttribute(); }

For the C++ side, why don't we simply define "String autocomplete() const;" on HTMLFormControlElement instead of on subclasses? We could just rename parseAutocompleteAttribute() to autocomplete() and call it a day.

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

Isn't the 'autocomplete' attribute supposed to be present on HTMLFormElement as well?

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

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

> Source/WebCore/html/HTMLFormControlElement.cpp:597
> +static inline bool isContactToken(const String& token)

token is actually an AtomicString so I think it would be nice to use AtomicString type here.

> Source/WebCore/html/HTMLFormControlElement.cpp:610
> +static inline AutofillCategory categoryForAutofillFieldToken(const String& token)

token is actually an AtomicString so I think it would be nice to use AtomicString type here.

> Source/WebCore/html/HTMLFormControlElement.cpp:612
> +    static NeverDestroyed<HashMap<String, AutofillCategory>> map;

We may want to use AtomicString for the key.

>> Source/WebCore/html/HTMLFormControlElement.cpp:614
>> +        map.get().add("off", AutofillCategory::Off);
> 
> We could use ASCIILiteral("off") for the first parameter. ditto below.

If we use AtomicString for the key, I think we would use AtomicString("off", ConstructFromLiteral) instead.

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

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

> LayoutTests/fast/forms/autocomplete-tokens.html:7
> +        <p id="description"></p>

Not needed.

> LayoutTests/fast/forms/autocomplete-tokens.html:8
> +        <div id="console"></div>

Not needed.

> LayoutTests/fast/forms/autocomplete-tokens.html:93
> +            shouldBe('input.autocomplete', '""');

We have a shouldBeEmptyString() for these sorts of checks.

[Jon Honeycutt]

(In reply to comment #12)
> Comment on attachment 264638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264638&action=review
> 
> r=me.
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:706
> > +    if (!tokens.size())
> 
> This should be "if (tokens.isEmpty())"

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:707
> > +        return String();
> 
> Should we be using "emptyString()" for all of these blank string returns?
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:713
> > +        return String();
> 
> Ditto. (etc. for rest of method).

The caller handles this in a way that makes it a little more efficient to return String().

Thanks for the review!

[Jon Honeycutt]

(In reply to comment #13)
> Comment on attachment 264638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264638&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:614
> > +        map.get().add("off", AutofillCategory::Off);
> 
> We could use ASCIILiteral("off") for the first parameter. ditto below.
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:673
> > +    if (map.get().contains(token))
> 
> It would be nice to factor this so we don't 1 hash lookup instead of 2
> (probably by using find()).

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:679
> > +static inline unsigned maxTokensForAutofillFieldCategory(enum AutofillCategory category)
> 
> We don't need the "enum"

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:695
> > +    default:
> 
> We don't need the default here. This way we get a build error if we forget
> to handle a value, instead of a runtime assertion hit.
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:696
> > +        ASSERT_NOT_REACHED();
> 
> We can just move these 2 lines after the switch.

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:723
> > +        return "off";
> 
> ASCIILiteral("off")
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:726
> > +        return "on";
> 
> ASCIILiteral("on")

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:728
> > +    String result = fieldToken;
> 
> Looks like we could use a StringBuilder for result.

Subsequent tokens are prepended to the result, rather than appended. It doesn't look like StringBuilder handles this.

> 
> > LayoutTests/fast/forms/autocomplete-tokens.html:26
> > +                shouldBe('input.autocomplete', '"' + keyword + '"');
> 
> This test should probably be using shouldBeEqualToString() more.

Fixed.

Thanks for looking at this!

[Jon Honeycutt]

(In reply to comment #14)
> Comment on attachment 264638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264638&action=review
> 
> >> Source/WebCore/html/HTMLFormControlElement.cpp:673
> >> +    if (map.get().contains(token))
> > 
> > It would be nice to factor this so we don't 1 hash lookup instead of 2 (probably by using find()).
> 
> Actually, I am pretty sure that "return map.get().contains(token);" would do
> the right thing and return AutofillCategory::Invalid (0) when the key is not
> in the hash map.
> 

That does appear to be the case. I made this change.

> > Source/WebCore/html/HTMLTextAreaElement.h:65
> > +    String autocomplete() const { return parseAutocompleteAttribute(); }
> 
> For the C++ side, why don't we simply define "String autocomplete() const;"
> on HTMLFormControlElement instead of on subclasses? We could just rename
> parseAutocompleteAttribute() to autocomplete() and call it a day.

Not all HTMLFormControlElements should have this behavior. Controls like "button" should have the default behavior when getting the value of their autocomplete attribute. I thought it would be confusing then if this were named "autocomplete" but only applied to some subclasses.

[Chris Dumez]

Comment on attachment 264638 [details]
Patch

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

>>> Source/WebCore/html/HTMLTextAreaElement.h:65
>>> +    String autocomplete() const { return parseAutocompleteAttribute(); }
>> 
>> For the C++ side, why don't we simply define "String autocomplete() const;" on HTMLFormControlElement instead of on subclasses? We could just rename parseAutocompleteAttribute() to autocomplete() and call it a day.
> 
> Not all HTMLFormControlElements should have this behavior. Controls like "button" should have the default behavior when getting the value of their autocomplete attribute. I thought it would be confusing then if this were named "autocomplete" but only applied to some subclasses.

Does HTMLButtonElement really have an autocomplete attribute? I don't see any.

[Jon Honeycutt]

(In reply to comment #15)
> Comment on attachment 264638 [details]
> Patch
> 
> Isn't the 'autocomplete' attribute supposed to be present on HTMLFormElement
> as well?

Yes, the autocomplete attribute is already present on HTMLFormElement. Its behavior is different from what this patch adds, though.

[Jon Honeycutt]

(In reply to comment #21)
> Comment on attachment 264638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264638&action=review
> 
> >>> Source/WebCore/html/HTMLTextAreaElement.h:65
> >>> +    String autocomplete() const { return parseAutocompleteAttribute(); }
> >> 
> >> For the C++ side, why don't we simply define "String autocomplete() const;" on HTMLFormControlElement instead of on subclasses? We could just rename parseAutocompleteAttribute() to autocomplete() and call it a day.
> > 
> > Not all HTMLFormControlElements should have this behavior. Controls like "button" should have the default behavior when getting the value of their autocomplete attribute. I thought it would be confusing then if this were named "autocomplete" but only applied to some subclasses.
> 
> Does HTMLButtonElement really have an autocomplete attribute? I don't see
> any.

No, it doesn't. What I meant was, if you have a control that you know inherits from HTMLFormControlElement, and you access its autocomplete attribute, I didn't want it to be unclear what would be returned. Maybe what I've written doesn't make this more clear, though.

[Jon Honeycutt]

(In reply to comment #16)
> Comment on attachment 264638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264638&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:597
> > +static inline bool isContactToken(const String& token)
> 
> token is actually an AtomicString so I think it would be nice to use
> AtomicString type here.

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:610
> > +static inline AutofillCategory categoryForAutofillFieldToken(const String& token)
> 
> token is actually an AtomicString so I think it would be nice to use
> AtomicString type here.

Fixed.

> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:612
> > +    static NeverDestroyed<HashMap<String, AutofillCategory>> map;
> 
> We may want to use AtomicString for the key.
> 

Fixed.

> >> Source/WebCore/html/HTMLFormControlElement.cpp:614
> >> +        map.get().add("off", AutofillCategory::Off);
> > 
> > We could use ASCIILiteral("off") for the first parameter. ditto below.
> 
> If we use AtomicString for the key, I think we would use AtomicString("off",
> ConstructFromLiteral) instead.

Fixed.

[Jon Honeycutt]

Landed in <http://trac.webkit.org/changeset/191981> with Chris’s suggestion to rename parseAutocompleteAttribute to autocomplete().