451955 - Improve password manager selection restoring code after filtering

Status: RESOLVED WORKSFORME

(Thunderbird :: Preferences, defect)

Description

[Simon Bünzli]

Steps to Reproduce:
1. Tools -> Options -> Security -> Saved Passwords...
2. Enter a search term and clear it
3. Enter a search term and clear it

Actual result:
After step 3, the text below the search field is stuck at "The following passwords match your search:" and the following error's in the console:

Error: table[selections[0]] is undefined
Source File: chrome://passwordmgr/content/passwordManagerCommon.js
Line: 176

This bug's present on Firefox 3.0 and the latest Trunk nightlies.

Comment 1

[Simon Bünzli]

Problem is that SignonClearFilter selects the nonexistent first element...

Ehsan: The selection restoration bits in SignonClearFilter look slightly wrong to me. What should singleSelection stand for? seltype="single" or the fact that at the moment only a single element is selected? The former would make slightly more sense (though I'm not sure why it has to be special cased to a wrong selection) whereas the latter is currently the case.

Comment 2

[(no longer active)]

This is not specific to the case where no passwords have already been entered, and can be reproduced elsewhere by entering a search term with no results and then changing it to produce no results again.

The points mentioned in comment 1 are valid points, and I'm not sure why I coded selection restoring this way, but it's clearly wrong.  Apart from the problem explained in comment 1, the SignonSaveState function may be called after entering a search string, provided that it doesn't turn up any results (because I'm using the number of the items in the result set as an indication of whether we're entering the filtered mode.)

I have a patch which solves both these issues.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Apart from the
> problem explained in comment 1, the SignonSaveState function may be called
> after entering a search string, provided that it doesn't turn up any results
> (because I'm using the number of the items in the result set as an indication
> of whether we're entering the filtered mode.)

The problematic code:

<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/content/passwordManager.js&rev=1.26&mark=308#299>

Comment 4

[(no longer active)]

Attachment: Patch

Comment 5

[Justin Dolske [:Dolske]]

So... This basically looks ok, but I'm unable to reproduce this bug on a current nightly (ie, without patch). Neither can I reproduce bug 430343 (which is probably a dupe of this one). Did the steps to reproduce change?

This patch needs a test, too.

Comment 6

[Simon Bünzli]

(In reply to comment #5)
> I'm unable to reproduce this bug on a current nightly

Have you tried on a clean profile (with NO saved passwords)?

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 337194 [details] [diff] [review]
Patch

Code changes are good, just need a test for r+.

Comment 9

[(no longer active)]

Updating to reality: I won't have the time to write a test for this patch in the near future.  Maybe someone else can pick it up?

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 451955 - Implement the password selection restoring in Password Manager.;

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

Comment 11

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review98274

::: toolkit/components/passwordmgr/content/passwordManager.js:657
(Diff revision 2)
>  }
>  
>  function CopyPassword() {
>    // Don't copy passwords if we aren't already showing the passwords & a master
>    // password hasn't been entered.
>    if (!showingPasswords && !masterPasswordLogin())

`showingPasswords` as a global boolean should be removed as it doesn't reflect the state of the individual login in question. Visibility is no longer an all-or-nothing case.

e.g. I shouldn't be able to filter based on the password field for logins where the password isn't visible/selected.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review98298

::: toolkit/components/passwordmgr/content/passwordManager.js:85
(Diff revision 2)
>  
>    togglePasswordsButton.label = kSignonBundle.getString("showPasswords");
>    togglePasswordsButton.accessKey = kSignonBundle.getString("showPasswordsAccessKey");
>    signonsIntro.textContent = kSignonBundle.getString("loginsDescriptionAll");
>    document.getElementsByTagName("treecols")[0].addEventListener("click", (event) => {
> +    StoreSelections(GetTreeSelections());

Please move store and restore inside the sort function.  The main issue is that you're storing unnecessarily.

::: toolkit/components/passwordmgr/content/passwordManager.js:349
(Diff revision 2)
>  }
>  
> +function StoreSelections(selections) {
> +  selectedSignons = [];
> +  let filterSet = signonsTreeView._filterSet;
> +  let table = filterSet.length ? filterSet : signons;

I would rather avoid duplicating this logic to get the visible list.  Can you get the ID from the selected row directly? I thought we already used the grid as an identifier.

::: toolkit/components/passwordmgr/content/passwordManager.js:350
(Diff revision 2)
>  
> +function StoreSelections(selections) {
> +  selectedSignons = [];
> +  let filterSet = signonsTreeView._filterSet;
> +  let table = filterSet.length ? filterSet : signons;
> +  selections.forEach(index => {

Nit: for...of is preferred

::: toolkit/components/passwordmgr/content/passwordManager.js:356
(Diff revision 2)
> +    selectedSignons.push(table[index].guid);
> +  });
> +}
> +
> +function RestoreSelections() {
> +  let filterSet = signonsTreeView._filterSet;

Same here

::: toolkit/components/passwordmgr/content/passwordManager.js:360
(Diff revision 2)
> +function RestoreSelections() {
> +  let filterSet = signonsTreeView._filterSet;
> +  let table = filterSet.length ? filterSet : signons;
> +  signonsTreeView.selection.clearSelection();
> +  for (let i = 0; i < table.length; i++) {
> +    if (selectedSignons.indexOf(table[i].guid) != -1) {

Use Array.prototype.includes

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

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

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review98298

> I would rather avoid duplicating this logic to get the visible list.  Can you get the ID from the selected row directly? I thought we already used the grid as an identifier.

After reading [1], I only find the way to get the selected indexes rather than the selected items. I guess we still need `let filterSet = signonsTreeView._filterSet;let table = filterSet.length ? filterSet : signons;` to get the current signons. Do I miss any possible solutions?

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITreeSelection

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review98274

> `showingPasswords` as a global boolean should be removed as it doesn't reflect the state of the individual login in question. Visibility is no longer an all-or-nothing case.
> 
> e.g. I shouldn't be able to filter based on the password field for logins where the password isn't visible/selected.

If I understand correctly, `showingPasswords` affects the selected password showing. The selected password showing will be implemented in bug 1257078. May I fix `showingPasswords` in bug 1257078?

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review98840

::: toolkit/components/passwordmgr/content/passwordManager.js:625
(Diff revision 3)
>  }
>  
>  function FilterPasswords() {
> +  let selectedSignons = StoreSelections(GetTreeSelections());
>    if (filterField.value == "") {
>      SignonClearFilter();

I found `filterField.value == ""` case will cause a nested StoreSelections/RestoreSelections pair, so the selection behavior will be incorrect in `filterField.value == ""` case.

The solution in this patch is to keep the selections locally rather than storing the selections in a global variable.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review99512

::: toolkit/components/passwordmgr/content/passwordManager.js:568
(Diff revision 3)
>    // Restore selection
>    if (singleSelection) {
>      signonsTreeView.selection.clearSelection();
>      for (let i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
>        let range = signonsTreeView._lastSelectedRanges[i];
>        signonsTreeView.selection.rangedSelect(range.min, range.max, true);
>      }
>    } else {
>      signonsTreeView.selection.select(0);
>    }

This should also be updated/replaced?

::: toolkit/components/passwordmgr/content/passwordManager.js:610
(Diff revision 3)
>  function SignonSaveState() {
>    // Save selection
>    let seln = signonsTreeView.selection;
>    signonsTreeView._lastSelectedRanges = [];
>    let rangeCount = seln.getRangeCount();

Why aren't you replacing/updating this method which is very similar?

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review99514

::: toolkit/components/passwordmgr/content/passwordManager.js:272
(Diff revision 3)
>    // restore the selection
>    let selectedRow = -1;
>    if (selectedNumber >= 0 && false) {
>      for (let s = 0; s < table.length; s++) {
>        if (table[s].number == selectedNumber) {
>          // update selection
>          // note: we need to deselect before reselecting in order to trigger ...Selected()
>          signonsTree.view.selection.select(-1);
>          signonsTree.view.selection.select(s);
>          selectedRow = s;
>          break;
>        }
>      }
>    }

This should also be updated/removed

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

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

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

Hi MattN,

Here is the change in the latest patch:
1. Update 'mochitest' to verify the selection behavior.
2. Some selection restoring codes are removed.

Since this bug depends on bug 1324136, I will rebase it again once bug 1324136 resolved. So please give this patch a feedback.
Thank you.

Comment 22

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review100034

::: toolkit/components/passwordmgr/content/passwordManager.js:339
(Diff revision 4)
> +  signonsTreeView.selection.clearSelection();
> +  for (let i = 0; i < table.length; i++) {
> +    if (selectedSignons.includes(table[i].guid)) {
> +      signonsTreeView.selection.rangedSelect(i, i, true);
> +    }
> +  };

Unnecessary semicolon here.

Comment 23

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

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

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Rebase the patch to latest m-c.

Comment 25

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

https://reviewboard.mozilla.org/r/97964/#review99512

> Why aren't you replacing/updating this method which is very similar?

I removed `SignonSaveState` and use `StoreSelections` instead.

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8817752 [details]
Bug 451955 - Implement the password selection restoring in Password Manager.;

Not a priority to look at this at the moment and the code is fragile and there are multiple interwoven issues in the code that I was trying to fix with my own patches. One fix to the manager related to this did land many months ago which should help.

Comment 27

[Mark Banner (:standard8)]

Sean's account is disabled, so unassigning him as I guess he won't be working on this.

Comment 28

[Wayne Mery (:wsmwk)]

This no longer occurs