Bug 150731 - Implement support for the autocomplete attribute
Summary: Implement support for the autocomplete attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-10-30 13:14 PDT by Jon Honeycutt
Modified: 2015-11-03 14:54 PST (History)
2 users (show)

See Also:


Attachments
Patch (29.28 KB, patch)
2015-10-30 13:57 PDT, Jon Honeycutt
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (777.98 KB, application/zip)
2015-10-30 14:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (816.90 KB, application/zip)
2015-10-30 14:46 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (724.16 KB, application/zip)
2015-10-30 14:53 PDT, Build Bot
no flags Details
Patch (37.63 KB, patch)
2015-11-02 15:30 PST, Jon Honeycutt
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 2015-10-30 13:14:01 PDT
We should implement support for the autocomplete attribute, <https://html.spec.whatwg.org/multipage/forms.html#autofill>.

<rdar://problem/21078968>
Comment 1 Jon Honeycutt 2015-10-30 13:57:19 PDT
Created attachment 264416 [details]
Patch
Comment 2 Build Bot 2015-10-30 14:44:26 PDT
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
Comment 3 Build Bot 2015-10-30 14:44:28 PDT
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
Comment 4 Build Bot 2015-10-30 14:46:33 PDT
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
Comment 5 Build Bot 2015-10-30 14:46:35 PDT
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
Comment 6 Jon Honeycutt 2015-10-30 14:49:51 PDT
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.
Comment 7 Build Bot 2015-10-30 14:53:49 PDT
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
Comment 8 Build Bot 2015-10-30 14:53:52 PDT
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
Comment 9 Darin Adler 2015-10-31 15:06:06 PDT
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.
Comment 10 Jon Honeycutt 2015-11-02 15:30:47 PST
Created attachment 264638 [details]
Patch
Comment 11 Jon Honeycutt 2015-11-02 15:53:36 PST
(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!
Comment 12 Brent Fulgham 2015-11-03 10:01:58 PST
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).
Comment 13 Chris Dumez 2015-11-03 11:02:14 PST
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.
Comment 14 Chris Dumez 2015-11-03 11:11:49 PST
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.
Comment 15 Chris Dumez 2015-11-03 11:21:00 PST
Comment on attachment 264638 [details]
Patch

Isn't the 'autocomplete' attribute supposed to be present on HTMLFormElement as well?
Comment 16 Chris Dumez 2015-11-03 11:34:50 PST
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.
Comment 17 Chris Dumez 2015-11-03 11:39:07 PST
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.
Comment 18 Jon Honeycutt 2015-11-03 12:10:53 PST
(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!
Comment 19 Jon Honeycutt 2015-11-03 13:00:19 PST
(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!
Comment 20 Jon Honeycutt 2015-11-03 13:16:11 PST
(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.
Comment 21 Chris Dumez 2015-11-03 13:22:08 PST
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.
Comment 22 Jon Honeycutt 2015-11-03 13:25:39 PST
(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.
Comment 23 Jon Honeycutt 2015-11-03 14:12:25 PST
(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.
Comment 24 Jon Honeycutt 2015-11-03 14:20:57 PST
(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.
Comment 25 Jon Honeycutt 2015-11-03 14:54:12 PST
Landed in <http://trac.webkit.org/changeset/191981> with Chris’s suggestion to rename parseAutocompleteAttribute to autocomplete().