[Form Autofill] Recognize "United States" as "United States"

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
P3
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: B.J. Herbison, Assigned: lchang)

Tracking

57 Branch
mozilla57
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [form autofill:MVP])

User Story

When I enter "United States" or "USA" or "United States of America" in the Country field in the Autofill Demo the country isn't saved.

"United States" is especially annoying as the preferences page showing saved forms lists "United States" as the only possible value.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

9 months ago
User Story: (updated)
Summary: [Form Autofill] Recognize "United States → [Form Autofill] Recognize "United States" as "United States"
Whiteboard: [form autofill]
We should consider this for the MVP if we have time. We already dealt with something similar in another bug so maybe it needs to be applied to the form saving code.
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
Whiteboard: [form autofill] → [form autofill:MVP]
(Assignee)

Updated

9 months ago
Assignee: nobody → lchang
(Assignee)

Comment 2

9 months ago
It's actually by design because the country field in the demo page uses the "country" attribute of @autocomplete, which is explicitly designed to accept a valid ISO 3166-1-alpha-2 country code [1]. That's the reason why we don't intend to identify it. You can try it on another demo page [2] which containing the "country-name" field and we do identify the various country names if it's available.

Though I think we can still do it more smartly by also identifying the "country" field if it doesn't match any valid country code. It can't be worse anyway. I'll make a patch for that.

[1] https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-fe-autocomplete-country
[2] https://luke-chang.github.io/autofill-demo/autocomplete-all.html
Comment hidden (mozreview-request)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8903542 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

https://reviewboard.mozilla.org/r/175362/#review180534

Looks good!! Thank you.

::: browser/extensions/formautofill/ProfileStorage.jsm:1292
(Diff revision 1)
>        country = FormAutofillUtils.identifyCountryCode(address["country-name"]);
>      }
>  
> +    // The "identifyCountryCode" can only identify currently-supported countries
> +    // ("US" in MVP). Uses "address.country" as the final fallback so we won't
> +    // lose any valid country code.

I think once we walked in fallback section that means the country code isn't in our valid country list, so would it be less misleading to change the string from "... lose any valid country code" to "... lose any country code from user input"?
Attachment #8903542 - Flags: review?(ralin) → review+
(In reply to Luke Chang [:lchang] from comment #2)
> It's actually by design because the country field in the demo page uses the
> "country" attribute of @autocomplete, which is explicitly designed to accept
> a valid ISO 3166-1-alpha-2 country code [1]. That's the reason why we don't
> intend to identify it. You can try it on another demo page [2] which
> containing the "country-name" field and we do identify the various country
> names if it's available.
> 
> Though I think we can still do it more smartly by also identifying the
> "country" field if it doesn't match any valid country code. It can't be
> worse anyway. I'll make a patch for that.

I don't think we should try to be smart in this case since it would be a website author problem and we are following the spec. It seems like we should just update the test page label or @autocomplete instead since it's currently wrong/misleading.

> [1]
> https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#attr-
> fe-autocomplete-country
> [2] https://luke-chang.github.io/autofill-demo/autocomplete-all.html
(Assignee)

Comment 6

9 months ago
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> I don't think we should try to be smart in this case since it would be a
> website author problem and we are following the spec. It seems like we
> should just update the test page label or @autocomplete instead since it's
> currently wrong/misleading.

Yeah, I agree it's website author's problems if "country" attribute is used explicitly. However, what I worry is that our heuristics marks a country-like field as "country" instead of "country-name". In this case we can't expect those fields follow the "country" spec. Therefore, I would think there's no downside to also try to identify "country" field since it's not bad to save as much data as we can (given the identified results here are accurate at most of time). Changing heuristics to always mark them as "country-name" can be another choice but I feel it might not be sufficient as website authors do make mistakes on following the spec. What do you think?
Flags: needinfo?(MattN+bmo)
(Reporter)

Comment 7

9 months ago
Should the distinction be between "country code" and "country name"? You can't store a "country" in a form field.
(Assignee)

Comment 8

9 months ago
"country" is actually the keyword that stands for "country code" in @autocomplete spec.
(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 #5)
> > I don't think we should try to be smart in this case since it would be a
> > website author problem and we are following the spec. It seems like we
> > should just update the test page label or @autocomplete instead since it's
> > currently wrong/misleading.
> 
> Yeah, I agree it's website author's problems if "country" attribute is used
> explicitly. However, what I worry is that our heuristics marks a
> country-like field as "country" instead of "country-name". In this case we
> can't expect those fields follow the "country" spec. Therefore, I would
> think there's no downside to also try to identify "country" field since it's
> not bad to save as much data as we can (given the identified results here
> are accurate at most of time). Changing heuristics to always mark them as
> "country-name" can be another choice but I feel it might not be sufficient
> as website authors do make mistakes on following the spec. What do you think?

Sean added metadata to indicate if the field info comes from @autocomplete or not but I don't recall at what level it gets deleted. I'd be fine with using that if it's available to only be looser when it's not from @autocomplete.
Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

9 months ago
Attachment #8903542 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 11

9 months ago
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #9)
> Sean added metadata to indicate if the field info comes from @autocomplete
> or not but I don't recall at what level it gets deleted. I'd be fine with
> using that if it's available to only be looser when it's not from
> @autocomplete.

Thanks. I uploaded a new patch that only tries to identify the "country" field if it doesn't use @autocomplete attribute.


Hi Steve,

Since Ray will be on PTO, could you take a look? Note that it depends on bug 1394139. Thanks.

Comment 12

9 months ago
mozreview-review
Comment on attachment 8904972 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

https://reviewboard.mozilla.org/r/176812/#review182206

::: browser/extensions/formautofill/FormAutofillHandler.jsm:582
(Diff revision 1)
>      }
>  
>      return data;
>    },
>  
>    normalizeAddress(address) {

nit: Maybe having an underscore prefix like `_normalizeAddress` for a private method?
Attachment #8904972 - Flags: review?(schung) → review+
Comment on attachment 8904972 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

https://reviewboard.mozilla.org/r/176812/#review182214
Attachment #8904972 - Flags: review+
(Assignee)

Comment 14

9 months ago
mozreview-review-reply
Comment on attachment 8904972 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

https://reviewboard.mozilla.org/r/176812/#review182206

> nit: Maybe having an underscore prefix like `_normalizeAddress` for a private method?

It's a privaate method indeed. Thanks.
Comment hidden (mozreview-request)

Comment 16

9 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41f763899ab8
[Form Autofill] Identify country code from "country" attribute as well. r=MattN,steveck
https://hg.mozilla.org/mozilla-central/rev/41f763899ab8
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 18

9 months ago
Comment on attachment 8904972 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

Approval Request Comment
[Feature/Bug causing the regression]:
Feature.

[User impact if declined]:
Less data can be saved.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It affects Form Autofill system add-on only.

[String changes made/needed]:
No.
Attachment #8904972 - Flags: approval-mozilla-beta?
Comment on attachment 8904972 [details]
Bug 1394854 - [Form Autofill] Identify country code from "country" attribute as well.

Fix form autofill issue. Beta56+.
Attachment #8904972 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 20

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/a52bc131f20b
status-firefox56: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.