Refactor FormLikeFactory to its own module for use by Form Autofill

RESOLVED FIXED in Firefox 52

Status

()

P3
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [form autofill])

Attachments

(1 attachment)

FormLikeFactory provides a FormLike abstraction that lets one work with form fields not in a <form> as if they are in a <form>. From the beginning we will want Form Autofill to work outside <form> so we will want to re-use this helper for non-login-specific pages.
Comment hidden (mozreview-request)
Whiteboard: [form autofill]
Hi Matt, do you think it's a part of autofill MVP? I'm not sure how often we might need this form-like module for dealing with inputs. Maybe we should move it in MVP if necessary.
Flags: needinfo?(MattN+bmo)
I think it should probably be part of the MVP since it's trivial to adjust and affects compatibility with websites. I don't think it really matters what milestone it's in though especially since my work is being tracked with the estimates.
Flags: needinfo?(MattN+bmo)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8800899 [details]
Bug 1310049 - Refactor FormLikeFactory to its own module for use by Form Autofill.

https://reviewboard.mozilla.org/r/85714/#review85378

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1350
(Diff revision 1)
>  
>      if (aField.form) {
>        return this.createFromForm(aField.form);
>      }
>  
> -    let doc = aField.ownerDocument;
> +    let formLike = FormLikeFactory.createFromField(aField);

Just wondering whether we could set the action as optional parameter object like:
  FormLikeFactory.createFromField(aField,{
     action: LoginUtils._getPasswordOrigin(aField.ownerDocument.baseURI)
   });
   
But I'm fine if we rarely change the formLike properties in general.

::: toolkit/modules/FormLikeFactory.jsm:49
(Diff revision 1)
> -    LoginManagerContent._formLikeByRootElement.set(formLike.rootElement, formLike);
>      return formLike;
>    },
>  
>    /**
> -   * Create a FormLike object from a password or username field.
> +   * Create a FormLike object from an <input> in a document.

I know the formLike is created based on target input, but the formLike actually wraps multiple inputs inside. Maybe we could add "and wrap all the related inputs" or something in the end of comment?

::: toolkit/modules/FormLikeFactory.jsm:57
(Diff revision 1)
>     * Otherwise, create a FormLike with a rootElement (wrapper) according to
>     * heuristics. Currently all <input> not in a <form> are one FormLike but this
>     * shouldn't be relied upon as the heuristics may change to detect multiple
>     * "forms" (e.g. registration and login) on one page with a <form>.
>     *
>     * Note that two FormLikes created from the same field won't return the same FormLike object.

It seems the FormLike object for field requires more properties than the one for form. Maybe it's the polyfills that adding missing properties to make it just like the form element. Do we need explanation for that?

Comment 5

2 years ago
mozreview-review
Comment on attachment 8800899 [details]
Bug 1310049 - Refactor FormLikeFactory to its own module for use by Form Autofill.

https://reviewboard.mozilla.org/r/85714/#review85394

Sorry about the late review since I'm not familiar with Formlike object, but overall LGTM. I just left some small nits on the reviewboard.
Attachment #8800899 - Flags: review?(schung) → review+
BTW try build showed that test_getPasswordFields.js had timed out error.
Flags: needinfo?(MattN+bmo)
Comment hidden (mozreview-request)
(In reply to Steve Chung [:steveck] from comment #6)
> BTW try build showed that test_getPasswordFields.js had timed out error.

Thanks. Fixed in https://reviewboard.mozilla.org/r/85714/diff/1-2/ but the interdiff is messed up, it was just the missing export and let to var.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8800899 [details]
Bug 1310049 - Refactor FormLikeFactory to its own module for use by Form Autofill.

https://reviewboard.mozilla.org/r/85714/#review85378

> Just wondering whether we could set the action as optional parameter object like:
>   FormLikeFactory.createFromField(aField,{
>      action: LoginUtils._getPasswordOrigin(aField.ownerDocument.baseURI)
>    });
>    
> But I'm fine if we rarely change the formLike properties in general.

I think I would rather we fix password manager to handle the regular value instead of using a special cleansed version. Callers of createFromField are in places like browser/base/content/content.js which I would rather not have to teach about how to handle the action property in password manager, i.e. I think that detail should be encapsulated inside LoginFormFactory.

> I know the formLike is created based on target input, but the formLike actually wraps multiple inputs inside. Maybe we could add "and wrap all the related inputs" or something in the end of comment?

Right, it's just like a <form>, hence the name FormLike. The comment on line 12 explains what a FormLike is so I expanded on that comment instead.

> It seems the FormLike object for field requires more properties than the one for form. Maybe it's the polyfills that adding missing properties to make it just like the form element. Do we need explanation for that?

They have the same properties but the form ones have some copied from the <form> using _propsFromForm. I elaborated on the comment of line 13 to explain that we are emulating the properties of an HTMLFormElement.
Comment hidden (mozreview-request)

Comment 11

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/61ff4d4cfa3b
Refactor FormLikeFactory to its own module for use by Form Autofill. r=steveck

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61ff4d4cfa3b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1311521
You need to log in before you can comment on or make changes to this bug.