1310049 - Refactor FormLikeFactory to its own module for use by Form Autofill

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

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 1

[Matthew N. [:MattN]]

Attachment: Bug 1310049 - Refactor FormLikeFactory to its own module for use by Form Autofill.

This introduces LoginFormFactory which wraps FormLikeFactory for use with login-specific contexts.

Review commit: https://reviewboard.mozilla.org/r/85714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85714/

Comment 2

[Steve Chung [:steveck]]

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.

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Steve Chung [:steveck]]

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

[Steve Chung [:steveck]]

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.

Comment 6

[Steve Chung [:steveck]]

BTW try build showed that test_getPasswordFields.js had timed out error.

Comment 7

[Matthew N. [:MattN]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85714/diff/1-2/

Comment 8

[Matthew N. [:MattN]]

(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.

Comment 9

[Matthew N. [:MattN]]

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 10

[Matthew N. [:MattN]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85714/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61ff4d4cfa3b
Refactor FormLikeFactory to its own module for use by Form Autofill. r=steveck

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/61ff4d4cfa3b