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)

(Reporter)

Description

16 years ago
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.

Comment 1

15 years ago
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

Updated

9 years ago
Whiteboard: [obsoleted when bug 588421 lands]

Comment 6

6 years ago
Confirmed. Bug still in Firefox 18.0.1.

Updated

3 years ago
Duplicate of this bug: 1242693

Updated

3 years ago
Duplicate of this bug: 1116463

Updated

3 years ago
Component: Passwords & Permissions → Password Manager
Product: SeaMonkey → Toolkit

Updated

3 years ago
Severity: minor → normal
OS: Windows XP → All
Hardware: x86 → All

Updated

3 years ago
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

Updated

3 years ago
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
(Assignee)

Comment 10

2 years ago
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.

Comment 11

2 years ago
(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
(Assignee)

Comment 12

2 years ago
(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)

Comment 13

2 years ago
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)

Updated

2 years ago
See Also: → 1345883
(Assignee)

Comment 14

2 years ago
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.
(Assignee)

Comment 15

2 years ago
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?

Comment 16

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

You can use both.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8846247 - Flags: review?(schung)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
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.
(Assignee)

Comment 21

2 years ago
(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)

Comment 22

2 years ago
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8846271 - Attachment is obsolete: true
Attachment #8846271 - Flags: review?(schung)
(Assignee)

Updated

2 years ago
Attachment #8846247 - Flags: review?(schung)
Attachment #8846247 - Flags: review?(dolske)
Attachment #8846247 - Flags: a11y-review?
(Assignee)

Updated

2 years ago
Flags: needinfo?(schung)
(Assignee)

Comment 24

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
Just wanted to remind :dolske that this is waiting for review.
Flags: needinfo?(dolske)
(Assignee)

Comment 27

2 years ago
:Loic, I haven't seen any review activity yet? Did I do something wrong?
Flags: needinfo?(epinal99-bugzilla2)

Comment 28

2 years ago
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 30

2 years ago
mozreview-review
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

Comment 33

2 years ago
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

Comment 34

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3a3e0d6518f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 35

2 years ago
Thanks a lot for helping me submit my first patch.

Comment 36

2 years ago
Firefox 53.0.3 still jumps to the top of the list in saved logins window, when you delete an entry. Hm?

Comment 37

2 years ago
Oh I just noticed that it will be fixed only in v55, alright.. ;-) Thanks.
QA Whiteboard: [good first verify]

Comment 38

2 years ago
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.

Comment 39

2 years ago
[bugday-20170726] Verified the fix on 55.0b12 Linux and I confirm the behaviour in the previous comment.
(Assignee)

Comment 40

2 years ago
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.
(Assignee)

Updated

2 years ago
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
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED

Comment 42

2 years ago
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.