In Password Manager, clicking "Remove" resets display of "Saved Logins" dialog to top of list

VERIFIED FIXED in Firefox 55

Status

()

defect
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: cilias, Assigned: hashhar_dev)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [passwords:management])

Attachments

(1 attachment, 1 obsolete attachment)

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040218

In Password Manager, clicking "Remove" resets display to top of list.

Reproducible: Always
Steps to Reproduce:
1. Start the password manager
2. Scroll down to a point where the top of the list isn't in display
3. Select a password, and click "Remove"

Actual Results:  
The password is deleted, and the display goes back to the top of the list.

Expected Results:  
The password is deleted, and the display should remain at the same point within
the list.

Possibly related to Bug 210461.
I also see it in Thunderbird 0.5, but not in Firefox 0.8.
I'd be surprised if this isn't a duplicate, but I don't see one.

confirming this happens with a current 1.7 branch build on win2k. it also
happens with the 1.3.1 release.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Browser → Seamonkey
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051103 SeaMonkey/1.0b] (release) (W98SE)

Moreover, "Remove" resets the sort order to "Site, ascending" too.
For example, "Username, ascending" sort order is lost.
(In reply to comment #2)
> [Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051103 SeaMonkey/1.0b]
> (release) (W98SE)

(For the record, this build was a nightly :->)
Assignee: dveditz → nobody
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bug still there.
Severity: normal → minor
Duplicate of this bug: 449711
Whiteboard: [obsoleted when bug 588421 lands]
Confirmed. Bug still in Firefox 18.0.1.
Duplicate of this bug: 1242693
Duplicate of this bug: 1116463
Component: Passwords & Permissions → Password Manager
Product: SeaMonkey → Toolkit
Severity: minor → normal
OS: Windows XP → All
Hardware: x86 → All
Summary: In Password Manager, clicking "Remove" resets display to top of list → In Password Manager, clicking "Remove" resets display of ""Saved Logins" dialog to top of list
Summary: In Password Manager, clicking "Remove" resets display of ""Saved Logins" dialog to top of list → In Password Manager, clicking "Remove" resets display of "Saved Logins" dialog to top of list
Whiteboard: [obsoleted when bug 588421 lands] → [passwords:management]
Duplicate of this bug: 1081779
See Also: → 211352
I'll start working on a fix if somebody can tell me where to look. I've been hit my this many more times than I can deal with. Hoping that I make my first contribution.
(In reply to Ashhar Hasan from comment #10)
> I'll start working on a fix if somebody can tell me where to look. I've been
> hit my this many more times than I can deal with. Hoping that I make my
> first contribution.

https://dxr.mozilla.org/mozilla-central/search?q=DeleteSignon
(In reply to YF (Yang) from comment #11)
> (In reply to Ashhar Hasan from comment #10)
> > I'll start working on a fix if somebody can tell me where to look. I've been
> > hit my this many more times than I can deal with. Hoping that I make my
> > first contribution.
> 
> https://dxr.mozilla.org/mozilla-central/search?q=DeleteSignon

It looks like the function 'tree.treeBoxObject.ensureRowIsVisible(nextSelection);' isn't working since all the places where it is called (sorting the passwords) also have no effect.

Looking for the function definition I found https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dom/bindings/TreeBoxObjectBinding.cpp#614

I can't figure out what the function is doing.
Flags: needinfo?(yfdyh000)
I see the go to top behavior is due to FinalizeSignonDeletions, LoginHelper.notifyStorageChanged("removeLogin", storedLogin); and Services.obs.notifyObservers(null, "passwordmgr-dialog-updated", null);, through Browser Toolbox - Debugger.
Flags: needinfo?(yfdyh000)
See Also: → 1345883
I think I have tracked this down. I'll need to step through the code in gdb. The error is in the nsTreeBodyFrame::EnsureRowIsVisible() most likely. I'll report with more concrete information and hopefully a path by the end of the day.
Okay, I finally tracked it down and got it working on the manual cases I could think of. I have college now so I'll start the tests and hopefully by the time I come back, nothing breaks.

Also, where I should push the patch, Bugzilla or Mozreview?
(In reply to Ashhar Hasan from comment #15)
> Also, where I should push the patch, Bugzilla or Mozreview?

You can use both.
Attachment #8846247 - Flags: review?(schung)
Okay I have pushed two changesets, one to address this issue and one to address https://bugzilla.mozilla.org/show_bug.cgi?id=211352.

I saw that there was a function called SignonSaveState() (https://dxr.mozilla.org/mozilla-central/rev/4ceb9062ea8f4113bfd1b3536ace4a840a72faa7/toolkit/components/passwordmgr/content/passwordManager.js#592-602) so I was thinking maybe we could use that instead of the global variable. It will reduce deduplication. Should I do that or leave it as it is before a review.
(In reply to Loic from comment #16)
> (In reply to Ashhar Hasan from comment #15)
> > Also, where I should push the patch, Bugzilla or Mozreview?
> 
> You can use both.

I've added two changesets for review. After self reviewing my code I find that there's a better way to do this without adding globals. We can add a call to SignonSaveState (it saves the selection ranges) before all operations that modify the TreeView and restore the selection ranges in 'SignonReloadDisplay'?

How should I proceed?
Flags: needinfo?(schung)
Flags: needinfo?(epinal99-bugzilla2)
You have to set the previous patch as obsolete (Click on "Details" on the right, then "edit details") and cancel the review flag for it. Like that, the reviewer will only review the good patch(es).
Flags: needinfo?(epinal99-bugzilla2)
Attachment #8846271 - Attachment is obsolete: true
Attachment #8846271 - Flags: review?(schung)
Attachment #8846247 - Flags: review?(schung)
Attachment #8846247 - Flags: review?(dolske)
Attachment #8846247 - Flags: a11y-review?
Flags: needinfo?(schung)
I'm really sorry for the mess I made in the review process.

I have made a simple two line fix, tested manually for any UNEXPECTED differences in behavior and think this can now be reviewed whenever anyone wants to. Consider this locked before I get a review.

Sorry for not having read about MozReview properly before pushing to it.
Just wanted to remind :dolske that this is waiting for review.
Flags: needinfo?(dolske)
:Loic, I haven't seen any review activity yet? Did I do something wrong?
Flags: needinfo?(epinal99-bugzilla2)
No. CC'ing MattN as is in charge of this component, maybe he could find another reviewer who's available.
Flags: needinfo?(epinal99-bugzilla2) → needinfo?(MattN+bmo)
Sorry for the long delay here. :(

Part of this is due to MattN and I both feeling like the existing code here is quite fragile, has existing bugs, and is generally difficult/tedious to understand. There are also a couple of efforts underway to replace some of this code, but nothing immediate. So we'd generally prefer to avoid touching this code in its current form as much as possible.

As far as I can tell this does seem to fix the bug as described without making any of the other problems worse, so calling it r+.
Flags: needinfo?(dolske)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

https://reviewboard.mozilla.org/r/119332/#review138708
Attachment #8846247 - Flags: review?(dolske) → review+
Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

No need for A11Y review.
Attachment #8846247 - Flags: a11y-review?
Try push on mozreview is in progress.
Assignee: nobody → hashhar_dev
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3a3e0d6518f
Make selected row visible after removing/editing entries in password manager. r=Dolske
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3a3e0d6518f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thanks a lot for helping me submit my first patch.
Firefox 53.0.3 still jumps to the top of the list in saved logins window, when you delete an entry. Hm?
Oh I just noticed that it will be fixed only in v55, alright.. ;-) Thanks.
QA Whiteboard: [good first verify]
I see the issue is resolved in 56.0a1 (2017-06-15) x86. However, if removed line on the middle of list, the upper part of the visible range rather than the descent line is revealed, which seems to be an anti-traditional act.
[bugday-20170726] Verified the fix on 55.0b12 Linux and I confirm the behaviour in the previous comment.
I'm a little busy on the weekdays so I will work on it this weekend. Sorry for the huge dealy in getting back to this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to YF (Yang) from comment #38)
> I see the issue is resolved in 56.0a1 (2017-06-15) x86. However, if removed
> line on the middle of list, the upper part of the visible range rather than
> the descent line is revealed, which seems to be an anti-traditional act.

(In reply to Ashhar Hasan from comment #40)
> I'm a little busy on the weekdays so I will work on it this weekend. Sorry
> for the huge dealy in getting back to this.

Please handle this issue in a new bug. Once a bug has been resolved from a merge we normally only reopen if the code is reverted/backed out.
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
If a new bug report is opened, please report the bug number in this bug report.
You need to log in before you can comment on or make changes to this bug.