1415077 - [Form Autofill] Classify the fields into multiple sections based on the section part of autocomplete attr.

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

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

If there are any fields with the section part of autocomplete attr, these fields should be classified with `autocomplete` attr.

If there is a form looks like this one:
<form>
<input autocomplete="name">
<input autocomplete="billing street-address">
<input autocomplete="billing postal-code">
<input autocomplete="shipping street-address">
<input autocomplete="shipping postal-code">
<input autocomplete="email">
</form>

`FormAutofillHeuristics.getFormInfo` should provide the result:
[
  [{fieldName: "name"}, {fieldName: "email"}], // fields without section name
  [{fieldName: "street-address", ...}, {fieldName: "postal-code", ...}], // billing
  [{fieldName: "street-address", ...}, {fieldName: "postal-code", ...}], // shipping
]

The detail autocomplete info should be based on the real result of getAutoCompleteInfo() API.

Comment 2

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

Attachment: Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

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

Comment 3

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

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

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

Comment 4

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

The latest patch provides the section support, and the sections can be classified by the section part of autocomplete attributes. The related test will be implemented soon. BTW, bug 1416664 will handle the heuristic of grouping.
This sample page is good for trying this patch with your own tweaks:
http://jsbin.com/dedoqoxako/1/edit?html,output

Comment 5

[Ray Lin[:ralin]]

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

https://reviewboard.mozilla.org/r/199318/#review205196

Thanks, overall looks good!

If the section concept is running on dual tracks behind pref, it's fine to keep orginial path as is. Though, I'm thinking there's no harm keeping only one fieldDetails store instead of two, and use a single access point to "fieldDetails" getter that return conditionally according to pref, then we don't have to trace two different routes with or without pref section enabled.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:115
(Diff revision 2)
> +      name,
> +      fieldDetails: [fieldDetail],
> +    });
> +  }
> +
> +  getSections(allowDuplicates) {

function name getSections seems a bit misleading.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:746
(Diff revision 2)
>            string = string.toLowerCase().split("united state").join("");
>          }
>          if (this.RULES[regexp].test(string)) {
>            return {
>              fieldName: regexp,
>              section: "",

just curious to know, should we someways set the serialized section name here?

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

https://reviewboard.mozilla.org/r/199318/#review205196

> function name getSections seems a bit misleading.

Maybe `getSectionFieldDetails`?

Comment 7

[Ray Lin[:ralin]]

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

https://reviewboard.mozilla.org/r/199318/#review205196

> just curious to know, should we someways set the serialized section name here?

Sorry, misunderstood this. Please ignore and close the issue :P

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

https://reviewboard.mozilla.org/r/199318/#review205834

Comment 9

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

Attachment: Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

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

Comment 10

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

Attachment: Bug 1415077 - Implement the new multiple section tests.

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

Comment 11

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

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

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

Comment 12

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

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199318/diff/3-4/

Comment 13

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

Comment on attachment 8930137 [details]
Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

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

Comment 14

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

Comment on attachment 8930138 [details]
Bug 1415077 - Implement the new multiple section tests.

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

Comment 15

[Ray Lin[:ralin]]

Comment on attachment 8930137 [details]
Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

https://reviewboard.mozilla.org/r/201306/#review207194

Looks good! Thanks

::: browser/extensions/formautofill/test/unit/heuristics/third_party/test_HomeDepot.js:10
(Diff revision 2)
>  runHeuristicsTest([
>    {
>      fixturePath: "Checkout_ShippingPayment.html",
>      expectedResult: [
>        [
> +        [

nit: inconsistent line break for array brackets, maybe keep in the same way regardless of the amount of items.

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8930138 [details]
Bug 1415077 - Implement the new multiple section tests.

https://reviewboard.mozilla.org/r/201308/#review207196

Comment 17

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8930137 [details]
Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

https://reviewboard.mozilla.org/r/201306/#review207244

::: browser/extensions/formautofill/test/unit/head.js:84
(Diff revision 2)
>  }
>  
> +function verifySectionFieldDetails(sections, expectedResults) {
> +  Assert.equal(sections.length, expectedResults.length, "Expected section count.");
> +  sections.forEach((section, sectionIndex) => {
> +    let sectionInfo = sections[sectionIndex];

Why don't you use `section` directly?

Comment 18

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8930138 [details]
Bug 1415077 - Implement the new multiple section tests.

https://reviewboard.mozilla.org/r/201308/#review207248

Comment 19

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

Comment on attachment 8930137 [details]
Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

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

Comment 20

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

Comment on attachment 8930138 [details]
Bug 1415077 - Implement the new multiple section tests.

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

Comment 21

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

Thank you for reviewing the patch!

Comment 22

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

Comment on attachment 8928082 [details]
Bug 1415077 - Classify the field details with autocomplete attr into multiple groups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199318/diff/4-5/

Comment 23

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

Comment on attachment 8930137 [details]
Bug 1415077 - Modify FormAutofill test to satisfy the section support feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201306/diff/3-4/

Comment 24

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

Comment on attachment 8930138 [details]
Bug 1415077 - Implement the new multiple section tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201308/diff/3-4/

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1667b83dc10
Classify the field details with autocomplete attr into multiple groups. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/e78023ba8b13
Modify FormAutofill test to satisfy the section support feature. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/d827c67e862e
Implement the new multiple section tests. r=lchang,ralin

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e1667b83dc10
https://hg.mozilla.org/mozilla-central/rev/e78023ba8b13
https://hg.mozilla.org/mozilla-central/rev/d827c67e862e