The Toolkit Password Manager (which is also used by SeaMonkey) lacks sort markers/arrows and the sorting is reset when clearing the filter (for no apparent reason). I'm attaching a patch for FF/Toolkit which will be a prerequisite for the SeaMonkey fix (which will be as simple as porting the sortDirection attribute changes). I'll file a new bug for the latter to allow this one to be closed independently from the other. Fixing SeaMonkey 2 would require this bug to be fixed on the 1.9.1 branch, that's why I'm requesting 1.9.1 approval at the same time. I aligned the implementation with the changes I made to SeaMonkey's Cookie Manager; see http://mxr.mozilla.org/comm-central/source/suite/common/permissions/cookieViewer.* for reference. If you look closely you'll see that the Password Manager, unlike the Cookie Manager, doesn't get notified of changes made in the background (saving passwords or adding sites to the reject list). Implementing those observers might have an impact on sorting but I think this is out of scope here.

Comment on attachment 369810 [details] [diff] [review] proposed patch Punting to dolske, he's a better reviewer and he should have cycles.

Comment on attachment 369810 [details] [diff] [review] proposed patch >+ if (lastSignonSortAscending) >+ sortedCol.setAttribute("sortDirection", "ascending"); >+ else >+ sortedCol.setAttribute("sortDirection", "descending"); sortedCol.setAttribute("sortDirection", lastSignonSortAscending ? "ascending" : "descending"; >+ // clear out the sortDirection attribute on the rest of the columns >+ var currentCol = sortedCol.parentNode.firstChild; >+ while (currentCol) { >+ if (currentCol != sortedCol && currentCol.localName == "treecol") >+ currentCol.removeAttribute("sortDirection"); >+ currentCol = currentCol.nextSibling; >+ } This can be smarter, because earlier in the function you know exactly what the last sorted column was (lastSignonSortColumn). Spin out the switch statement to something like getColumnByName(), call it early on with lastSignonSortColumn, and removeAttribute on it. >- lastSignonSortColumn = ""; >- lastSignonSortAscending = false; >+ lastSignonSortAscending = !lastSignonSortAscending; > LoadSignons(); I don't understand why you're toggling lastSignonSortAscending. In any case, move the set of lastSignonSortAscending into LoadSignons(). It resets the sort column there, so it should reset the sort order too. >--- a/toolkit/components/passwordmgr/content/passwordManagerExceptions.js >+++ b/toolkit/components/passwordmgr/content/passwordManagerExceptions.js Similar comments for the related changes in this file. :) Finally, I'd like to see a test for this. Ehsan already wrote one for filtering (see toolkit/components/passwordmgr/test/browser), you should be able to cut'n'paste a fair amount from it. Just add a few logins, synthesize clicking on columns, and verify the order is as expected. Should be fairly straightforward...

(In reply to comment #2) > >- lastSignonSortColumn = ""; > >- lastSignonSortAscending = false; > >+ lastSignonSortAscending = !lastSignonSortAscending; > > LoadSignons(); > > I don't understand why you're toggling lastSignonSortAscending. I added an inline comment. You'll see that this actually fixes a bug that was just not too visible until I introduced the sort markers: The sort direction is reset upon clearing the filter without this change. > Finally, I'd like to see a test for this. Ehsan already wrote one for filtering > (see toolkit/components/passwordmgr/test/browser), you should be able to > cut'n'paste a fair amount from it. Just add a few logins, synthesize clicking > on columns, and verify the order is as expected. Should be fairly > straightforward... Easier said then done... I'd first have to setup a FF build environment and then figure out how to start the test - make mochitest-browser-chrome, right?

Comment on attachment 387978 [details] [diff] [review] patch v2 (1.9.1 branch) >+ // The sort column didn't change. SortTree (called by >+ // SignonColumnSort) assumes we want to toggle the sort >+ // direction but here we don't so we have to trick it >+ lastSignonSortAscending = !lastSignonSortAscending; >+ SignonColumnSort(lastSignonSortColumn); Ah. I think you need similar code + comment for LoadRejects(). Could you also update the patch to apply cleanly? I'd like to test it out briefly before marking r+.

Comment on attachment 388328 [details] [diff] [review] test >+ function setFilter(string) { >+ filter.focus(); >+ filter.value = string; >+ EventUtils.synthesizeKey("VK_RETURN", {}, win); >+ setTimeout(runNextTest, 0); >+ } You'll need to pick up the fix for bug 507046 here. >+ case 0: >+ checkSortDirection(siteCol, true); >+ // Toggle sort direction on Host column >+ clickCol(siteCol); >+ break; Test looks good, but can you also have it check to see if the tree contents are in the correct order? [just the username column should be sufficient] See toolkit/components/satchel/test/test_form_autocomplete.html for an example, specifically checkMenuEntries() and getMenuEntries(). That's poking at a <tree> too, so I bet that code will work with minimal tweaks. r+ with these two things.

Sorry for the review delay, I promise to review the updates ASAP you post them. :/

Notes: - followed bug 507046 comment 6 - checked content order against sorted column instead of (always) first one because it's safer and more comprehensible (e.g. if checking Site column while sorting by Username, we'd have to guess whether http://mozilla.com or http://mozilla.org were listed first since both have user ehsan)

I hope this applies cleanly for you. (In reply to comment #5) > (From update of attachment 387978 [details] [diff] [review]) > >+ // The sort column didn't change. SortTree (called by > >+ // SignonColumnSort) assumes we want to toggle the sort > >+ // direction but here we don't so we have to trick it > >+ lastSignonSortAscending = !lastSignonSortAscending; > >+ SignonColumnSort(lastSignonSortColumn); > > Ah. I think you need similar code + comment for LoadRejects(). I don't think so. The above is only needed because that code is triggered when you clear the search. For Exceptions, there is no search.

Comment on attachment 394592 [details] [diff] [review] patch v2 (trunk) Looks and works fine! Thanks!

(Please checkin the code + test as a single changeset.)

(I have this in my checkin queue, just waiting for a green tree)

Pushed http://hg.mozilla.org/mozilla-central/rev/6bb21e3e54c3

This has been on trunk for a few days, asking for branch approvals (implicitly requesting the same for the test). Reasoning: * relatively simple patch with test so low chance of regressing things * not just visual polish but actually fixing a bug (sort order toggled upon clearing the filter) * SM 2.0 can only pick this up (bug 485702) if it lands on 1.9.1

Comment on attachment 387978 [details] [diff] [review] patch v2 (1.9.1 branch) Not the kind of fix we have to take on the stable branch. 1.9.1 approval denied.

(In reply to comment #15) > (From update of attachment 387978 [details] [diff] [review]) > Not the kind of fix we have to take on the stable branch. 1.9.1 approval > denied. OK, can we talk about a timely 1.9.2 decision then? Once that branch becomes stable the chances for approval there become equally low.

There is already an approval1.9.2 flag on the patch. Dietrich: Can you approve it?

Comment on attachment 394592 [details] [diff] [review] patch v2 (trunk) polish + bugfix + low footprint + tests = approval granted for 1.9.2.

pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4af6c444c16a