1020496 - API to access to the different components of the "autocomplete" IDL attribute value

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Paolo Amadini]

We might either parse the "autocomplete" attribute into a structured form using a chrome-only interface, or do the same in the JavaScript modules.

Comment 1

[Matthew N. [:MattN]]

Attachment: WIP 1 - Add mozAutocompleteFieldName on HTMLInputElement

Hey Neil,

I have a WIP patch for part of this which I'm attaching. It duplicates some of the logic from GetAutocomplete so I was planning on factoring out a helper that makes it easy to either get the hints or the field name (last token). I was thinking about moving atomCount to be a member of the class (e.g. mAutocompleteTokenCount) and maybe have the helper take start and end offsets to index into the atoms.

e.g. DOMString AutocompleteTokenString([optional] in uint32_t start, [optional] in uint32_t end)

and then GetMozAutocompleteFieldName would call it like:
AutocompleteTokenString(mAutocompleteTokenCount - 1, mAutocompleteTokenCount);

I'm not sure we need individual methods for hints yet as they are much less commonly used (e.g. password manager and satchel will mostly just care about the last token via mozAutocompleteFieldName and not need the hints). Code that cares about hints can just split on spaces.

Comment 2

[Neil Deakin]

Can you please describe what needs to be implemented for this bug? What I thought was intended from the description, and I spent time on, turns out to already be implemented.

Comment 3

[Matthew N. [:MattN]]

Note that in comment 1, replace "hint" with "hint set".

For historical reasons, the autocomplete IDL attribute is a DOMString instead of a DOMTokenList which means that any code (e.g. satchel, password manager, or form auto fill) that wants to use the value would need to split the value on spaces, handle the empty string case and then figure out which type of "autofill hint set" tokens are used (since they are all optional). Having shared code know the individual tokens of the autocomplete IDL attribute is the goal of this bug. Note that we don't currently support the section-* token but we may have to eventually.

Examples:
* foo.autocomplete => "shipping mobile tel"
Field Name: "tel"
Hint Set:   "shipping mobile"
Mode:       "shipping"
Contact:    "mobile"

* foo.autocomplete => "billing tel"
Field Name: "tel"
Hint Set:   "billing"
Mode:       "billing"
Contact:    ""

* foo.autocomplete => "mobile tel"
Field Name: "tel"
Hint Set:   "mobile"
Mode:       ""
Contact:    "mobile"

(btw. I don't like some of the component names from the spec)

Ideally this bug would provide methods or IDL attributes to get the individual components (probably "hint set" isn't needed since you can join the Mode + Contact with a space to get it e.g. [mode, contact].join(" ").
My patch shows the initial direction I was going in to get the field name. There isn't an exact API in mind so suggestions are welcome. Also note that @autocomplete wil eventually be implemented on <select> and <textarea> so majority of the logic shouldn't be on HTMLInputElement.

Comment 4

[Neil Deakin]

The spec mentions the autofill idl attribute which is always a processed version of the autocomplete attribute, although it's not formally defined in the interface section, which I'm assuming is an oversight.

It is always one of the following:
 ""
 on
 off
 section field
 section mode field
 section contact field
 section mode contact field

It seems like it would be simple to implement this and easy to get the specific fields out.

let autofill = element.autofill.split(' ');
let mode = autofill.length > 2 ? autofill[1] : '';

I think it would be more useful though to know what callers will need here and design an api based on that rather than the reverse.

Comment 5

[Neil Deakin]

OK, so the spec is a bit misleading and the autofill attribute is actually the autocomplete attribute, which is already implemented, except for not implementing sections.

So I'm not really sure what extra api is actually needed here. Since there isn't any specific caller need currently, I'd say there is nothing to do here right now.

Comment 6

[Matthew N. [:MattN]]

(In reply to Neil Deakin from comment #4)
> It is always one of the following:
>  ""
>  on
>  off
>  section field
>  section mode field
>  section contact field
>  section mode contact field

section is optional so the last 4 examples could be repeated with section removed.

> It seems like it would be simple to implement this and easy to get the
> specific fields out.
> 
> let autofill = element.autofill.split(' ');
> let mode = autofill.length > 2 ? autofill[1] : '';

This would be incorrect since mode can be the first token.

> I think it would be more useful though to know what callers will need here
> and design an api based on that rather than the reverse.

Callers definitely need to easily get the last token. I already mentioned before that password manager and form manager could use that right away. If there was an attribute to get each type of token individually, we would use that in a few days for requestAutocomplete when we build up the data structure about the form to pass to the requestAutocomplete UI. The current proposal is something like so: https://mattn.pastebin.mozilla.org/5420795 where there is nesting under the section, mode and field tokens or "" if the token is absent.

If we had a getter for each of the tokens this would be really easy to create.

(In reply to Neil Deakin from comment #5)
> OK, so the spec is a bit misleading and the autofill attribute is actually
> the autocomplete attribute, which is already implemented, except for not
> implementing sections.

Right

> So I'm not really sure what extra api is actually needed here. Since there
> isn't any specific caller need currently, I'd say there is nothing to do
> here right now.

Who said there isn't any specific caller need currently? If you just want to implement an attribute for the last token, that would be a good start. Note that bug 1020697 has progress and will be moving some code around. The implementation for this bug should keep <select> and <textarea> in mind. I'm thinking we may want a new interface for autocompletable elements but I don't know how complicated that is. It would have members for the cached state, the AutocompleteCategory, and other information that would make this bug easy to implement without re-parsing.

Comment 7

[Neil Deakin]

Attachment: Add autocompleteFields attribute

I added a autocompleteFields attribute which returns an array of the four fields (or ['on'], ['off'] or []). I included the empty section even though it isn't yet supported.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8442889 [details] [diff] [review]
Add autocompleteFields attribute

Review of attachment 8442889 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take a look more on the plane today.

::: dom/webidl/HTMLInputElement.webidl
@@ +168,5 @@
>  
>    boolean mozIsTextField(boolean aExcludePassword);
> +
> +  [Pure, Cached, Frozen]
> +  readonly attribute sequence<DOMString> autocompleteFields;

I'm not familiar with the various webidl attributes but based on the test it seems like this is accessible to unprivileged content. I think you want to include [ChromeOnly].

Comment 9

[Neil Deakin]

> I'm not familiar with the various webidl attributes but based on the test it
> seems like this is accessible to unprivileged content. I think you want to
> include [ChromeOnly].

I had that originally, but removed it as the test didn't work. But I should be able to get it to work.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8442889 [details] [diff] [review]
Add autocompleteFields attribute

Review of attachment 8442889 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Neil.

(In reply to Neil Deakin from comment #9)
> > I'm not familiar with the various webidl attributes but based on the test it
> > seems like this is accessible to unprivileged content. I think you want to
> > include [ChromeOnly].
> 
> I had that originally, but removed it as the test didn't work. But I should
> be able to get it to work.

This is a mochitest-plain test that is testing the web-developer POV. I would prefer if a separate chrome test was used to test this chrome-only API. It probably doesn't need as many tests for invalid values anyways. It can focus on the data format.

::: content/base/src/nsContentUtils.cpp
@@ -871,5 @@
> - *
> - * @return {AutocompleteAttrState} The state of the attribute (invalid/valid).
> - */
> -nsContentUtils::AutocompleteAttrState
> -nsContentUtils::InternalSerializeAutocompleteAttribute(const nsAttrValue* aAttrVal,

I think the internal method should stay around and the patch would ideally be rebased on bug 1020697 (which bounced) where the string building is done in SerializeAutocompleteAttribute so it can be shared with <select> and <textarea>. The internal method would return the array while the other would return the string.

::: content/html/content/src/HTMLInputElement.h
@@ +367,5 @@
>    {
>      SetHTMLAttr(nsGkAtoms::autocomplete, aValue, aRv);
>    }
>  
> +  void GetAutocompleteFields(nsTArray< nsString >& aArray);

An array is definitely not as nice as a JS object or separate attributes as it's less obvious what autocompleteFields[2] is compared to accessing one of them. Is this approach significantly better than those two? If we end up with an array because the other options are too problematic, I'd at least like consts available from JS that provides names for the indices. Names would include suffixes like section/addressType/contactType/fieldName.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8442889 [details] [diff] [review]
Add autocompleteFields attribute

>+
>+  [Pure, Cached, Frozen]
>+  readonly attribute sequence<DOMString> autocompleteFields;
definitely [ChromeOnly], and does this need to be Pure, Cached attribute?
Wouldn't
[ChromeOnly]
sequence<DOMString> getAutoCompleteFields();
work?
Or is this somehow performance critical?

Comment 12

[Matthew N. [:MattN]]

Olli, did you have a preference on the proposal in the last paragraph of comment 10?

Comment 13

[Neil Deakin]

(In reply to Olli Pettay [:smaug] from comment #11)
> >+  [Pure, Cached, Frozen]
> >+  readonly attribute sequence<DOMString> autocompleteFields;
> definitely [ChromeOnly], and does this need to be Pure, Cached attribute?
> Wouldn't
> [ChromeOnly]
> sequence<DOMString> getAutoCompleteFields();
> work?

Returned sequences have to be marked as such. I don't know the underlying reason.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

No they don't need to, if you return a sequence from a method.
If you have an attribute, then sure.

I think a [ChromeOnly] method would be simplest, assuming there isn't performance reasons to cache
the array.

Comment 15

[Neil Deakin]

(In reply to Matthew N. [:MattN] from comment #10)
> An array is definitely not as nice as a JS object or separate attributes as
> it's less obvious what autocompleteFields[2] is compared to accessing one of
> them. Is this approach significantly better than those two? If we end up
> with an array because the other options are too problematic, I'd at least
> like consts available from JS that provides names for the indices. Names
> would include suffixes like section/addressType/contactType/fieldName.

Only to avoid having several different properties that would likely all do the same parsing. I assumed that most callers would retrieve several or all values at once.

But I still think that this bug should not be implemented until a clearer idea of what callers will be using it.

Comment 16

[Matthew N. [:MattN]]

(In reply to Neil Deakin from comment #15)
> But I still think that this bug should not be implemented until a clearer
> idea of what callers will be using it.

See https://bugzilla.mozilla.org/attachment.cgi?id=8445593&action=diff#a/toolkit/components/formautofill/FormAutofillContentService.js_sec2

This API would replace _parseAutocomplete there.

Password Manager and Form Manager will just want to access the last token which is the field name and won't care about the others.

Comment 17

[Neil Deakin]

Attachment: Add autocompleteFields attribute, v2

This version addresses comments and is built on top of the patch in bug 1020697.

I didn't address whether it should be an array or separate fields. Maybe Olli can provide feedback. I'll re-add the needinfo from comment 12.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

separate fields would be indeed better for api uses. Perhaps some the method should just return a
dictionary which has the right fields.

Comment 19

[:Paolo Amadini]

I also think we'll need separate fields, so that the caller doesn't need to apply much logic itself. There is no need for the fields to handle the "on" and "off" special values.

I'd actually use a getter, something like "autocompleteInfo". No need to cache the result, since this code is definitely not performance-critical.

I'd also make the getter never return null, so callers only need to check whether there is a fieldName, and we can extend the object with other properties that may be present even when no fieldName is specified (for example an "enabled" property).

* element.autocompleteInfo.fieldName

  The type of information stored, or empty string if unspecified.
  Examples: "name", "email", "tel", "cc-name", "tel-country-code".

* element.autocompleteInfo.customSectionName

  The manually-specified section token, including the "section-" prefix.
  This is an empty string if unspecified.

* element.autocompleteInfo.contactType

  The token like "work", "home", "fax", or empty string if unspecified.
  Only applies to the field names that are in the "contact" category.

* element.autocompleteInfo.addressType

  Can be "billing", "shipping", or empty string if unspecified.
  This applies to all fields, even if they don't represent addresses.

Since addressType applies to all fields, I wonder if we should call it something different, like "purpose", "standardSectionName", or something else. The current specification does not have a name for this specific fields, though according to comment 3 it used to be "mode".

Comment 20

[:Paolo Amadini]

By the way, "fieldName" matches the specification, but is also unclear, since we may have many fields in a form with the same "fieldName", but different HTML names, to request the same type of information multiple times or in different sections. I wonder if we should use "informationType", "dataClass", or something along these lines.

Comment 21

[:Paolo Amadini]

See also _getAutocompleteInfo in attachment 8450319 [details] [diff] [review].

Comment 22

[Neil Deakin]

Attachment: Patch using a dictionary

This version returns a dictionary instead of an array.

Comment 23

[:Paolo Amadini]

Comment on attachment 8450989 [details] [diff] [review]
Patch using a dictionary

Looks good to me.

Can we turn getAutocompleteInfo into an autocompleteInfo getter?

Comment 24

[Neil Deakin]

(In reply to :Paolo Amadini from comment #23)
> Can we turn getAutocompleteInfo into an autocompleteInfo getter?

No, as dictionaries can't be the type of an attribute, similar to the above in comments 13/14.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

dictionaries can be used as attributes, but they need to be cached
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Cached
so the cached value needs to be cleared when needed.

I think I'd prefer a method over an attribute.

Comment 26

[:Paolo Amadini]

Neil, is this patch ready for review then?

Comment 27

[Neil Deakin]

Attachment: Patch using a dictionary, v2

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8451682 [details] [diff] [review]
Patch using a dictionary, v2


>+  /* Variation that is used to retrieve a dictionary of the parts of the
>+   * autcomplete attribute.
autocomplete


>--- a/content/base/src/nsContentUtils.cpp	Mon Jul 07 11:06:24 2014 -0400
>+++ b/content/base/src/nsContentUtils.cpp	Mon Jul 07 11:36:53 2014 -0400
>@@ -785,23 +785,73 @@
>     return aCachedState;
>   }
> 
>-  AutocompleteAttrState state = InternalSerializeAutocompleteAttribute(aAttr, aResult);
>+  aResult.Truncate();
>+
>+  nsAutoString enabledState;
>+  mozilla::dom::AutocompleteInfo info;
>+  AutocompleteAttrState state =
>+    InternalSerializeAutocompleteAttribute(aAttr, enabledState, info);
>   if (state == eAutocompleteAttrState_Valid) {
>-    ASCIIToLower(aResult);
>-  } else {
>-    aResult.Truncate();
>-  }
>+    // If enabledState is set, it will be either 'on' or 'off'. Return without
>+    // using any of the other fields.
>+    if (!enabledState.IsEmpty()) {
>+      aResult += enabledState;
>+      return state;
>+    }
>+
>+    // Otherwise, concatenate the other fields.
>+    aResult = info.mSection;
>+
>+    if (!info.mAddressType.IsEmpty()) {
>+      if (!aResult.IsEmpty()) {
>+        aResult += ' ';
>+      }
>+      aResult += info.mAddressType;
>+    }
>+
>+    if (!info.mContactType.IsEmpty()) {
>+      if (!aResult.IsEmpty()) {
>+        aResult += ' ';
>+      }
>+      aResult += info.mContactType;
>+    }
>+
>+    if (!info.mFieldName.IsEmpty()) {
>+      if (!aResult.IsEmpty()) {
>+        aResult += ' ';
>+      }
>+      aResult += info.mFieldName;
>+    }
>+  }
>+
So this setup feels odd. Some serialization happens in Internal and some in non-internal.


>  * Helper to validate the @autocomplete tokens.
>  *
>+ * @param aEnabledState set to 'on', 'off' or '' when other fields are used.
... and it is mainly because of this.
Per spec autofill field name should be "off" in case attribute value is "off".
Why we don't set "off"? Same with "on"
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#processing-model-2

Comment 29

[:Paolo Amadini]

(In reply to Olli Pettay [:smaug] from comment #28)
> Per spec autofill field name should be "off" in case attribute value is
> "off".
> Why we don't set "off"? Same with "on"
> http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-
> controls-and-forms.html#processing-model-2

This doesn't make a lot of sense to us - we'd then need to special-case "on" and "off" in the caller. We're aiming at a simple ChromeOnly interface so that we don't need a JavaScript wrapper over it. We don't need to follow the specification literally. Moreover, as far as I understand it, the specification is pretty much a work in progress at this point.

I'd be fine with calling this attribute something different, like I suggested in comment 20 (and maybe you can figure out a better name than those I wrote there).

Comment 30

[Neil Deakin]

> So this setup feels odd. Some serialization happens in Internal and some in
> non-internal.
> 

What were you suggesting instead? The Internal method returns an array, while this one returns a concatentated string from that array.

> Why we don't set "off"? Same with "on"
> http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-
> controls-and-forms.html#processing-model-2

I think the calling usecase here doesn't want to deal with 'on' and 'off'.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Neil Deakin from comment #30)
> > So this setup feels odd. Some serialization happens in Internal and some in
> > non-internal.
> > 
> 
> What were you suggesting instead? The Internal method returns an array,
> while this one returns a concatentated string from that array.
> 
If "on" and "off" weren't special cased in the Internal, then the setup would be fine.
Internal would just fill the dictionary values, and wouldn't even take inout nsAString param.


> > Why we don't set "off"? Same with "on"
> > http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-
> > controls-and-forms.html#processing-model-2
> 
> I think the calling usecase here doesn't want to deal with 'on' and 'off'.
That sounds surprising. Why doesn't the caller care about all the values?

Comment 32

[Neil Deakin]

Attachment: Patch using a dictionary, v2.1, put on/off in field name

This version puts the on/off value in the field name. The other alternative is to have a fifth dictionary field with either '', 'on' or 'off'.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8452431 [details] [diff] [review]
Patch using a dictionary, v2.1, put on/off in field name

Btw, 
[defaults]
diff = -p -U 8
in .hgrc would make patches easier to read
>@@ -2044,6 +2045,17 @@
>                                  AutocompleteAttrState aCachedState =
>                                    eAutocompleteAttrState_Unknown);
> 
>+  /* Variation that is used to retrieve a dictionary of the parts of the
>+   * autcomplete attribute.
autocomplete

>+    // Otherwise, concatenate the other fields.
I don't understand this comment. "Otherwise" ? and other?
Leftover from the previous patch I guess

Comment 34

[:Paolo Amadini]

This isn't really what we needed, but is still a step forward as it categorizes the tokens, and the JavaScript side can be much simpler.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

What is then needed?

Comment 36

[:Paolo Amadini]

Something like _getAutocompleteInfo in attachment 8450319 [details] [diff] [review], which is basically the same as this patch except we have a field that holds only the autocomplete field name, but not the unrelated "on"/"off" state.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

(I don't understand how "on" and "off" are unrelated.)

Comment 38

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #36)
> Something like _getAutocompleteInfo in attachment 8450319 [details] [diff] [review]
> [review], which is basically the same as this patch except we have a field
> that holds only the autocomplete field name, but not the unrelated
> "on"/"off" state.

I don't have a problem with off/on should in the field name field so that the map reflects the four token positions. It makes it easier to convert existing code that checks .autocomplete or getAttribute("autocomplete") (including for "off") to now check .fieldName.

Comment 39

[Matthew N. [:MattN]]

s/should//

Comment 40

[Neil Deakin]

So is there any objection to this latest patch?

Comment 41

[Matthew N. [:MattN]]

Comment on attachment 8452431 [details] [diff] [review]
Patch using a dictionary, v2.1, put on/off in field name

Review of attachment 8452431 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks!

::: content/base/src/nsContentUtils.cpp
@@ +835,4 @@
>  /**
>   * Helper to validate the @autocomplete tokens.
>   *
> + * @param aEnabledState set to 'on', 'off' or '' when other fields are used.

Leftover @param

Comment 42

[Neil Deakin]

Since the patch here is applied on top of bug 1020697, I'll wait until that is fixed before checking this in.

Comment 43

[:Paolo Amadini]

(In reply to Neil Deakin from comment #42)
> Since the patch here is applied on top of bug 1020697, I'll wait until that
> is fixed before checking this in.

This blocks bug 1020602 and we'd like to wrap up the requestAutocomplete milestone one quickly.

Can you please rebase the patch excluding or importing the changes from bug 1020697? That bug hasn't had any activity in the past week.

Comment 44

[Neil Deakin]

Attachment: Updated patch not based on the other bug

Comment 45

[Neil Deakin]

I can't check this in until next week, nor have I tested a try build so someone else should if needed before then.

Comment 46

[:Paolo Amadini]

(In reply to Neil Deakin from comment #45)
> I can't check this in until next week, nor have I tested a try build so
> someone else should if needed before then.

I can do that!

https://tbpl.mozilla.org/?tree=Try&rev=5dd966e7f6ff

Comment 47

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/a9ccbda97c35

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9ccbda97c35