Closed Bug 1339731 Opened 7 years ago Closed 7 years ago

[Form Autofill] Refactor FormAutofillHandler to support multiple section mechanism

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(2 files)

Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details
Bug 1339731 - Refactor some duplicated codes and remove the unused method.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details 
Form filling-in feature should be improved with section concept, and the relative logic could be in FormAutofillContent.getAllFieldNames (bug 1339727).

We need to consider how to handle a blank value to fill a field or not.
Whiteboard: [form autofill:M2] → [form autofill:MVP]
Whiteboard: [form autofill:MVP] → [form autofill]
Depends on: 1339727
Depends on: 1358941
For supporting multiple section mechanism, a new `FormAutofillHandler` design should be able to handle filling, previewing, and creating forms in a multiple section form. Hence, a proper data structure should maintain the relationship between all `fieldDetails` in the different sections of a form.

The current methods (e.g. `collectFormFields`, `autofillFormFields`, `clearPreviewedFormFields`, `previewFormFields`, etc.) are also necessary to refactor in order to manipulate the `fieldDetails` in the various cases for supporting section mechanism.

FormAutofillContent is the only consumer of FormAutofillHandler, so any methods used by FormAutofillContent will be under the concept of multiple sections unless there is any reason.
Blocks: 1411877
No longer blocks: 990188
No longer depends on: 1339727, 1358941, 1339721
Summary: Improve form filling-in feature with section concept → [Form Autofill] Refactor FormAutofillHandler to support multiple section mechanism
Whiteboard: [form autofill] → [form autofill:V2]
Assignee: nobody → selee
Status: NEW → ASSIGNED
Hi Luke, Ray,

This patch is going to separate some section related functions from FormAutofillHandler to FormAutofillSection.
The most change is the process of `collectFormFields` to create a new FormAutofillSection object.
Please take a look the patch and give a feedback. Thank you.
Attachment #8923301 - Flags: feedback?(ralin)
Attachment #8923301 - Flags: feedback?(lchang)
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review201250

::: browser/extensions/formautofill/FormAutofillContent.jsm:106
(Diff revision 4)
>      let focusedInput = formFillController.focusedInput;
>      let info = FormAutofillContent.getInputDetails(focusedInput);
>      let isAddressField = FormAutofillUtils.isAddressField(info.fieldName);
>      let handler = FormAutofillContent.getFormHandler(focusedInput);
> +    let section = handler.sections[0]; // TODO
>      let allFieldNames = handler.allFieldNames;

should be `let allFieldNames = section.allFieldNames;`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:199
(Diff revision 4)
> +   *        card field.
> +   */
> +  async autofillFormFields(profile, focusedInput) {
> +    let section = this.getSectionByElement(focusedInput);
> +    let targetSetOutput = {};
> +    await section.autofillFields(profile, focusedInput, targetSetOutput);

I'm not sure it's clear to pass an object reference here, it seems implicit to me. I'll comment my opinion in next issue.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:237
(Diff revision 4)
> +    this.form.rootElement.addEventListener("input", onChangeHandler);
> +    this.form.rootElement.addEventListener("reset", onChangeHandler);

We might want to avoid registering several handlers after more than one section being auto-filled, the handler can be registered only the first time auto-filled occurs in form.

I think we can delegate to section and let it handle the change within its own scope. At form level, what we need is just loop over all the sections afterwards like:
```
hasFilledFields = this.sections.some({address, creditCard} => address.filledRecordGUID || creditCard.filledRecordGUID);
```
(perhaps to create a helper function in section to simplify this check, or not if we decide to separate two collection types into two sections in the future)

then to decide whether we should remove the listeners from form. If so, I don't think the retrieval of section reference is needed.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:309
(Diff revision 4)
> -      log.debug("Ignoring address related fields since it has only",
> +      log.debug("Ignoring address related fields since the section has only",
> -                this.address.fieldDetails.length,
> +        this.address.fieldDetails.length,
> -                "field(s)");
> +        "field(s).");

nit: the indents were intentionally fixed in the other bug. Maybe leave it as-is.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:610
(Diff revision 4)
>          if (element == focusedInput ||
> -            (element != focusedInput && !element.value)) {
> +          (element != focusedInput && !element.value)) {
>            element.setUserInput(value);
>            this.changeFieldState(fieldDetail, "AUTO_FILLED");
>            continue;
>          }

nit: still one space off to the right indent.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:636
(Diff revision 4)
>          // Autofill highlight appears regardless if value is changed or not
>          this.changeFieldState(fieldDetail, "AUTO_FILLED");
>        }
>      }
>  
> -    // Handle the highlight style resetting caused by user's correction afterward.
> +    targetSetOutput.targetSet = targetSet;

`targetSetOutput.targetSet = this;

::: browser/extensions/formautofill/FormAutofillHandler.jsm:848
(Diff revision 4)
>  
>      this._normalizeAddress(data.address);
>  
>      if (data.address && !this._isAddressRecordCreatable(data.address.record)) {
>        log.debug("No address record saving since there are only",
> -                Object.keys(data.address.record).length,
> +        Object.keys(data.address.record).length,

nit: same here, multi-line strings indent.
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

Thanks. Overall looks good! though, I haven't read thoroughly all the lines, might miss some parts.
Attachment #8923301 - Flags: feedback?(ralin) → feedback+
Blocks: 1415073
Blocks: 1415077
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review201250

> We might want to avoid registering several handlers after more than one section being auto-filled, the handler can be registered only the first time auto-filled occurs in form.
> 
> I think we can delegate to section and let it handle the change within its own scope. At form level, what we need is just loop over all the sections afterwards like:
> ```
> hasFilledFields = this.sections.some({address, creditCard} => address.filledRecordGUID || creditCard.filledRecordGUID);
> ```
> (perhaps to create a helper function in section to simplify this check, or not if we decide to separate two collection types into two sections in the future)
> 
> then to decide whether we should remove the listeners from form. If so, I don't think the retrieval of section reference is needed.

Thanks for the suggestion. The latest patch provides these method (`clearFieldState`, `resetFieldStates`, and `isFilled`) in `FormAutofillSection` for `FormAutofillHandler` to handle the events of `input` and `reset`.
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review202736

Looks good, thanks! A few not big deal nits and thoughts.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:54
(Diff revision 5)
> -  winUtils: null,
> -
> -  /**
> +    /**
> -   * Enum for form autofill MANUALLY_MANAGED_STATES values
> +     * Enum for form autofill MANUALLY_MANAGED_STATES values
> -   */
> +     */
> -  fieldStateEnum: {
> +    this.fieldStateEnum = {

I think this enums could move out to global scope as a const, no need to keep in every instance.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:524
(Diff revision 5)
>      }
>  
>      fieldDetail.state = nextState;
> -  },
> +  }
> +
> +  clearFieldState(focusedInput) {

"clear" seems a bit inexpressive to the state transition which not really clear something. Not sure, IMO, the it's only consumer  derived from input change event of form, so would something like "handleFieldChanged" more corresponding for that?

Another idea for the similarity of two functions, maybe consider combining to a more generic handler like:
`handleFieldsChanged(...fieldDetails)`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:534
(Diff revision 5)
> +      targetSet = this.address;
> +    } else if (FormAutofillUtils.isCreditCardField(focusedInput)) {
> +      targetSet = this.creditCard;
> +    }
> +
> +    if (!targetSet.fieldDetails.some(detail => detail.state == "AUTO_FILLED")) {

Maybe 
```
if (targetSet.fieldDetails && !targetSet.fieldDetails.some....) {
```
though it's an unnecessary check for now, the absence of targetSet.fieldDetails might looks potentially throw error here since we're using else if insteaf of else above.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:890
(Diff revision 5)
> +    let section = this.getSectionByElement(focusedInput);
> +    section.getAdaptedProfiles(originalProfiles);
> +    return originalProfiles;
> +  }
> +
> +  isNoSectionFilled() {

nit: hasFilledSection() might be straightforward than the opposite of isFilled() in section.
Attachment #8923301 - Flags: review?(ralin)
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review202736

> Maybe 
> ```
> if (targetSet.fieldDetails && !targetSet.fieldDetails.some....) {
> ```
> though it's an unnecessary check for now, the absence of targetSet.fieldDetails might looks potentially throw error here since we're using else if insteaf of else above.

Sorry, I meant:
```
if (targetSet && !targetSet.fieldDetails.some....)
```
Component: Form Manager → Form Autofill
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review203168

I think the methods within `FormAutofillHandler` and `FormAutofillSection` should be given an underline prefix for the one which is private.
I will fix this in another bug.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:54
(Diff revision 5)
> -  winUtils: null,
> -
> -  /**
> +    /**
> -   * Enum for form autofill MANUALLY_MANAGED_STATES values
> +     * Enum for form autofill MANUALLY_MANAGED_STATES values
> -   */
> +     */
> -  fieldStateEnum: {
> +    this.fieldStateEnum = {

Since `fieldStateEnum` was in FormAutofillHandler and not used out of FormAutofillHandler, I would prefer to put it into FormAutofillSection only to clarify its scope.

I will change its name to `_FIELD_STATE_ENUM` for the private scope and const variable.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:524
(Diff revision 5)
>      }
>  
>      fieldDetail.state = nextState;
> -  },
> +  }
> +
> +  clearFieldState(focusedInput) {

In my original idea, `clearFieldState` is to reset one specific field state to "NORMAL" which means it is without preview value or not auto-filled.
IMO, the name of `handleFieldsChanged` can not express what the function exactly does.

About merging two functions, `resetFieldStates` is for the whole section, it does not need to check address or credit card field. That's why I remain two functions here.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:534
(Diff revision 5)
> +      targetSet = this.address;
> +    } else if (FormAutofillUtils.isCreditCardField(focusedInput)) {
> +      targetSet = this.creditCard;
> +    }
> +
> +    if (!targetSet.fieldDetails.some(detail => detail.state == "AUTO_FILLED")) {

`clearFieldState` is used in `async autofillFormFields(profile, focusedInput)` function:

```JS
let section = this.getSectionByElement(e.target);
section.clearFieldState(e.target);
```
so the elemenet must be in the section.
If the case happened, I think there might be some other errors. Hence, I would prefer to see the exception thrown.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:890
(Diff revision 5)
> +    let section = this.getSectionByElement(focusedInput);
> +    section.getAdaptedProfiles(originalProfiles);
> +    return originalProfiles;
> +  }
> +
> +  isNoSectionFilled() {

Yeah, function name changed to `hasFilledSection`, and I use `some` instead of `every`.
(In reply to Sean Lee [:seanlee][:weilonge] from comment #14)
> Comment on attachment 8923301 [details]
> Bug 1339731 - Refactor FormAutofillHandler to support multiple section
> machanism.
> 

> Since `fieldStateEnum` was in FormAutofillHandler and not used out of
> FormAutofillHandler, I would prefer to put it into FormAutofillSection only
> to clarify its scope.
> 
No problem, I was thinking the state will be used in other place, e.g. clear form button in the future. To get to know if the focused field is autofilled or not. Nonetheless, I'll handle that part in the other bug.

> > +  clearFieldState(focusedInput) {
> 
> In my original idea, `clearFieldState` is to reset one specific field state
> to "NORMAL" which means it is without preview value or not auto-filled.
> IMO, the name of `handleFieldsChanged` can not express what the function
> exactly does.
>
I'm fine with it. What I meant was the function doesn't really clear the state, but change or restore instead. 

> About merging two functions, `resetFieldStates` is for the whole section, it
> does not need to check address or credit card field. That's why I remain two
> functions here.
Okay, it's just a trade-off between conciseness and performance.  

> 
> `clearFieldState` is used in `async autofillFormFields(profile,
> focusedInput)` function:
> 
> ```JS
> let section = this.getSectionByElement(e.target);
> section.clearFieldState(e.target);
> ```
> so the elemenet must be in the section.
> If the case happened, I think there might be some other errors. Hence, I
> would prefer to see the exception thrown.
> 
I was just thinking an absence check is preferable if the undefined variable isn't guarded by all cases, though indeed as yet no chance it will happen. 

Thanks :D
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review203270

Thanks! per discussion offline, please close the issues we both concur with.
Attachment #8923301 - Flags: review?(ralin) → review+
Blocks: 1416664
Blocks: 1416665
Comment on attachment 8927801 [details]
Bug 1339731 - Refactor some duplicated codes and remove the unused method.

https://reviewboard.mozilla.org/r/199098/#review204326

Looks good! Thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:432
(Diff revision 1)
>          this.savedFieldNames = data;
>        }
>      }
>    },
>  
> +  getSearchParams(focusedInput) {

It seems a bit indirect to me since yet no other consumer would re-use it.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:137
(Diff revision 1)
>  
>    getFieldDetailByName(fieldName) {
>      return this._validDetails.find(detail => detail.fieldName == fieldName);
>    }
>  
> -  getFieldDetailsByElement(element) {
> +  _getTargetSet(element) {

It feels like we're introducing another conceptual layer "set" between form and section, and still every lookups go through this path.
Not so sure, but I assume it's fine if this API will be deprecated once Section class and actual section became one-to-one correspondence in the future.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:162
(Diff revision 1)
> +  }
> +
> +  getFilledRecordGUID(element) {
> +    let targetSet = this._getTargetSet(element);
> +    if (!targetSet) {
> +      return [];

maybe return null instead.
Attachment #8927801 - Flags: review?(ralin) → review+
Blocks: 1417803
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194480/#review205274

::: browser/app/profile/firefox.js:1712
(Diff revision 7)
>  // 2: saw the doorhanger
>  // 3: submitted an autofill'ed credit card form
>  pref("extensions.formautofill.creditCards.used", 0);
>  pref("extensions.formautofill.firstTimeUse", true);
>  pref("extensions.formautofill.heuristics.enabled", true);
> +pref("extensions.formautofill.section.enabled", false);

Let's turn it on by default.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:11
(Diff revision 7)
>   * Defines a handler object to represent forms that autofill can handle.
>   */
>  
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["FormAutofillHandler"];
> +this.EXPORTED_SYMBOLS = ["FormAutofillHandler"]; /* exported FormAutofillHandler */

In Form Autofill code base, we usually put `/* exported ... */` at the top of the file (above `"use strict"`).
Comment on attachment 8927801 [details]
Bug 1339731 - Refactor some duplicated codes and remove the unused method.

https://reviewboard.mozilla.org/r/199098/#review204326

> It seems a bit indirect to me since yet no other consumer would re-use it.

Same here. I also don't think we need this function. Or, this function should be designed in the `FormAutofillHandler` as it's actually related to `Handler` instead of `Content`.
Comment on attachment 8923301 [details]
Bug 1339731 - Refactor FormAutofillHandler to support multiple section machanism.

https://reviewboard.mozilla.org/r/194482/#review205298
Attachment #8923301 - Flags: review?(lchang) → review+
Comment on attachment 8927801 [details]
Bug 1339731 - Refactor some duplicated codes and remove the unused method.

https://reviewboard.mozilla.org/r/199098/#review205300

The patch looks good for now. However, I think we should next implement bug 1417803 and different section type classes as soon as possible to make it closer to the result of our discussion.
Attachment #8927801 - Flags: review?(lchang) → review+
Blocks: 1417834
Comment on attachment 8927801 [details]
Bug 1339731 - Refactor some duplicated codes and remove the unused method.

https://reviewboard.mozilla.org/r/199098/#review205362

Thanks for the review! :)

::: browser/extensions/formautofill/FormAutofillContent.jsm:432
(Diff revision 1)
>          this.savedFieldNames = data;
>        }
>      }
>    },
>  
> +  getSearchParams(focusedInput) {

This part will be reverted.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:137
(Diff revision 1)
>  
>    getFieldDetailByName(fieldName) {
>      return this._validDetails.find(detail => detail.fieldName == fieldName);
>    }
>  
> -  getFieldDetailsByElement(element) {
> +  _getTargetSet(element) {

Agree, and I would like to implement this in bug 1417834.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8492412ff6f6
Refactor FormAutofillHandler to support multiple section machanism. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/9faf8dcf5c6e
Refactor some duplicated codes and remove the unused method. r=lchang,ralin
Keywords: checkin-needed
This issue is caused by a missing line when I am doing rebase, and I am preparing the patch to fix it. Sorry for the mistake.
Flags: needinfo?(selee)
The test looks good now. Please help to land the patches again. Thanks.
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d494b07e2d36
Refactor FormAutofillHandler to support multiple section machanism. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/f345598ccaf9
Refactor some duplicated codes and remove the unused method. r=lchang,ralin
Keywords: checkin-needed
Please reopen the review request on Mozreview so we can land the patch
Flags: needinfo?(selee)
I just reopened the review request. Could you help to check the patch landing? Thank you.
Flags: needinfo?(selee) → needinfo?(nerli)
https://hg.mozilla.org/mozilla-central/rev/d494b07e2d36
https://hg.mozilla.org/mozilla-central/rev/f345598ccaf9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(nerli)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: