Closed Bug 886990 Opened 6 years ago Closed 6 years ago

Passwords not saving or autofilling

Categories

(SeaMonkey :: Passwords & Permissions, defect)

defect
Not set

Tracking

(seamonkey2.21 fixed, seamonkey2.22 fixed)

RESOLVED FIXED
seamonkey2.22
Tracking Status
seamonkey2.21 --- fixed
seamonkey2.22 --- fixed

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 839961 stopped passwords from saving or autofilling automatically.

Merely loading the module suffices to start saving passwords globally but autofilling requires manually adding appropriate event listeners.

We can either add event listeners globally or just for the tabbed browser. (I don't know how many people expect passwords to autofill on RSS feeds.)
Attached patch Add listeners globally (obsolete) — Splinter Review
Attachment #767428 - Flags: feedback?(philip.chee)
Attachment #767428 - Flags: feedback?(iann_bugzilla)
Attached patch Add listeners locally (obsolete) — Splinter Review
Attachment #767429 - Flags: feedback?(philip.chee)
Attachment #767429 - Flags: feedback?(iann_bugzilla)
> We can either add event listeners globally or just for the tabbed browser. (I don't
> know how many people expect passwords to autofill on RSS feeds.)
What did the old code do?

> +var onContentLoaded = LoginManagerContent.onContentLoaded.bind(LoginManagerContent);
> +var onUsernameInput = LoginManagerContent.onUsernameInput.bind(LoginManagerContent);
Not sure why these are needed.

> +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> +    aWebProgress.DOMWindow.addEventListener("DOMContentLoaded", onContentLoaded, true);
> +    aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
Do we need to listen for "blur" as well? The old Login Manager code did this and Dolske wondered about this as well.

> -              .addProgressListener(this, Components.interfaces.nsIWebProgress.NOTIFY_LOCATION);
> +              .addProgressListener(this, Components.interfaces.nsIWebProgress.NOTIFY_LOCATION | Components.interfaces.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
[Perhaps a bit more wrapping of long lines?]
Do we need to care about STATE_RESTORING?

> +      <handler event="DOMAutoComplete" phase="capturing" action="this.LoginManagerContent.onUsernameInput(event);"/>
Why do we need capturing? The old code didn't use capturing.
(In reply to Philip Chee from comment #3)
> > We can either add event listeners globally or just for the tabbed browser.
> > (I don't know how many people expect passwords to autofill on RSS feeds.)
> What did the old code do?
It added listeners globally using the document loader to listen for state changes and then adding a DOMContentLoaded listener to the document to prefill it and add DOMAutoComplete listener to each detected login field (see below).

> > +var onContentLoaded = LoginManagerContent.onContentLoaded.bind(LoginManagerContent);
> > +var onUsernameInput = LoginManagerContent.onUsernameInput.bind(LoginManagerContent);
> Not sure why these are needed.
These methods expect to be invoked on the object. If I pass them directly to an event listener then that doesn't happen, so I need to bind them.

> > +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> > +    aWebProgress.DOMWindow.addEventListener("DOMContentLoaded", onContentLoaded, true);
> > +    aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
> Do we need to listen for "blur" as well? The old Login Manager code did this
> and Dolske wondered about this as well.
Works for me without it. I can't remember if it's possible to turn off autocomplete. If it is then I'll add a blur listener too. (Or maybe it should be a change listener, there should be fewer of those events.)

> > -              .addProgressListener(this, Components.interfaces.nsIWebProgress.NOTIFY_LOCATION);
> > +              .addProgressListener(this, Components.interfaces.nsIWebProgress.NOTIFY_LOCATION | Components.interfaces.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
> [Perhaps a bit more wrapping of long lines?]
[The lines were already long, I couldn't think of a sane way to wrap them. Please feel free to suggest something.]
> Do we need to care about STATE_RESTORING?
The old code didn't, probably because that doesn't delete event listeners.

> > +      <handler event="DOMAutoComplete" phase="capturing" action="this.LoginManagerContent.onUsernameInput(event);"/>
> Why do we need capturing? The old code didn't use capturing.
The old code added DOMAutoComplete event listeners to each field individually, which made the distinction between bubbling and capturing meaningless.
Duplicate of this bug: 886720
Comment on attachment 767428 [details] [diff] [review]
Add listeners globally

I tested this patch with bugzilla and one or two other sites and password saving and filling is working as expected.

>> [Perhaps a bit more wrapping of long lines?]
> [The lines were already long, I couldn't think of a sane way to wrap them.
> Please feel free to suggest something.]

Hmm tricky. Perhaps something like:
    var nsIWebProgress = Components.interfaces.nsIWebProgress;
    Components.classes['@mozilla.org/docloaderservice;1']
              .getService(nsIWebProgress)
              .addProgressListener(this, nsIWebProgress.NOTIFY_LOCATION |
                                         nsIWebProgress.NOTIFY_STATE_DOCUMENT);

In either case with or without wrapping, f=me.
Attachment #767428 - Flags: feedback?(philip.chee) → feedback+
Attachment #767428 - Flags: review?(bugzilla)
Comment on attachment 767428 [details] [diff] [review]
Add listeners globally

f=me too
Attachment #767428 - Flags: feedback?(iann_bugzilla) → feedback+
(In reply to neil@parkwaycc.co.uk from comment #4)
> (In reply to Philip Chee from comment #3)
> > > +  onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
> > > +    aWebProgress.DOMWindow.addEventListener("DOMContentLoaded", onContentLoaded, true);
> > > +    aWebProgress.DOMWindow.addEventListener("DOMAutoComplete", onUsernameInput, true);
> > Do we need to listen for "blur" as well? The old Login Manager code did this
> > and Dolske wondered about this as well.
> Works for me without it. I can't remember if it's possible to turn off
> autocomplete. If it is then I'll add a blur listener too. (Or maybe it
> should be a change listener, there should be fewer of those events.)

Fron what I see we need a blur listener for some more or less edge case: You can turn off usernmae/password autocomplete on pageload with signon.autofillForms set to false. When you then open a page with a username/password field, type in the username (and ignore the drop-down with the suggested username!) and then move the focus away from the username field, it does not fill in the password field. Triggering onUsernameInput on blur event would then fix this?
Attached patch With added change listener (obsolete) — Splinter Review
OK, added a change listener seems to fix that edge case.
Assignee: nobody → neil
Attachment #767428 - Attachment is obsolete: true
Attachment #767429 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #767428 - Flags: review?(bugzilla)
Attachment #767429 - Flags: feedback?(philip.chee)
Attachment #767429 - Flags: feedback?(iann_bugzilla)
Attachment #775518 - Flags: review?(bugzilla)
Attached patch Rebased patchSplinter Review
Patch did not apply anymore due to changes in nsSuiteGlue.js. I fixed that and reviewed the patch.
Attachment #775518 - Attachment is obsolete: true
Attachment #775518 - Flags: review?(bugzilla)
Attachment #777086 - Flags: review+
(In reply to Frank Wein from comment #10)
> Patch did not apply anymore due to changes in nsSuiteGlue.js. I fixed that
Or you could have just use -F6 option to patch...
Pushed comm-central changeset bad026f4a798.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.22
Comment on attachment 777086 [details] [diff] [review]
Rebased patch

[Approval Request Comment]
Regression caused by (bug #): 839961
User impact if declined: Login manager inoperable
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Low, behaviour is believed to match that previous to 839961.
String changes made by this patch: None
Attachment #777086 - Flags: approval-comm-aurora?
Attachment #777086 - Flags: approval-comm-aurora? → approval-comm-aurora+
Blocks: 908141
You need to log in before you can comment on or make changes to this bug.