We should implement support for the autocomplete attribute, <https://html.spec.whatwg.org/multipage/forms.html#autofill>. <rdar://problem/21078968>
Created attachment 264416 [details] Patch
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
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 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
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
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 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
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 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.
Created attachment 264638 [details] Patch
(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 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 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 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 on attachment 264638 [details] Patch Isn't the 'autocomplete' attribute supposed to be present on HTMLFormElement as well?
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 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.
(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!
(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!
(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 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.
(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.
(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.
(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.
Landed in <http://trac.webkit.org/changeset/191981> with Chris’s suggestion to rename parseAutocompleteAttribute to autocomplete().