Work - LoginManager does not fill in username fields on some sites like gmail due to html5 input types

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=work [completed-elm])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
While working on form fill I noticed that the login manager does not always fill in the username field.  The reason is because it only considers input types of type="text" right now.  But it should accept other input types too. 

This fix (attaching shortly) is similar to existing code already used on desktop firefox here:
http://dxr.mozilla.org/mozilla-central/toolkit/components/passwordmgr/nsLoginManager.js.html#l668
(Assignee)

Comment 1

6 years ago
Created attachment 708791 [details] [diff] [review]
Patch v1.
Assignee: nobody → netzen
Attachment #708791 - Flags: review?(ally)
(Assignee)

Updated

6 years ago
Summary: LoginManager does not fill in username fields on some sites like gmail → LoginManager does not fill in username fields on some sites like gmail due to html5 input types

Updated

6 years ago
Summary: LoginManager does not fill in username fields on some sites like gmail due to html5 input types → Work - LoginManager does not fill in username fields on some sites like gmail due to html5 input types
Whiteboard: feature=work
Comment on attachment 708791 [details] [diff] [review]
Patch v1.

Review of attachment 708791 [details] [diff] [review]:
-----------------------------------------------------------------

looks pretty solid, (the f+ not r+ is more a reflection on me than you) and I think the whitelist approach is the way to go, however:

- There is no test with this. I feel like this is something we could have caught with an automated test easily. I would be fine with a) a test b) a follow up bug to write a test c) a reason why we are unable to write a test

- Optionally, if you have it, it would be nice to list some sites other than gmail, STRs, or anything else that might help qa. 

- I don't think I know this code well enough to qualify as a reviewer of it. So, I'm going to find you someone else to do a driveby, and if it looks good to them, then you can either take their r+ or morph my f+ into an r+. 


question: the msg lists timA as the reviewer. Did you mean to have him review the patch as well?
Attachment #708791 - Flags: review?(ally) → feedback+
(Assignee)

Comment 3

6 years ago
Thanks Ally, I'll post a followup for a test.
I was originally going to put this login manager patch for review by TimA but I couldn't get his name to come up in autocomplete so I picked you!!! Lucky! :P

For step to reproduce, you can just have an <input type="email"> for a form with a password after it.  In that case the username will not be autofilled because it can't find the username field because it is not <input type="text">
(Assignee)

Updated

6 years ago
Depends on: 837295
Comment on attachment 708791 [details] [diff] [review]
Patch v1.

Review of attachment 708791 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/LoginManagerChild.js
@@ +326,5 @@
> +            if (fieldType == "text"  ||
> +                fieldType == "email" ||
> +                fieldType == "url"   ||
> +                fieldType == "tel"   ||
> +                fieldType == "number") {

I think that ((element instanceof HTMLInputElement && element.mozIsTextField(true)) is a better way to write this, unless there's some reason I'm not seeing that desktop Firefox did it the other way.

http://mxr.mozilla.org/mozilla-central/ident?i=mozIsTextField

Seconding ally's request for a test.
Attachment #708791 - Flags: feedback+
(Assignee)

Comment 5

6 years ago
Test bug is here: bug 837295.  I'll work on those tests Monday or possibly over the weekend.
Comment on attachment 708791 [details] [diff] [review]
Patch v1.

Okay, I see the mozIsTextField option was debated on desktop, and more-or-less concluded with bug 600551 comment 15.  Let's just copy desktop.
Attachment #708791 - Flags: feedback+ → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/projects/elm/rev/8fc5aeb14b6f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: feature=work → feature=work [completed-elm]
\o/ and thank you for the prompt test bug :)
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.