[Form Autofill] Auto-create new profiles based off submitted form data

ASSIGNED
Assigned to

Status

()

Toolkit
Form Manager
ASSIGNED
3 years ago
12 hours ago

People

(Reporter: MarcoM, Assigned: steveck, NeedInfo)

Tracking

(Blocks: 2 bugs)

Trunk
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [form autofill:M2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Breaking down Story into multiple smaller, easier-to-estimate bugs.  These individual bugs are dependencies which block the completion of the whole story.  The team provides point estimates to each of the individual bugs instead of the entire story.
(Reporter)

Updated

3 years ago
No longer blocks: 950073
Flags: firefox-backlog+
This is quite difficult to do for all locales if the page isn't using the new attributes Google proposed[1] as it would rely on many heuristics. I would suggest we implement this in toolkit so Android can use the same backend.

[1] http://wiki.whatwg.org/wiki/Autocomplete_Types
Component: General → Form Manager
Product: Firefox → Toolkit
Hardware: x86_64 → All
(Reporter)

Updated

3 years ago
Whiteboard: [form autofill] p=0 [qa-] → [form autofill] p=1 [qa-]
Depends on: 1009935
(Reporter)

Updated

3 years ago
Points: --- → 1
QA Whiteboard: [qa-]
Whiteboard: [form autofill] p=1 [qa-] → [form autofill]

Updated

3 months ago
Whiteboard: [form autofill] → [form autofill:MVP]

Updated

2 months ago
Whiteboard: [form autofill:MVP] → [form autofill:M2]
(Assignee)

Updated

2 months ago
Depends on: 1019483
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Depends on: 1348193
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a month ago
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

Hi MattN/Luke
This patch is simply about applying profile saving while form submitted. Profile updating and doorhanger will be introduced in another bugs. Test will be added once you're ok with the patch(or you prefer to address bug 1348193 first), thanks!
Attachment #8847056 - Flags: feedback?(lchang)
Attachment #8847056 - Flags: feedback?(MattN+bmo)

Updated

a month ago
Assignee: nobody → schung

Comment 5

a month ago
mozreview-review
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review125828

::: browser/extensions/formautofill/FormAutofillContent.jsm:394
(Diff revision 2)
>      formFillController.markAsAutofillField(field);
>    },
> +
> +  _maybeSaveProfile(guid, details) {
> +    let profile = this._detailsToProfile(details);
> +    if (Object.keys(profile).length < AUTOFILL_FIELDS_THRESHOLD) {

Does it mean that we don't save the profile unless 3 or more fields are actually filled?
(Assignee)

Comment 6

a month ago
mozreview-review-reply
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review125828

> Does it mean that we don't save the profile unless 3 or more fields are actually filled?

My original idea is for preventing meaningless profile saved in storage, since it shouldn't be a common case that the valid profile is created by less than 3 fields. I'll ni Juwei for more insight form UX's perspective.
(Assignee)

Comment 7

a month ago
Hi Juwei, do you think we should set a threshold while saving the profile after form submitted? I left my thoughts in comment 6, but I'm not quite sure it's ok because we don't have any limitation while saving the profile via preference.
Flags: needinfo?(jhuang)

Comment 8

a month ago
I think 3 fields is a reasonable number. If it's less than 3 fields, users still can use form history to fill out the form.
Flags: needinfo?(jhuang)

Comment 9

a month ago
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

Looks good. Thanks.
Attachment #8847056 - Flags: feedback?(lchang) → feedback+
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review127488

I guess this mostly needs tests but otherwise it's not too complicated.

::: browser/extensions/formautofill/FormAutofillContent.jsm:382
(Diff revision 2)
>        this._formsDetails.set(form.rootElement, formHandler);
> +      // TODO: should be able to support non-formElement form in bug 1348193.
> +      form.rootElement.addEventListener("submit", () => {
> +        let {filledProfileGUID, fieldDetails} = formHandler;
> +        this._maybeSaveProfile(filledProfileGUID, fieldDetails);
> +      }, {once: true});

I don't think `once` is what you want. If I submit a form, get an error, then click back to make a correction and the page is in the back-forward (bfcache) then I think you won't get the event when I submit again. You should also consider if the pages calls preventDefault for the "submit" event. I forget the timing of the events because no form code in mozilla-central uses the submit event, they all use a "earlyformsubmit" observer before the submit event which also means it doesn't hold a <form> reference. For consistency and to make it easier to avoid leaks I think it may be worth switching to a form submit observer which then checks if the form element is a relevant one. See https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/toolkit/components/passwordmgr/LoginManagerContent.jsm#53-69,146 for an example.

::: browser/extensions/formautofill/FormAutofillContent.jsm:399
(Diff revision 2)
> +    if (Object.keys(profile).length < AUTOFILL_FIELDS_THRESHOLD) {
> +      return;
> +    }
> +
> +    if (guid) {
> +      // TODO: get the profile and check if user update the fields.

This can be done in a different bug/commit if you want… I think it will be easier to review separately.

::: browser/extensions/formautofill/FormAutofillContent.jsm:401
(Diff revision 2)
> +    }
> +
> +    if (guid) {
> +      // TODO: get the profile and check if user update the fields.
> +    } else {
> +      Services.cpmm.sendAsyncMessage("FormAutofill:SaveProfile", {profile});

Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that.
Status: NEW → ASSIGNED
Summary: Story Breakdown - [Form Autofill] Auto-create profiles based off submitted form data → [Form Autofill] Auto-create new profiles based off submitted form data
QA Whiteboard: [qa-]
Attachment #8847056 - Flags: feedback?(MattN+bmo) → feedback+
(Assignee)

Comment 11

29 days ago
mozreview-review-reply
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review127488

> Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that.

You mean we shouldn't save the profile if the data is not from autofill storage directly? I didn't know that we have such requirement in MVP. Let me doublecheck with Luke again.
> > Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that.
> 
> You mean we shouldn't save the profile if the data is not from autofill
> storage directly? I didn't know that we have such requirement in MVP. Let me
> doublecheck with Luke again.

I think Matt was saying that we shouldn't save the profile which is identical to one of the saved profiles in the storage but wasn't autofilled by Form Autofill feature. I personally think we can do this in MVP.
(Assignee)

Comment 13

29 days ago
(In reply to Luke Chang [:lchang] from comment #12)
> > > Is this bug going to handle not saving if a profile matches data which wasn't autofilled? I know there was talk about not doing that for the MVP but I think we'll see what users think about that.
> > 
> > You mean we shouldn't save the profile if the data is not from autofill
> > storage directly? I didn't know that we have such requirement in MVP. Let me
> > doublecheck with Luke again.
> 
> I think Matt was saying that we shouldn't save the profile which is
> identical to one of the saved profiles in the storage but wasn't autofilled
> by Form Autofill feature. I personally think we can do this in MVP.

Oh yes, I think it's not difficult if we only perform strict comparison. I'll implement this in ProfileStorage add API(maybe redirect to update API since UX prefers to treat it as "update" than doing nothing).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review133052

::: commit-message-e9e23:1
(Diff revision 4)
> +Bug 990219 - Part 1: Add submit event listener and dispatch saveProfile message. r?MattN

The commit message is now stale since the submit event listener isn't added/used
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hi Steve,

We should avoid saving user's profile in the private browsing mode. Is it possible to cover that part in this bug?
Flags: needinfo?(schung)
Comment on attachment 8847056 [details]
Bug 990219 - Part 1: Add earlyformsubmit observer and dispatch saveProfile message.

https://reviewboard.mozilla.org/r/120096/#review137060

Please add the private browsing check and add a mochitest for it.

::: browser/extensions/formautofill/FormAutofillContent.jsm:322
(Diff revision 5)
> +  _detailsToProfile(details) {
> +    let profile = {};
> +
> +    details.forEach(detail => {

Have you considered putting this method on the handler instead of FormAutofillContent?

::: browser/extensions/formautofill/FormAutofillContent.jsm:363
(Diff revision 5)
> +  _saveProfile(guid, profile) {
> +    if (guid) {

As Luke said, we need to use PrivateBrowsingUtils.isContentWindowPrivate to not save anything to storage in PB mode. You will need to do that check in a method which has a reference to the window e.g. use the .ownerDocument.defaultView from the form rootElement.

::: browser/extensions/formautofill/FormAutofillContent.jsm:365
(Diff revision 5)
> +   * @param  {Object} profile
> +   *         The new profile used to add or overwrite the old one.
> +   */
> +  _saveProfile(guid, profile) {
> +    if (guid) {
> +      // TODO: get the profile and check if user update the fields.

Please add a bug number

::: browser/extensions/formautofill/test/unit/test_onFormSubmitted.js:77
(Diff revision 5)
> +      },
> +    },
> +  },
> +];
> +
>  add_task(function* () {

Please add a name to this function so it shows in logs. I didn't notice it was missing before
Attachment #8847056 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8858733 [details]
Bug 990219 - Part 2: Avoid saving the duplicate profile.

https://reviewboard.mozilla.org/r/130768/#review137068

::: browser/extensions/formautofill/ProfileStorage.jsm:323
(Diff revision 2)
>    _saveImmediately() {
>      return this._store._save();
>    },
> +
> +  /**
> +   * Check if storage has the same profile saved.

s/the same/a matching/

::: browser/extensions/formautofill/ProfileStorage.jsm:327
(Diff revision 2)
> +  /**
> +   * Check if storage has the same profile saved.
> +   * @param {Profile} targetProfile
> +   *        The profile for duplication check before saving.
> +   * @returns {string|null}
> +   *          Return guid string if there's a matched profile in storage or null if not.

s/matched/matching/

::: browser/extensions/formautofill/ProfileStorage.jsm:329
(Diff revision 2)
> +   * @param {Profile} targetProfile
> +   *        The profile for duplication check before saving.
> +   * @returns {string|null}
> +   *          Return guid string if there's a matched profile in storage or null if not.
> +   */
> +  _dupProfileChecking(targetProfile) {

Nit: `_findMatchingProfile`

::: browser/extensions/formautofill/ProfileStorage.jsm:332
(Diff revision 2)
> +      if (size != Object.keys(profile).length - this.INTERNAL_FIELDS.length) {
> +        continue;
> +      }
> +
> +      let data = this._clone(profile);
> +      this.INTERNAL_FIELDS.forEach((field) => {
> +        delete data[field];
> +      });
> +
> +      for (let key in targetProfile) {
> +        if (!data[key] || data[key] != targetProfile[key]) {
> +          break;
> +        }
> +        delete data[key];
> +      }
> +      if (Object.keys(data).length == 0) {
> +        return profile.guid;
> +      }

Can you factor this out into a separate `_areProfilesMatching` method in this JSM? Sync also needs the same type of method.
Attachment #8858733 - Flags: review?(MattN+bmo)
(Assignee)

Updated

19 hours ago
Blocks: 1360079
(Assignee)

Updated

14 hours ago
Blocks: 1303513
No longer blocks: 1360079
Flags: needinfo?(schung)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

12 hours ago
(In reply to Luke Chang [:lchang] from comment #20)
> Hi Steve,
> 
> We should avoid saving user's profile in the private browsing mode. Is it
> possible to cover that part in this bug?

I added the private browsing mode checking in the first patch.

Hi Matt, do you prefer to land the Bug 1325538 first then add mochitest on top of it, or land this patch first then add private browsing mochitest in Bug 1325538? Since you've reviewed both patches and the they are all updated, I'm fine for both options if we could land one of the patches first.
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.