Use a country-specific join character for joining address lines together

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: selee)

Tracking

(Blocks 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3])

Attachments

(3 attachments, 1 obsolete attachment)

If I have the following in my address profile:
> 123 Sesame Street
> Apt ABC

It will currently get filled into an autocomplete="street-address" field like:
> 123 Sesame StreetApt ABC
(note that there is no space or comma between the two address lines)

We should use join character(s) between concatenated address lines appropriate for the country.
It seems like the U.S. uses a space[1] for this example so maybe it's fine to use a space for now but we should declare it somewhere to we remember to update it when adding other countries.

[1] https://pe.usps.com/text/pub28/28c2_001.htm
Duplicate of this bug: 1370431
Assignee

Updated

2 years ago
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review154988

::: browser/extensions/formautofill/FormAutofillContent.jsm:116
(Diff revision 1)
>        if (this.forceStop) {
>          return;
>        }
>  
> +      let handler = FormAutofillContent.getFormHandler(focusedInput);
> +      let computedAddresses = handler.getComputedProfiles(addresses);

The wording `computed` might cause confusion with the parent-side `computedFields`. How about `adaptedAddresses`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:105
(Diff revision 1)
> +      let streetAddresses = profile["street-address"].split("\n");
> +      profile["street-address"] = FormAutofillUtils.toOneLineAddress(streetAddresses);

I think we need an one-line "street-address" only if the target field exists and is a single-line input. I would recommend you to check the existence and input type (though I know we haven't supported <textarea> yet) so we can avoid unnecessary converting.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:108
(Diff revision 1)
> +      // Assume that "address-line2" always exists when address-line1&3 already
> +      // exist.

Can we concatenate "address-line2" to "address-line1" (if it exists) in this case? (Though it's an edge case indeed.)

::: browser/extensions/formautofill/FormAutofillHandler.jsm:118
(Diff revision 1)
> +          profile["address-line2"] = FormAutofillUtils.toOneLineAddress([
> +            profile["address-line2"],
> +            profile["address-line3"],
> +          ]);
> +        } else {
> +          profile["address-line1"] = profile["street-address"];

Since `street-address` might not be converted in advance (according to the above comment), this line needs changes.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:124
(Diff revision 1)
> +        }
> +      }
> +    }
> +  },
> +
> +  getComputedProfiles(originalProfiles) {

Can we leverage `allFieldNames`, which is collected in `FormAutofillContent.startSearch`, by passing it as a argument to reduce the times on calling `getFieldDetailByName` when the existence of a field is the only thing we care?
Attachment #8879047 - Flags: review?(lchang)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8879048 [details]
Bug 1370475 - Part 1: address-line[1-3] should be recognized as "street-address" in the secondary label order.

https://reviewboard.mozilla.org/r/150370/#review155402

It looks good overall.

The only concern is that I'd prefer not to convert "street-address" until it's necessary. In other words, "street-adddress" might remain multi-line when the target field is a <textarea>. Therefore, we need to convert it explicitly in the result if we want a single-line "street-address" as the secondary label.

::: browser/extensions/formautofill/ProfileAutoCompleteResult.jsm:90
(Diff revision 1)
>     * @param   {Array<Object>} allFieldNames The field names in the same section
>     * @param   {object} profile The profile providing the labels to show.
>     * @returns {string} The secondary label
>     */
>    _getSecondaryLabel(focusedFieldName, allFieldNames, profile) {
> -    /* TODO: Since "name" is a special case here, so the secondary "name" label
> +    const possibleAddressFields = [

nit: Since we use `address` to refer to the entire profile, I would prefer `possibleStreetAddressFields` or `possibleAddressLinesFields`.
Attachment #8879048 - Flags: review?(lchang)
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review154988

> The wording `computed` might cause confusion with the parent-side `computedFields`. How about `adaptedAddresses`?

Do you mean the variable name or function name or both?

> I think we need an one-line "street-address" only if the target field exists and is a single-line input. I would recommend you to check the existence and input type (though I know we haven't supported <textarea> yet) so we can avoid unnecessary converting.

The single line version is a must for ProfileAutoCompleteResult to generate the label since ProfileAutoCompleteResult always needs the single line version.

> Can we concatenate "address-line2" to "address-line1" (if it exists) in this case? (Though it's an edge case indeed.)

The comment mentioned the case like address-line1, address-line2, and address-line3 all exist. Could you explain the case that you mentioned?

> Can we leverage `allFieldNames`, which is collected in `FormAutofillContent.startSearch`, by passing it as a argument to reduce the times on calling `getFieldDetailByName` when the existence of a field is the only thing we care?

`allFieldNames` sounds good. This comment reminds me that `allFieldNames` can be cached in handler as well, and we don't need an extra argument for `getComputedProfiles`.
Assignee

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8879048 [details]
Bug 1370475 - Part 1: address-line[1-3] should be recognized as "street-address" in the secondary label order.

https://reviewboard.mozilla.org/r/150370/#review155402

I add the part 3 commit to fix this.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8879785 - Attachment is obsolete: true
Attachment #8879785 - Flags: review?(lchang)

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review154988

> Do you mean the variable name or function name or both?

I prefer both.

> The single line version is a must for ProfileAutoCompleteResult to generate the label since ProfileAutoCompleteResult always needs the single line version.

Yeah, I see. Since multi-line version of "street-address" is also necessary for further implementations, we need to work out it.

> The comment mentioned the case like address-line1, address-line2, and address-line3 all exist. Could you explain the case that you mentioned?

I meant in the case that only "address-line2" is omitted. Is it possible to leave "address-line3" as-is but merge "address-line2" into "address-line1". What I imagined is something like:

```js
let waitForConcat = [];
for (let f of ["address-line3", "address-line2", "address-line1"]) {
  waitForConcat.unshift(profile[f]);
  if (this.getFieldDetailByName(f)) {
    if (waitForConcat.length > 1) {
      profile[f] = FormAutofillUtils.toOneLineAddress(waitForConcat);
    }
    waitForConcat = [];
  }
}
```

Comment 15

2 years ago
mozreview-review
Comment on attachment 8879048 [details]
Bug 1370475 - Part 1: address-line[1-3] should be recognized as "street-address" in the secondary label order.

https://reviewboard.mozilla.org/r/150370/#review156598

It looks good.

BTW, I also did a little refactoring on `ProfileAutoCompleteResult.jsm` in bug 1358944, so one of us will need to resolve conflicts.
Attachment #8879048 - Flags: review?(lchang) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review156652

::: browser/extensions/formautofill/FormAutofillHandler.jsm:116
(Diff revision 3)
> +      this._cacheValue.allFieldNames = this.fieldDetails.map(record => record.fieldName);
> +    }
> +    return this._cacheValue.allFieldNames;
> +  },
> +
> +  _getOneLineFullAddress(address) {

"OneLineFullAddress" makes me think of an entire address including city, state and country. I would prefer "OneLineStreetAddress".

::: browser/extensions/formautofill/FormAutofillHandler.jsm:118
(Diff revision 3)
> +    return this._cacheValue.allFieldNames;
> +  },
> +
> +  _getOneLineFullAddress(address) {
> +    if (!this._cacheValue.oneLineFullAddress) {
> +      this._cacheValue.oneLineFullAddress = {};

How about using `map`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:129
(Diff revision 3)
> +      let streetAddressField = this.getFieldDetailByName("street-address");
> +      profile["street-address"] = this._getOneLineFullAddress(profile["street-address"]);

I suppose you're going to improve this part as `streetAddressField` hasn't been used yet.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:132
(Diff revision 3)
> +      // Assume that "address-line2" always exists when address-line1&3 already
> +      // exist.

Should we consider concatenating line4, line5...?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:62
(Diff revision 3)
> +    // white space is a temporary solution.
> +    return " ";
> +  },
> +
> +  toOneLineAddress(strings) {
> +    return strings.join(this.getAddressSeparator());

We might need to skip empty lines.

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:22
(Diff revision 3)
>  /* import-globals-from ../../../../../toolkit/components/satchel/test/satchel_common.js */
>  /* import-globals-from formautofill_common.js */
>  
>  "use strict";
>  
> +const {FormAutofillUtils} = SpecialPowers.Cu.import("resource://formautofill/FormAutofillUtils.jsm");

Test cases should be tested by methods other than `toOneLineAddress` so we can also make sure nothing wrong with that API.
Attachment #8879047 - Flags: review?(lchang)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8879786 [details]
Bug 1370475 - Part 3: Add a custom field "-moz-street-address-one-line" for popup label and filling street-address.

https://reviewboard.mozilla.org/r/151160/#review156660

::: browser/extensions/formautofill/FormAutofillHandler.jsm:132
(Diff revision 2)
>    _addressTransformer(profile) {
>      if (profile["street-address"]) {
>        let streetAddressField = this.getFieldDetailByName("street-address");
> -      profile["street-address"] = this._getOneLineFullAddress(profile["street-address"]);
> +      // "-moz-street-address-one-line" is for filling "street-address" field
> +      // and the label of street-address or address-lines.
> +      profile["-moz-street-address-one-line"] = this._getOneLineFullAddress(profile["street-address"]);

The idea of `-moz-street-address-line` is great, but I think it's still important to set the one-line street-address to `profile["street-address"]` (when needed) as they can mean differently.

`-moz-street-address-line` can be used by those who explicitly need one-line version. e.g. the suggestion dropdown, the dialog in preferences. (For this reason, I think we probably should do that in profileStorage.)

`street-address` should be always adapted to the field, not only single-line v.s. multi-line but also limitations we might consider in the future. Therefore, We don't need to change `fieldDetail.fieldName` for any field and we won't introduce complexity.

What do you think?
Attachment #8879786 - Flags: review?(lchang)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
mozreview-review-reply
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review156652

> How about using `map`?

The key of `oneLineFullAddress` is "string". Is there any benfit to use Map here?

> Should we consider concatenating line4, line5...?

line4, line5, etc. should be handled in `ProfileStroage` rather than `FormAutofillHandler`.
Assignee

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8879786 [details]
Bug 1370475 - Part 3: Add a custom field "-moz-street-address-one-line" for popup label and filling street-address.

https://reviewboard.mozilla.org/r/151160/#review156660

> The idea of `-moz-street-address-line` is great, but I think it's still important to set the one-line street-address to `profile["street-address"]` (when needed) as they can mean differently.
> 
> `-moz-street-address-line` can be used by those who explicitly need one-line version. e.g. the suggestion dropdown, the dialog in preferences. (For this reason, I think we probably should do that in profileStorage.)
> 
> `street-address` should be always adapted to the field, not only single-line v.s. multi-line but also limitations we might consider in the future. Therefore, We don't need to change `fieldDetail.fieldName` for any field and we won't introduce complexity.
> 
> What do you think?

I would like to keep the mulit-line version in `"street-adddress"` and fill the right version when filling values. In case there is any change before filling value, so we still have both version to choose.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8879047 [details]
Bug 1370475 - Part 2: Convert the address related fields for display purpose.

https://reviewboard.mozilla.org/r/150368/#review157068

::: browser/extensions/formautofill/FormAutofillContent.jsm:116
(Diff revision 5)
>        if (this.forceStop) {
>          return;
>        }
>  
> +      let handler = FormAutofillContent.getFormHandler(focusedInput);
> +      let adaptedAddresses = handler.getAdoptedProfiles(addresses);

typo: adapted (including many variables, function names and the file name `test_getAdoptedProfiles.js`)

::: browser/extensions/formautofill/FormAutofillUtils.jsm:61
(Diff revision 5)
> +  toOneLineAddress(strings) {
> +    return strings.filter(s => s).join(this.getAddressSeparator());

nit: It would be more convenient if it can accept strings as well so we can get rid of plenty `split("\n")`.
Attachment #8879047 - Flags: review?(lchang) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8879786 [details]
Bug 1370475 - Part 3: Add a custom field "-moz-street-address-one-line" for popup label and filling street-address.

https://reviewboard.mozilla.org/r/151160/#review157116

::: browser/extensions/formautofill/FormAutofillHandler.jsm:129
(Diff revision 4)
> -      profile["street-address"] = this._getOneLineStreetAddress(profile["street-address"]);
> +      // "-moz-street-address-one-line" is for filling "street-address" field
> +      // and the label of street-address or address-lines.

nit: "-moz-street-address-one-line" is used by the labels in ProfileAutoCompleteResult only.
Attachment #8879786 - Flags: review?(lchang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 34

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72a1b578682a
Part 1: address-line[1-3] should be recognized as "street-address" in the secondary label order. r=lchang
https://hg.mozilla.org/integration/autoland/rev/66f8f7dc0b98
Part 2: Convert the address related fields for display purpose. r=lchang
https://hg.mozilla.org/integration/autoland/rev/f3dc2e86a424
Part 3: Add a custom field "-moz-street-address-one-line" for popup label and filling street-address. r=lchang
Keywords: checkin-needed
Assignee

Updated

2 years ago
Blocks: 1360114

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/72a1b578682a
https://hg.mozilla.org/mozilla-central/rev/66f8f7dc0b98
https://hg.mozilla.org/mozilla-central/rev/f3dc2e86a424
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.