Last Comment Bug 600551 - Password manager not working with input type="email"
: Password manager not working with input type="email"
Status: RESOLVED FIXED
[good first bug]
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b8
Assigned To: Bobby Johnson [:rjohnson19]
:
:
Mentors:
: 558370 605429 (view as bug list)
Depends on:
Blocks: 558370
  Show dependency treegraph
 
Reported: 2010-09-29 08:30 PDT by Matt Cosentino
Modified: 2010-12-10 19:04 PST (History)
9 users (show)
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+


Attachments
patch (3.35 KB, patch)
2010-10-09 07:07 PDT, Bobby Johnson [:rjohnson19]
no flags Details | Diff | Splinter Review
patch v2 (8.08 KB, patch)
2010-10-10 19:37 PDT, Bobby Johnson [:rjohnson19]
no flags Details | Diff | Splinter Review
patch v2.1 (8.08 KB, patch)
2010-10-10 20:37 PDT, Bobby Johnson [:rjohnson19]
mounir: feedback+
Details | Diff | Splinter Review
patch v2.2 (8.95 KB, patch)
2010-10-11 16:39 PDT, Bobby Johnson [:rjohnson19]
dolske: review-
Details | Diff | Splinter Review
patch v3 (8.32 KB, patch)
2010-10-26 16:22 PDT, Bobby Johnson [:rjohnson19]
dolske: review-
Details | Diff | Splinter Review
patch v4 (8.21 KB, patch)
2010-11-19 16:00 PST, Bobby Johnson [:rjohnson19]
dolske: review+
Details | Diff | Splinter Review

Description Matt Cosentino 2010-09-29 08:30:31 PDT
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
Comment 1 Jo Hermans 2010-09-29 12:49:02 PDT
That's because HTML5 has such an "email" type. And Firefox 4.0 uses the HTML5 parser, while the older versions didn't.
Comment 2 Matt Cosentino 2010-09-29 12:57:17 PDT
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.
Comment 3 (mostly gone) XtC4UaLL [:xtc4uall] 2010-09-29 14:33:41 PDT
Care to provide a Testcase and attach it?
Comment 4 Justin Dolske [:Dolske] 2010-09-29 18:34:22 PDT
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.
Comment 5 Bobby Johnson [:rjohnson19] 2010-10-09 07:07:02 PDT
Created attachment 482036 [details] [diff] [review]
patch

Identify username fields using mozIsTextField() instead of type="text".
Comment 6 Justin Dolske [:Dolske] 2010-10-09 20:52:06 PDT
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?
Comment 7 Mounir Lamouri (:mounir) 2010-10-10 15:30:16 PDT
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.
Comment 8 Mounir Lamouri (:mounir) 2010-10-10 15:33:47 PDT
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.
Comment 9 Bobby Johnson [:rjohnson19] 2010-10-10 19:37:18 PDT
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.
Comment 10 Bobby Johnson [:rjohnson19] 2010-10-10 20:37:56 PDT
Created attachment 482179 [details] [diff] [review]
patch v2.1

Corrected a typo.
Comment 11 Mounir Lamouri (:mounir) 2010-10-11 09:25:55 PDT
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"
Comment 12 Justin Dolske [:Dolske] 2010-10-11 14:51:15 PDT
*** Bug 558370 has been marked as a duplicate of this bug. ***
Comment 13 Bobby Johnson [:rjohnson19] 2010-10-11 16:39:16 PDT
Created attachment 482400 [details] [diff] [review]
patch v2.2

Addresses feedback from comment 11.
Comment 14 Phil Ringnalda (:philor) 2010-10-19 07:54:04 PDT
*** Bug 605429 has been marked as a duplicate of this bug. ***
Comment 15 Justin Dolske [:Dolske] 2010-10-23 18:48:01 PDT
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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-24 00:25:26 PDT
Why is that mixing use of .type and .getAttribute("type"), anyways? Seems like we should always use the former.
Comment 17 Bobby Johnson [:rjohnson19] 2010-10-24 07:50:57 PDT
(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")?
Comment 18 Bobby Johnson [:rjohnson19] 2010-10-26 16:22:08 PDT
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().
Comment 19 Justin Dolske [:Dolske] 2010-10-26 19:02:45 PDT
(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 20 Justin Dolske [:Dolske] 2010-11-14 19:07:03 PST
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".]
Comment 21 Matt Cosentino 2010-11-15 12:12:02 PST
(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?
Comment 22 Mounir Lamouri (:mounir) 2010-11-16 07:10:42 PST
(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.
Comment 23 Bobby Johnson [:rjohnson19] 2010-11-17 14:36:24 PST
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.
Comment 24 Mounir Lamouri (:mounir) 2010-11-17 15:46:21 PST
(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.
Comment 25 Bobby Johnson [:rjohnson19] 2010-11-19 16:00:29 PST
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.
Comment 26 Justin Dolske [:Dolske] 2010-12-10 16:15:23 PST
Comment on attachment 491982 [details] [diff] [review]
patch v4

Yay! Sorry this dragged out so long, my fault. :(
Comment 27 Justin Dolske [:Dolske] 2010-12-10 16:15:47 PST
Comment on attachment 491982 [details] [diff] [review]
patch v4

(oops, this is already blocking so no approval needed)
Comment 28 Justin Dolske [:Dolske] 2010-12-10 19:04:52 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/0020a1042b3f

Thanks for the patch!

Note You need to log in before you can comment on or make changes to this bug.