Closed
Bug 451955
Opened 16 years ago
Closed 3 years ago
Improve password manager selection restoring code after filtering
Categories
(Thunderbird :: Preferences, defect)
Thunderbird
Preferences
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: zeniko, Unassigned)
References
()
Details
(Keywords: polish)
Attachments
(2 files)
(deleted),
patch
|
Dolske
:
review-
|
Details | Diff | Splinter Review |
(deleted),
text/x-review-board-request
|
Details |
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.
Reporter | ||
Comment 1•16 years ago
|
||
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.
Depends on: 327048
Updated•16 years ago
|
Whiteboard: [ui]
Comment 2•16 years ago
|
||
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.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Summary: Password search stuck on repeated search with no passwords stored → Improve password manager selection restoring code after filtering
Version: unspecified → Trunk
Comment 3•16 years ago
|
||
(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•16 years ago
|
||
Attachment #337194 -
Flags: review?(dolske)
Comment 5•16 years ago
|
||
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.
Reporter | ||
Comment 6•16 years ago
|
||
(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•16 years ago
|
||
Comment on attachment 337194 [details] [diff] [review]
Patch
Code changes are good, just need a test for r+.
Attachment #337194 -
Flags: review?(dolske) → review-
Comment 9•15 years ago
|
||
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?
Assignee: ehsan → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Whiteboard: [ui] → [passwords:management]
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
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.
Attachment #8817752 -
Flags: review?(MattN+bmo)
Comment 13•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 15•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review-reply |
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•8 years ago
|
||
mozreview-review |
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•8 years ago
|
||
mozreview-review |
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?
Attachment #8817752 -
Flags: review?(MattN+bmo)
Comment 19•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 21•8 years ago
|
||
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.
Attachment #8817752 -
Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
Comment 22•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 24•8 years ago
|
||
Rebase the patch to latest m-c.
Comment 25•8 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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.
Attachment #8817752 -
Flags: review?(MattN+bmo)
Updated•6 years ago
|
Updated•6 years ago
|
Priority: -- → P3
Comment 27•6 years ago
|
||
Sean's account is disabled, so unassigning him as I guess he won't be working on this.
Assignee: selee → nobody
Status: ASSIGNED → NEW
Updated•5 years ago
|
Component: Password Manager → Preferences
Priority: P3 → --
Product: Toolkit → Thunderbird
Whiteboard: [passwords:management]
Comment 28•3 years ago
|
||
This no longer occurs
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•