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

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Chris Ilias [:cilias]]

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

[Michael Lefevre]

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.

Comment 2

[Serge Gautherie (:sgautherie)]

[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.

Comment 3

[Serge Gautherie (:sgautherie)]

(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 :->)

Comment 4

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bug still there.

Comment 6

[Mikael Pettersson]

Confirmed. Bug still in Firefox 18.0.1.

Comment 10

[Ashhar Hasan]

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

[YF (Yang)]

(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

Comment 12

[Ashhar Hasan]

(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.

Comment 13

[YF (Yang)]

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.

Comment 14

[Ashhar Hasan]

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.

Comment 15

[Ashhar Hasan]

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

[Loic]

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

You can use both.

Comment 17

[Ashhar Hasan]

Attachment: Bug 234973 - Make selected row visible after removing/editing entries in password manager.

This commit adds a global variable savedSelection to passwordManager.js in
the passwordmgr component which keeps track of the new selection to bring
into view (it does not change the selection)after an entry is removed from
the password manager.

The variable is used as an argument to treeBoxObject.ensureRowIsVisible()
to bring it into view.

Review commit: https://reviewboard.mozilla.org/r/119332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119332/

Comment 18

[Ashhar Hasan]

Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119332/diff/1-2/

Comment 19

[Ashhar Hasan]

Attachment: Bug 234973 - Persist state when editing entries in password manager (211352).

This fixes the related issue 211352. This commit uses the global variable
savedSelection introduced in previous commit to also persist state when a login
is modified.

Review commit: https://reviewboard.mozilla.org/r/119374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119374/

Comment 20

[Ashhar Hasan]

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.

Comment 21

[Ashhar Hasan]

(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?

Comment 22

[Loic]

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).

Comment 23

[Ashhar Hasan]

Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119332/diff/2-3/

Comment 24

[Ashhar Hasan]

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 25

[Ashhar Hasan]

Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119332/diff/3-4/

Comment 26

[Ashhar Hasan]

Just wanted to remind :dolske that this is waiting for review.

Comment 27

[Ashhar Hasan]

:Loic, I haven't seen any review activity yet? Did I do something wrong?

Comment 28

[Loic]

No. CC'ing MattN as is in charge of this component, maybe he could find another reviewer who's available.

Comment 29

[Justin Dolske [:Dolske]]

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+.

Comment 30

[Justin Dolske [:Dolske]]

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

Comment 31

[Justin Dolske [:Dolske]]

Comment on attachment 8846247 [details]
Bug 234973 - Make selected row visible after removing/editing entries in password manager.

No need for A11Y review.

Comment 32

[Matthew N. [:MattN]]

Try push on mozreview is in progress.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3a3e0d6518f
Make selected row visible after removing/editing entries in password manager. r=Dolske

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3a3e0d6518f

Comment 35

[Ashhar Hasan]

Thanks a lot for helping me submit my first patch.

Comment 36

[berncity]

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

Comment 37

[berncity]

Oh I just noticed that it will be fixed only in v55, alright.. ;-) Thanks.

Comment 38

[YF (Yang)]

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

[Jagan]

[bugday-20170726] Verified the fix on 55.0b12 Linux and I confirm the behaviour in the previous comment.

Comment 40

[Ashhar Hasan]

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.

Comment 41

[Matthew N. [:MattN]]

(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.

Comment 42

[David E. Ross]

If a new bug report is opened, please report the bug number in this bug report.