The default bug view has changed. See this FAQ.

Password manager not working with input type="email"

RESOLVED FIXED in mozilla2.0b8

Status

()

Toolkit
Password Manager
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Matt Cosentino, Assigned: rjohnson19)

Tracking

Trunk
mozilla2.0b8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100928 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100928 Firefox/4.0b7pre

Saved passwords are not populating when the username is in a input with a type of "email".  This worked in previous versions of Firefox and works in every other browser.  The problem seems to be that in previous versions the browser would report the type as "text" if it didn't recognize the value, and now it recognizes a type of "email".

Reproducible: Always
(Reporter)

Updated

7 years ago
Version: unspecified → Trunk

Comment 1

7 years ago
That's because HTML5 has such an "email" type. And Firefox 4.0 uses the HTML5 parser, while the older versions didn't.
(Reporter)

Comment 2

7 years ago
Maybe I wasn't clear.  I understand that's why it is recognizing the type of "email", that's a good thing.  The problem is that the password manager is only detecting inputs of type "text".  So now that inputs of type "email" are being recognized by the parser, the password manager does not populate the fields.
Care to provide a Testcase and attach it?
Severity: major → normal
Component: General → Password Manager
Product: Firefox → Toolkit
QA Contact: general → password.manager
Bug 557620 fixed this issue for form manager, via mozIsTextField(). Not sure why it didn't occur to me that login manager would need fixed too! [Although I vaguely remember pondering if it really made sense for login manager to accept these fields for usernames -- but it seems common enough already.]

We probably ought to block on this, and it's a simple enough fix.
Status: UNCONFIRMED → NEW
blocking2.0: --- → final+
Ever confirmed: true
Whiteboard: [good first bug]
(Assignee)

Comment 5

7 years ago
Created attachment 482036 [details] [diff] [review]
patch

Identify username fields using mozIsTextField() instead of type="text".
Assignee: nobody → rjohnson19
Status: NEW → ASSIGNED
Attachment #482036 - Flags: review?(dolske)
Mounir: what will nsGenericHTMLFormElement::IsSingleLineTextControlInternal() do for the other HTML5 input types not yet implemented; like color, datatime, or number? Will it only return |true| for the cases where the "types" table near the top of http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html lists "A text field" for "Control type"? IE, is this expected to always be a field type where the user can type in some kind of alphanumeric value?
OS: Windows 7 → All
Hardware: x86_64 → All
Hmm, this is a duplicate of bug 558370... Let's mark it as a blocker.

(In reply to comment #6)
> Mounir: what will nsGenericHTMLFormElement::IsSingleLineTextControlInternal()
> do for the other HTML5 input types not yet implemented; like color, datatime,
> or number?

For now, it will return true because 'color', 'number', 'range' and all date types are not recognized, this will be considered as 'text' type.

> Will it only return |true| for the cases where the "types" table
> near the top of
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
> lists "A text field" for "Control type"? IE, is this expected to always be a
> field type where the user can type in some kind of alphanumeric value?

All types considered as "Text fields" by the specifications will be considered as single line text control by Gecko. Including password and search.

If we use mozIsTextField(true), we should at least exclude 'search' and think about unknown types that will be recognized as text fields. I guess we can just ignore that case or check that .toLowerCase() doesn't return a type we know we do not support and is not a text field.
Blocks: 558370
Comment on attachment 482036 [details] [diff] [review]
patch

>-            if (form.elements[i].type == "text") {
>+            if (form.elements[i].mozIsTextField(true)) {

You should at least check that it's not a 'search' field and maybe check that it's not a type we do not support.

>diff --git a/toolkit/components/passwordmgr/test/test_basic_form_email.html b/toolkit/components/passwordmgr/test/test_basic_form_email.html
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/passwordmgr/test/test_basic_form_email.html
>@@ -0,0 +1,48 @@
>+<!DOCTYPE HTML>
>+<html>
>+<head>
>+  <title>Test for Login Manager</title>
>+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
>+  <script type="text/javascript" src="pwmgr_common.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body>
>+Login Manager test: simple form fill
>+<p id="display"></p>
>+
>+<div id="content" style="display: none">
>+
>+  <form id="form1" action="formtest.js">
>+    <p>This is form 1.</p>
>+    <input  type="email"      name="uname">
>+    <input  type="password"   name="pword">
>+
>+    <button type="submit">Submit</button>
>+    <button type="reset"> Reset </button>
>+  </form>

It would be nice to have all input types tested with some todo for the currently unsupported ones.
(Assignee)

Comment 9

7 years ago
Created attachment 482173 [details] [diff] [review]
patch v2

I've excluded search as well as all the unimplemented types according to https://developer.mozilla.org/en/HTML/Element/input#attr-type, and expanded the test.
Attachment #482036 - Attachment is obsolete: true
Attachment #482173 - Flags: review?(dolske)
Attachment #482036 - Flags: review?(dolske)
(Assignee)

Comment 10

7 years ago
Created attachment 482179 [details] [diff] [review]
patch v2.1

Corrected a typo.
Attachment #482173 - Attachment is obsolete: true
Attachment #482179 - Flags: review?(dolske)
Attachment #482173 - Flags: review?(dolske)
Comment on attachment 482179 [details] [diff] [review]
patch v2.1

I would add some todo like that:
todo(element.type, "color", "color shouldn't be recognized");
for all input types which are not supported so when a new input type will be supported, the test will fail. And add a comment asking to update the if so we don't keep a big condition when all types will be recognized.

And btw, change:
fieldType != "search"
to:
form.elements[i].type != "search"
Attachment #482179 - Flags: feedback+
Duplicate of this bug: 558370
(Assignee)

Comment 13

7 years ago
Created attachment 482400 [details] [diff] [review]
patch v2.2

Addresses feedback from comment 11.
Attachment #482179 - Attachment is obsolete: true
Attachment #482400 - Flags: review?(dolske)
Attachment #482179 - Flags: review?(dolske)
Duplicate of this bug: 605429
Comment on attachment 482400 [details] [diff] [review]
patch v2.2

Sorry for the delay getting to this.

>+            var fieldType = (form.elements[i].getAttribute("type") ? 

This can just be .getAttribute("type").toLowerCase(). If the attribute isn't set, you'll get empty-string so the .toLowerCase is fine to call (and is a no-op, basically).

>+            if (form.elements[i].mozIsTextField(true)
>+              && form.elements[i].type != "search" && fieldType != "color"
>+              && fieldType.indexOf("date") == -1
>+              && fieldType != "month" && fieldType != "week"
>+              && fieldType != "time" && fieldType != "number" 
>+              && fieldType != "range") {

So, I've bounced back and forth on this a few times, but I now think we should _not_ use mozIsTextField, and instead just explicitly list the types we want to support. For one, that's less code right now (and removes the todo). And I'm more comfortable with that as default behavior when, inevitably, new input types are defined. IE, I'd rather have them just not work (as username fields) until we explicitly decide they should.

Right now, that list would be: text, email, tel, url. Hmm, might as well allow "number" too.

r- for this easily fixed change, test looks great.
Attachment #482400 - Flags: review?(dolske) → review-
Why is that mixing use of .type and .getAttribute("type"), anyways? Seems like we should always use the former.
(Assignee)

Comment 17

7 years ago
(In reply to comment #16)
> Why is that mixing use of .type and .getAttribute("type"), anyways? Seems like
> we should always use the former.

Now that we want to list the types to support, I can use .type for for all but number. I would have to use getAttribute("type") to support number, because as an unrecognized type (bug 344616), .type will be "text".

This backwards compatibility is why type="email" works as a username in Firefox 3.6 and other browsers that don't support it.

So, to include number while being consistent, should I always use getAttribute("type")?
(Assignee)

Comment 18

7 years ago
Created attachment 486217 [details] [diff] [review]
patch v3

I tried to use
var fieldType = form.elements[i].getAttribute("type").toLowerCase();

But getAttribute("type") seems to return null for <input name="uname"> causing test errors: TypeError: form.elements[i].getAttribute("type") is null.

https://developer.mozilla.org/en/DOM/element.getAttribute agrees and I changed to use hasAttribute("type") as it suggests although I don't think it makes a difference here.

<input name="uname"> also made it problematic to only use getAttribute("type") because <input name="uname"> wouldn't be considered a username. So I used .type for everything but number.

Since we no longer exclude the other unimplemented types, I changed the relevant checks from is() to todo_is().
Attachment #482400 - Attachment is obsolete: true
Attachment #486217 - Flags: review?(dolske)
(In reply to comment #18)
> Created attachment 486217 [details] [diff] [review]
> patch v3
> 
> I tried to use
> var fieldType = form.elements[i].getAttribute("type").toLowerCase();
> 
> But getAttribute("type") seems to return null

Oh, blah, it's XUL in which getAttribute("foo") returns empty-string, in HTML it indeed returns null.
Comment on attachment 486217 [details] [diff] [review]
patch v3

Ack, sorry for taking so long to get back to this.

>         for (var i = pwFields[0].index - 1; i >= 0; i--) {
>-            if (form.elements[i].type == "text") {
>+            var fieldType = (form.elements[i].hasAttribute("type") ?
>+              form.elements[i].getAttribute("type").toLowerCase() : "");
>+            if (form.elements[i].type == "text"
>+              || form.elements[i].type == "email"
>+              || form.elements[i].type == "url"
>+              || form.elements[i].type == "tel"
>+              || fieldType == "number") {

Alas, this doesn't work as expected. If the |type| attribute is unknown, the |type| property will be "text" -- so the |fieldType == "number"| part will never be hit.

Also, evaluating "form.elements[i].type" repeatedly probably isn't great for perf.

So, I think this loop just needs to be:

  for (.....) {
      var element = form.elements[i];
      var fieldType = element.hasAttribute("type") ?
                      element.getAttribute("type").toLowerCase() : "text");
      if (fieldType == "text"  ||
          fieldType == "email" ||
          fieldType == "url"   ||
          fieldType == "number")
      {
          usernameField = element;
      }

That should default to "text" when no type is specified, and use the specified type otherwise.

I also tested this when script sets .type or the type-attribute to an unknown value. [Including the interesting case of |element.type = "blah"|, after which getAttribute("type") is "blah" but .type has reverted to "text".]
Attachment #486217 - Flags: review?(dolske) → review-
(Reporter)

Comment 21

7 years ago
(In reply to comment #20)
> I also tested this when script sets .type or the type-attribute to an unknown
> value. [Including the interesting case of |element.type = "blah"|, after which
> getAttribute("type") is "blah" but .type has reverted to "text".]

But don't we want to treat it as "text" in that case?
(In reply to comment #21)
> (In reply to comment #20)
> > I also tested this when script sets .type or the type-attribute to an unknown
> > value. [Including the interesting case of |element.type = "blah"|, after which
> > getAttribute("type") is "blah" but .type has reverted to "text".]
> 
> But don't we want to treat it as "text" in that case?

I would say so.
(Assignee)

Comment 23

6 years ago
I thought of a case which I think needs to be handled with this approach:
<input type="text">
<button></button>
<input type="password">

The button would be in form.elements and has no type attribute, it would be misidentified as the username field.
It isn't an input element, so _formFillService.markAsLoginManagerField(element) throws.

I can add a check that element.tagName.toLowerCase() == "input" as long as that doesn't hurt performance.
(In reply to comment #23)
> I thought of a case which I think needs to be handled with this approach:
> <input type="text">
> <button></button>
> <input type="password">
> 
> The button would be in form.elements and has no type attribute, it would be
> misidentified as the username field.
> It isn't an input element, so _formFillService.markAsLoginManagerField(element)
> throws.
> 
> I can add a check that element.tagName.toLowerCase() == "input" as long as that
> doesn't hurt performance.

Instead of:
      var fieldType = element.hasAttribute("type") ?
                      element.getAttribute("type").toLowerCase() : "text");
Doing:
      var fieldType = element.hasAttribute("type") ?
                      element.getAttribute("type").toLowerCase() : element.type);

Should prevent that.
(Assignee)

Comment 25

6 years ago
Created attachment 491982 [details] [diff] [review]
patch v4

As outlined in comment 20, except using element.type instead of "text" to handle the case in comment 23.

This only allows text, email, tel, url and number to be username fields.
Attachment #486217 - Attachment is obsolete: true
Attachment #491982 - Flags: review?(dolske)
Comment on attachment 491982 [details] [diff] [review]
patch v4

Yay! Sorry this dragged out so long, my fault. :(
Attachment #491982 - Flags: review?(dolske)
Attachment #491982 - Flags: review+
Attachment #491982 - Flags: approval2.0+
Comment on attachment 491982 [details] [diff] [review]
patch v4

(oops, this is already blocking so no approval needed)
Attachment #491982 - Flags: approval2.0+
Pushed http://hg.mozilla.org/mozilla-central/rev/0020a1042b3f

Thanks for the patch!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.