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)
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)
59 bytes,
text/x-review-board-request
|
steveck
:
review+
MattN
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
No description provided.
Reporter | ||
Updated•7 years ago
|
User Story: (updated)
Summary: [Form Autofill] Recognize "United States → [Form Autofill] Recognize "United States" as "United States"
Updated•7 years ago
|
Whiteboard: [form autofill]
Comment 1•7 years ago
|
||
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]
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → lchang
Assignee | ||
Comment 2•7 years 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•7 years 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+
Comment 5•7 years ago
|
||
(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•7 years 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•7 years ago
|
||
Should the distinction be between "country code" and "country name"? You can't store a "country" in a form field.
Assignee | ||
Comment 8•7 years ago
|
||
"country" is actually the keyword that stands for "country code" in @autocomplete spec.
Comment 9•7 years ago
|
||
(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•7 years ago
|
Attachment #8903542 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years 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•7 years 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 13•7 years 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/#review182214
Attachment #8904972 -
Flags: review+
Assignee | ||
Comment 14•7 years 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•7 years 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
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41f763899ab8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 18•7 years 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 19•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a52bc131f20b
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•