Closed Bug 1394854 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: bj, Assigned: lchang)

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.

Attachments

(1 file, 1 obsolete file)

      No description provided.
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.
Whiteboard: [form autofill] → [form autofill:MVP]
Priority: -- → P3
Assignee: nobody → lchang
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 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
(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)
Should the distinction be between "country code" and "country name"? You can't store a "country" in a form field.
"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)
Attachment #8903542 - Attachment is obsolete: true
(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 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+
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.
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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+
You need to log in before you can comment on or make changes to this bug.