Closed
Bug 1358960
Opened 7 years ago
Closed 7 years ago
Exclude "united" string when applying "address-level1" regular expression
Categories
(Toolkit :: Form Manager, enhancement)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: selee, Assigned: selee)
References
(Depends on 1 open bug)
Details
(Whiteboard: [form autofill:M3])
Attachments
(1 file)
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 hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 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 3•7 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 > 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•7 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/#review171030
Attachment #8894104 -
Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 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.
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6942a602ed51
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Whiteboard: [form autofill:MVP] → [form autofill:M3]
Comment 9•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/303b2be541c6
status-firefox56:
--- → fixed
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•