Exclude "united" string when applying "address-level1" regular expression

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: selee, Assigned: selee)

Tracking

(Depends on 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 attachment)

In bug 1349490, the regular expression of "address-level1" contains a backward matching pattern [1]. We should apply this pattern in JavaScript logic rather than RegExp.

[1] "(?<!united )state|county|region|province"
Assignee

Updated

2 years ago
Depends on: 1368858
Assignee

Updated

2 years ago
Assignee: nobody → selee
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
mozreview-review
Comment on attachment 8894104 [details]
Bug 1358960 - "united state" string should not be recognized as "address-level1".

https://reviewboard.mozilla.org/r/165186/#review170540

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:362
(Diff revision 1)
>        for (let string of getElementStrings) {
> +        // The original regexp "(?<!united )state|county|region|province" for
> +        // "address-line1" wants to exclude any "united state" string, so the
> +        // following code is to remove all "united state" string before applying
> +        // "addess-level1" regexp.
> +        if (["address-level1", "address-line2", "address-line3"].includes(regexp)) {

Hey MattN,

This patch provides a solution to apply the regexp `"(?<!united )state|county|region|province"` in JS.

I found "United State" string matches to "address-line2" and "address-line3" as well due to "unit" string.
AFAIK, Chromium uses the address-line parser to determine "address-line2" and "address-line3, so they won't have this problem.

May I know your suggestion that I can use this workaround here? or the address-line parser should implemented in MVP?

If you think this workaround is acceptable, I will add more comment to explain why we need this code here.

Thank you.

[1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/address_field.cc?type=cs&l=200
Comment on attachment 8894104 [details]
Bug 1358960 - "united state" string should not be recognized as "address-level1".

https://reviewboard.mozilla.org/r/165186/#review170540

> Hey MattN,
> 
> This patch provides a solution to apply the regexp `"(?<!united )state|county|region|province"` in JS.
> 
> I found "United State" string matches to "address-line2" and "address-line3" as well due to "unit" string.
> AFAIK, Chromium uses the address-line parser to determine "address-line2" and "address-line3, so they won't have this problem.
> 
> May I know your suggestion that I can use this workaround here? or the address-line parser should implemented in MVP?
> 
> If you think this workaround is acceptable, I will add more comment to explain why we need this code here.
> 
> Thank you.
> 
> [1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/address_field.cc?type=cs&l=200

I think the workaround is acceptable for now. Is there a reason for not using `.replace("united state", "")`?
Comment on attachment 8894104 [details]
Bug 1358960 - "united state" string should not be recognized as "address-level1".

https://reviewboard.mozilla.org/r/165186/#review171030
Attachment #8894104 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8894104 [details]
Bug 1358960 - "united state" string should not be recognized as "address-level1".

https://reviewboard.mozilla.org/r/165186/#review170540

> I think the workaround is acceptable for now. Is there a reason for not using `.replace("united state", "")`?

I think we should replace all "united state" string, e.g. <label>United State united sTATE city</label> should be recognized as addres-level2 rather than "address-level1". This case is added into the test as well.

Comment 7

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6942a602ed51
"united state" string should not be recognized as "address-level1". r=MattN

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6942a602ed51
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Whiteboard: [form autofill:MVP] → [form autofill:M3]
You need to log in before you can comment on or make changes to this bug.