Closed Bug 1400112 Opened 6 years ago Closed 6 years ago

[Form Autofill] Collect the form details again when any form change happens

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file)

Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.
59 bytes, text/x-review-board-request
MattN
: review+
lchang
: review+
Details 
When the form is changed, `FormAutofillHandler.collectFormFields` should be invoked again to ensure all fields are with the latest update. The issue has affected BestBuy and WooCommerce[1].

Since FormAutofillHandler.form will not be changed with a new change of a form, `FormLike` for `FormAutofillHandler.form` should be updated to ensure the correctness.

[1] https://bluestarcoffeeroasters.com/
Depends on: 1398101
Priority: -- → P3
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185820

::: browser/extensions/formautofill/FormAutofillHandler.jsm:41
(Diff revision 3)
>       */
>      fieldDetails: [],
>      /**
>       * String of the filled address' guid.
>       */
>      filledRecordGUID: null,

Do you think `filledRecordGUID` should be clear as well in `updateForm` function?

For a new element added case, I think it should be kept.
For a brandnew form case, I think it should be cleared.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:137
(Diff revision 3)
> +    }
> +
> +    if (!Array.from(this.form.elements).find(e => e === element)) {
> +      log.debug("The element can not be found in the current form.");
> +      return true;
> +    }

Probably we need to check `deadObject` here.
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185820

> Do you think `filledRecordGUID` should be clear as well in `updateForm` function?
> 
> For a new element added case, I think it should be kept.
> For a brandnew form case, I think it should be cleared.

Currently, I am not sure how to distinguish the two cases in an accurate and efficient way.
Whiteboard: [form autofill:MVP]
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185858

::: browser/extensions/formautofill/FormAutofillHandler.jsm:129
(Diff revision 5)
> +    if (element.form.elements.length != this.form.elements.length) {
> +      log.debug("The count of form elements is changed.");
> +      return true;
> +    }

What if there's no `element.form`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:134
(Diff revision 5)
> +    if (!Array.from(this.form.elements).find(e => e === element)) {
> +      log.debug("The element can not be found in the current form.");
> +      return true;
> +    }

nit: `this.form.elements.indexOf(element) === -1` should work here. (`this.form.elements` should be an array already.)

::: browser/extensions/formautofill/FormAutofillHandler.jsm:150
(Diff revision 5)
> +    this.winUtils = this.form.rootElement.ownerGlobal.QueryInterface(Ci.nsIInterfaceRequestor)
> +      .getInterface(Ci.nsIDOMWindowUtils);

`nsIDOMWindowUtils` seems not related to elements nor form. I don't think it needs to be updated.
Attachment #8909077 - Flags: review?(lchang)
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185820

> Currently, I am not sure how to distinguish the two cases in an accurate and efficient way.

I personally prefer to keep it for now if we can't tell the difference between two cases. BTW, we might need to clear the highlight states as well if we're going to reset `filledRecordGUID` in the end.
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185858

> What if there's no `element.form`?

Thanks for addressing this. I've fixed it with `FormLikeFactory.createFromField` and added the related test to verify `rootElement == <body>` case.
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185942

::: browser/extensions/formautofill/FormAutofillContent.jsm:496
(Diff revision 6)
> -    } else if (!formHandler.isFormChangedSinceLastCollection) {
> +    } else if (formHandler.isFormUpdateNeeded(element)) {
> +      this.log.debug("form update is needed!!!");
> +      formHandler.updateForm(FormLikeFactory.createFromField(element));

Looks like `FormLike` will be created twice. Can we avoid that? I can think of two ways:

1. Create it ouside `isFormUpdateNeeded` and then pass it into the function along with `element` itself.
2. Invoke `updateForm` inside `isFormUpdateNeeded` (and maybe change the function name accordingly) if there isn't a chance that we want to know the needed status but don't want to update it. If we do, a second parameter can also work though.

What do you think?
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185942

> Looks like `FormLike` will be created twice. Can we avoid that? I can think of two ways:
> 
> 1. Create it ouside `isFormUpdateNeeded` and then pass it into the function along with `element` itself.
> 2. Invoke `updateForm` inside `isFormUpdateNeeded` (and maybe change the function name accordingly) if there isn't a chance that we want to know the needed status but don't want to update it. If we do, a second parameter can also work though.
> 
> What do you think?

The latest patch renames `isFormUpdateNeeded` to `updateFormIfNeeded` and modified its behavior.
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review186440

Thanks.
Attachment #8909077 - Flags: review?(lchang) → review+
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review185820

> Probably we need to check `deadObject` here.

Checking `deadObject` seems expensive. In order not to block this bug, I think we can discuss a proper solution in a separate bug.
Hey MattN, could you take a look this patch and review it? Thank you.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8909077 [details]
Bug 1400112 - Refresh `FormAutofillHandler.form` when the related form is changed.

https://reviewboard.mozilla.org/r/180674/#review197908
Attachment #8909077 - Flags: review?(MattN+bmo) → review+
Thank MattN and Luke for the review!
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(selee)
Keywords: checkin-needed
Thanks, Ryan!
Flags: needinfo?(selee)
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/61eb9c2d2319
Refresh `FormAutofillHandler.form` when the related form is changed. r=lchang,MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61eb9c2d2319
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.