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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Sean Lee [:seanlee][:weilonge]]

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"

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1358960 - "united state" string should not be recognized as "address-level1".

Review commit: https://reviewboard.mozilla.org/r/165186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165186/

Comment 2

[Sean Lee [:seanlee][:weilonge]]

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 3

[Matthew N. [:MattN]]

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 4

[Matthew N. [:MattN]]

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

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

Comment 5

[Sean Lee [:seanlee][:weilonge]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165186/diff/1-2/

Comment 6

[Sean Lee [:seanlee][:weilonge]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6942a602ed51
"united state" string should not be recognized as "address-level1". r=MattN

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6942a602ed51

Comment 9

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/303b2be541c6