Make dom.forms.autocomplete.formautofill reflect actually supported autocomplete tokens

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 attachment)

Bug 1386120 toggled dom.forms.autocomplete.formautofill when autofill is enabled but didn't check if dom.forms.autocomplete.formautofill enables the appropriate tokens.

While looking at bug 1386120 comment 13 I noticed some issues:
* cc-type isn't going to be supported for the MVP and therefore should be removed

and some questions:
* are we going to support all of the tel-* combinations?
* Are we supposed to support "locality" and "region"? We should look at other browsers and make sure the spec and wpt tests are up-to-date
* Are we going to support cc-exp as one field?

Needinfo'ing Luke and Sean for answers to the above.
Flags: needinfo?(selee)
Flags: needinfo?(lchang)
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8894010 [details]
Bug 1387634 - Update unsupported @autocomplete token list to reflect the autofill MVP.

https://reviewboard.mozilla.org/r/165090/#review170586

rs+
Attachment #8894010 - Flags: review?(bugs) → review+
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #0)
> * are we going to support all of the tel-* combinations?

We support all tel-* except tel-extension.

> * Are we supposed to support "locality" and "region"? We should look at
> other browsers and make sure the spec and wpt tests are up-to-date

I didn't know there were "locality" and "region" in @autocomplete spec and it surprises me that Chrome supports them. Do you think we should support it, too?

> * Are we going to support cc-exp as one field?

Yes, we are going to support it in bug 1371145.
Flags: needinfo?(selee)
Flags: needinfo?(lchang)
(In reply to Luke Chang [:lchang] from comment #3)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #0)
> > * are we going to support all of the tel-* combinations?
> 
> We support all tel-* except tel-extension.

OK, I wasn't sure we weren't going to know how to split phone numbers into the prefix and suffix properly.

> > * Are we supposed to support "locality" and "region"? We should look at
> > other browsers and make sure the spec and wpt tests are up-to-date
> 
> I didn't know there were "locality" and "region" in @autocomplete spec and
> it surprises me that Chrome supports them. Do you think we should support
> it, too?

They aren't in the spec but I think are leftover from Chrome's attribute tokens from before standardization. In my commit I'm removing the leftover metadata from the deleted wpt. Are you saying you tested and they still support them?

> > * Are we going to support cc-exp as one field?
> 
> Yes, we are going to support it in bug 1371145.

Thanks
Flags: needinfo?(lchang)

Comment 5

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/5f177c216666
Update unsupported @autocomplete token list to reflect the autofill MVP. r=smaug
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> (In reply to Luke Chang [:lchang] from comment #3)
> > We support all tel-* except tel-extension.
> 
> OK, I wasn't sure we weren't going to know how to split phone numbers into
> the prefix and suffix properly.

We hard-coded a parser for US numbers only for now.

> > I didn't know there were "locality" and "region" in @autocomplete spec and
> > it surprises me that Chrome supports them. Do you think we should support
> > it, too?
> 
> They aren't in the spec but I think are leftover from Chrome's attribute
> tokens from before standardization. In my commit I'm removing the leftover
> metadata from the deleted wpt. Are you saying you tested and they still
> support them?

Yes, I tested it on macOS today. They still support them.
Flags: needinfo?(lchang)

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f177c216666
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Luke Chang [:lchang] from comment #6)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #4)
> > (In reply to Luke Chang [:lchang] from comment #3)
> > > We support all tel-* except tel-extension.
> > 
> > OK, I wasn't sure we weren't going to know how to split phone numbers into
> > the prefix and suffix properly.
> 
> We hard-coded a parser for US numbers only for now.

OK, I couldn't remember what that code looked like.

> > > I didn't know there were "locality" and "region" in @autocomplete spec and
> > > it surprises me that Chrome supports them. Do you think we should support
> > > it, too?
> > 
> > They aren't in the spec but I think are leftover from Chrome's attribute
> > tokens from before standardization. In my commit I'm removing the leftover
> > metadata from the deleted wpt. Are you saying you tested and they still
> > support them?
> 
> Yes, I tested it on macOS today. They still support them.

Hmm… I think we shouldn't for now since they're non-standard but we can re-evaluate if sites rely on it, in which case we may get it added to the spec. Ideally for now we would write wpt tests to fail if those 2 tokens are considered valid.
Comment on attachment 8894010 [details]
Bug 1387634 - Update unsupported @autocomplete token list to reflect the autofill MVP.

Approval Request Comment
[Feature/Bug causing the regression]: Form autofill
[User impact if declined]: Sites cannot reliably detect form autofill field support
[Is this code covered by automated tests?]: yes, wpt
[Has the fix been verified in Nightly?]: Yes, by myself
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: trivial list change
[String changes made/needed]: None
Attachment #8894010 - Flags: approval-mozilla-beta?
Comment on attachment 8894010 [details]
Bug 1387634 - Update unsupported @autocomplete token list to reflect the autofill MVP.

Form autofill will be enabled in Beta56. Beta56+.
Attachment #8894010 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #9)
> [Is this code covered by automated tests?]: yes, wpt
> [Has the fix been verified in Nightly?]: Yes, by myself
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Matthew's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.