Refactor FormLikeFactory to its own module for use by Form Autofill

RESOLVED FIXED in Firefox 52

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 years ago
3 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.
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 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 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)
(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)
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.
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
https://hg.mozilla.org/mozilla-central/rev/61ff4d4cfa3b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.