Work - LoginManager fills forms too late causing no u/p matching because of deault value username fields

RESOLVED FIXED

Status

Firefox for Metro
General
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
Some login input fields are preloaded with a value like in bugzilla with "email address".
This causes the old loginmanager code to only match on "email address".
This patch (which I'm about to post) changes it to work like the desktop Firefox one where we fill before these values are preloaded.

Similar code exists here: 
http://dxr.mozilla.org/mozilla-central/toolkit/components/passwordmgr/nsLoginManager.js.html#l202
(Assignee)

Comment 1

6 years ago
Created attachment 708838 [details] [diff] [review]
Patch v1.

Since you're already reviewing another login manager bug, I figured I'd send this one your way too.  

To explain a bit more, when you go to bugzilla.mozilla.org and are logged out, the username field contains 'email address'.  When you click on that field with your mouse it clears the text and allows you to enter your own username. 

The old login manager code looks in the db and tries to find a username called 'email address' that was previously saved.  But no one's username is really 'email address'. 

This new patch matches a blank string for username (accepting any usernames) and works the same as desktop Firefox because it is called before the default form value is filled.
Attachment #708838 - Flags: review?(ally)

Updated

6 years ago
Summary: LoginManager fills forms too late causing no u/p matching because of deault value username fields → Work - LoginManager fills forms too late causing no u/p matching because of deault value username fields
Whiteboard: feature=work
(Assignee)

Updated

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

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

Like bug 836915, I feel I'm not qualified to review this one solidly. mbrubeck has graciously agreed to provide a secondary review for this one as well.

- thank you for meaningful comments in your code :)

- same comment from 836915 about test coverage options applies here as well. 
- same (optional) comment from 836915 about strs& info for qa applies.

- nit-y question, but is there a particular reason you are using var or let? Some of my past reviewers have ranted at me pretty hard for *not* using let, so it seems to be a discouraged practice. 

- these look (on my screen anyway) like 4 space tabs. I thought we were a 2 space hard tab shop?
Attachment #708838 - Flags: review?(mbrubeck)
Attachment #708838 - Flags: review?(ally)
Attachment #708838 - Flags: feedback+
(Assignee)

Comment 3

6 years ago
Thanks for the review. 

I generally use 2 space tabs unless the file is already a 4-space tab file.
I usually also use let, but this code was copied from desktop so I just kept it the same.

I'm doing tests for both bugs inside bug 837295 by the way.
Comment on attachment 708838 [details] [diff] [review]
Patch v1.

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

Ugh, we should probably just switch to using toolkit's nsLoginManager.js, so we can stop porting their bug fixes to our version.  I believe the only reason our fork exists was for e10s support, which we no longer require.  :/
Attachment #708838 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 5

6 years ago
I was seriously considering that but I think the long term goal is to re-introduce e10s, so I'll push this in as is and we can reconsider that on the next bug.
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/projects/elm/rev/cbb24f3d4d11
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: feature=work → feature=work [completed-elm]
Depends on: 838711
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.