Closed Bug 1388152 Opened 2 years ago Closed 2 years ago

Duplicate addresses stored because of numeric state

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

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

People

(Reporter: Dolske, Assigned: ralin)

References

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

I noticed form autofill started suggesting multiple addresses for me, which is a little confusing because I only live at one location. :)

Looking at the three stored addresses:

1) Mostly complete (name+address+city+zip+email+phone), but with state = "5".
2) Name, email, phone, state = "CA"
3) Only state = "12", zipcode, US.

MattN says there's probably a bug where we shouldn't save a numeric state, since that will mess up the address collapsing/consolidation code.

Unfortunately I'm not sure what sites caused this.
If I had to guess I'd say this is related to <select> support. Maybe we are:
a) saving the site-specified value instead of the result of mapping to a US state, or
b) Saving the index of the option (less likely)

Scott, are you able to look at this soon? Justin had two occurrences of this bug and I had one so I worry about it on beta.
Flags: needinfo?(scwwu)
Whiteboard: [form autofill:M4]
I'll take a look at this. It might be related to how addresses are autosaved. Thanks for bringing this to my attention.
Assignee: nobody → scwwu
Flags: needinfo?(scwwu)
I can take this first.

Matt is right. We currently create a profile based on the value of <select> elements, which cases this problem. Using the result of the mapping is able to fix this. Though I think we should also normalize "address-level1" ("state" in US) in the storage so that "CA" and "California" won't cause duplicate. Therefore, it leads to two questions:

1. Should we change the "state" field in edit dialog to <select> element, too? It can avoid various expressions manually typed in by users.

2. Which normalized formats (e.g. "CA" or "California") are we prefer to use?
Assignee: scwwu → lchang
(In reply to Luke Chang [:lchang] from comment #3)
> 1. Should we change the "state" field in edit dialog to <select> element,
> too? It can avoid various expressions manually typed in by users.

I remember we had a discussion about this and decided not to use <select> in preferences because we are not always able to determine which country it is related to.

> 2. Which normalized formats (e.g. "CA" or "California") do we prefer to use?

add another question:

3. What should we do if we can not map it to a valid state name? Ignore it or save its "text" as-is anyway?
Whiteboard: [form autofill:M4] → [form autofill:M3]
Discussed with Luke and Scott in person, I'll take care of this.
Assignee: lchang → ralin
(In reply to Luke Chang [:lchang] from comment #4)
> > 2. Which normalized formats (e.g. "CA" or "California") do we prefer to use?
> 
"CA" is preferable in the other day we discussed in meeting, and I've confirmed again that with Juwei.

> add another question:
> 
> 3. What should we do if we can not map it to a valid state name? Ignore it
> or save its "text" as-is anyway?
Yeah. Juwei agreed to save the text in the case we cannot infer or map to a valid state name, so we won't surprise users when they see a numeric state and have no idea where it comes from.
Got few concerns when I was working on state normalization, and I ended up saving "text" for all <select>s according to the discussion with Juwei in the afternoon.

- Country should be known before starting to normalize the state name, if we decided to save the abbreviation like "CA" instead of "California". However, in the real case, country isn't always available, it probably take a long time for the matching and even worse is that we might map the state to a wrong country.
- The text of select in some way is more meaningful and genuine to users.
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review173954

::: browser/extensions/formautofill/FormAutofillHandler.jsm:484
(Diff revision 2)
> +        if (type == "address" &&
> +            element instanceof Ci.nsIDOMHTMLSelectElement &&
> +            element.selectedOptions.length == 1 &&
> +            value) {
> +          value = element.selectedOptions[0].text.trim();
> +        }

Some feedbacks here.

1. It shouldn't be for "address" only. The expiry date of "creditCard" should be applied as well. However, it might be easier to normalize expiry date from "value" than from "text" so I'm fine with this for now. Could you please file a bug for tracking how to create a profile from select elements of credit card fields and add a comment mentioning the bug number here.

2. The value shouldn't be collected if `element.selectedOptions.length > 1`. We need another check here.

3. Please also comment on why we need to check `value` itself here.
Attachment #8896918 - Flags: review?(lchang)
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review174556

::: browser/extensions/formautofill/FormAutofillHandler.jsm:482
(Diff revision 3)
> +        // Save displayed text instead of actual value of select
> +        // element.

nit: You forgot to mention: `Not applied to credit card for now because the value of "expiry date" is usually easier to parse than its label.`

::: browser/extensions/formautofill/test/unit/test_onFormSubmitted.js:9
(Diff revision 3)
>  
>  const MOCK_DOC = MockDocument.createTestDocument("http://localhost:8080/test/",
>                     `<form id="form1">
>                        <input id="street-addr" autocomplete="street-address">
> +                      <select id="address-level1" autocomplete="address-level1">
> +                        <option value=""></option>

Could you also test that the option with empty value won't be collected? Thanks.
Attachment #8896918 - Flags: review?(lchang) → review+
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review174632

::: browser/extensions/formautofill/test/unit/test_onFormSubmitted.js:8
(Diff revision 3)
>  Cu.import("resource://formautofill/FormAutofillContent.jsm");
>  
>  const MOCK_DOC = MockDocument.createTestDocument("http://localhost:8080/test/",
>                     `<form id="form1">
>                        <input id="street-addr" autocomplete="street-address">
> +                      <select id="address-level1" autocomplete="address-level1">

We ideally shouldn't save separate profiles if the state on differs in whether it's abbreviated or not. e.g. if I have a profile with the state saved as "CA" but then submit a form with the rest of the fields the same but "California" is used as the label in an <option> then there should be no new profile created and no update doorhanger.

::: browser/extensions/formautofill/test/unit/test_onFormSubmitted.js:17
(Diff revision 3)
> +                        <option value="AP">Armed Forces Pacific</option>
> +                        <option value="AE">Armed Force Europe</option>
> +                        <option value="AA">Armed Forces America</option>
> +                        <option value="AZ">Arizona</option>
> +                        <option value="AR">Arkansas</option>
> +                        <option value="CA">California</option>

You should also consider forms which provide both the   abbreviation and long text in the <option> label e.g.

CA - California

or 

09 - September    (for credit card expiration)

Saving "CA - California" doesn't seem ideal.
(In reply to Ray Lin[:ralin] from comment #7)
> Got few concerns when I was working on state normalization, and I ended up
> saving "text" for all <select>s according to the discussion with Juwei in
> the afternoon.
> 
> - Country should be known before starting to normalize the state name, if we
> decided to save the abbreviation like "CA" instead of "California". However,
> in the real case, country isn't always available, it probably take a long
> time for the matching and even worse is that we might map the state to a
> wrong country.

I think if we know the country it would still be good to normalize to CA IMO since that is standardized often even across languages so if the user is multilingual, CA will still match to Californië in Dutch (e.g. if the option has value="CA") whereas California isn't going to match to Californië. Maybe in our write processor we should try to normalize if we do know the country? That way if a profile initially doesn't have a country but then one gets merged in from submitting another form, the state can then become normalized too.

> - The text of select in some way is more meaningful and genuine to users.

In what UI? At least in Canada and the US the abbreviation is what everyone needs to use for mailing purposes so it's more useful to fill it in most forms. If you're talking about the management interface, I still think for countries and US states that we should provide UI smarter than just an <input> e.g. using <input list="states"> or <select> when the saved field value is empty or matches one in the list. This solution is for another bug though.
Status: NEW → ASSIGNED
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

Clearing the review flag as Luke may want to review over again.
Attachment #8896918 - Flags: review+
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review174556

Thanks for review

> nit: You forgot to mention: `Not applied to credit card for now because the value of "expiry date" is usually easier to parse than its label.`

Since the we focus on the state abbreviation, I guess we can leave credit card issue to another bug.

> Could you also test that the option with empty value won't be collected? Thanks.

Added in the new revision.
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review174632

> We ideally shouldn't save separate profiles if the state on differs in whether it's abbreviated or not. e.g. if I have a profile with the state saved as "CA" but then submit a form with the rest of the fields the same but "California" is used as the label in an <option> then there should be no new profile created and no update doorhanger.

Thank you Matt! I end up saving the abbreviation in the new patch, and indeed it's easier to handle the duplucate.
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

https://reviewboard.mozilla.org/r/168192/#review175134
Attachment #8896918 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/981e573fd0a9
Save abbreviated state name of address-level1 select element rather than its value in form autofill submission. r=lchang,MattN
See Also: → 1391634
https://hg.mozilla.org/mozilla-central/rev/981e573fd0a9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

Approval Request Comment
[Feature/Bug causing the regression]: <select> autofill support
[User impact if declined]: Garbage state values will be saved from address forms using <select>
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Wouldn't hurt. Test submitting US address forms with <select> like the one in comment 0 and confirm the correct state is saved in the profile.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: A bit 
[Why is the change risky/not risky?]: because it wasn't verified yet. Impact should be limited to state fields though.
[String changes made/needed]: None
Attachment #8896918 - Flags: approval-mozilla-beta?
Hi Florin, can we have some QA's help to verify this?
Flags: needinfo?(florin.mezei)
Moving to Andrei so they can get to this when they have time available.
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
Comment on attachment 8896918 [details]
Bug 1388152 - Save abbreviated state name of address-level1 select element rather than its value in form autofill submission.

Let's uplift this to beta 7 and test it there rather than in nightly.
Attachment #8896918 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Unable to reproduce issue on 57.0a1 Build: 20170807113452 with Windows 10x64, Ubuntu 14.04 and MacOS 10.12

Verified the latest nightly 57.0a1 (2017-09-17) and 56.0b12 on several top shopping sites and state is saved correctly under address profile.
Justin, can you please take a look at this issue to make sure it does not happen anymore?
Flags: needinfo?(dolske)
Flags: needinfo?(andrei.vaida)
As I noted in comment 0: I don't know what caused this to happen, so I can't say if it happens or not any more.
Flags: needinfo?(dolske)
You need to log in before you can comment on or make changes to this bug.