Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID:20180301024724 [Affected versions]: Nightly 60.0a1 [Affected platforms]: Ubuntu 16.04 x64, Windows 10 x64 [Preconditions]: Have some websites in Exceptions list (more than 10) [Steps to reproduce]: 1.Launch Nightly 60.0a1 with a new profile 2.Open Exceptions dialogue from about:preferences#privacy - Cookies and Site Data section 3.Click "Website" or "Status" table headers to sort the websites listed in Exceptions panel [Expected result]: The websites should be sorted An arrow should be displayed in the table header (Websites or Status) depending on the field that is sorted

I don't think that's a new regression.

This is indeed not a new regression, the indicator is missing in all exception windows (there might be a dupe of this bug already). I'd say this isn't a priority, but I'm happy to mentor someone who wants to fix this. This patch from 9 years ago (bug 485701) shows how they added the sort indicator to the logins window (it's about setting the sortDirection attribute): https://bugzilla.mozilla.org/attachment.cgi?id=387978&action=diff

I'm interested to take this up, however, would I need access to windows development environment? Because I do not have it.

(In reply to Trisha from comment #3) > I'm interested to take this up, however, would I need access to windows > development environment? Because I do not have it. You should be able to reproduce this on any platform using the steps in comment 0. The exceptions dialog code is here: https://searchfox.org/mozilla-central/source/browser/components/preferences/permissions.js Here's another example of how the site permissions dialog uses sortDirection: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/preferences/sitePermissions.js#325 Thanks!

(In reply to Johann Hofmann [:johannh] from comment #4) > (In reply to Trisha from comment #3) > > I'm interested to take this up, however, would I need access to windows > > development environment? Because I do not have it. > > You should be able to reproduce this on any platform using the steps in > comment 0. Yes, I see the problem now. Thanks! > > The exceptions dialog code is here: > https://searchfox.org/mozilla-central/source/browser/components/preferences/ > permissions.js > Can you please give me a hint as to where exactly the change should be? I see there's resorting also happening in the code, so I understand I'll have to take care of that too while making my changes. Thanks. > Here's another example of how the site permissions dialog uses > sortDirection: > https://searchfox.org/mozilla-central/rev/ > 6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/preferences/ > sitePermissions.js#325 > > Thanks!

(In reply to Trisha from comment #5) > Can you please give me a hint as to where exactly the change should be? I > see there's resorting also happening in the code, so I understand I'll have > to take care of that too while making my changes. Thanks. The sorting in permissions.js is done here: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/preferences/permissions.js#347 As I mentioned, the task in this bug is to set the correct "sortDirection" attribute on the currently sorted column. There are different ways to solve this. The change in passwordManager.js from https://bugzilla.mozilla.org/attachment.cgi?id=387978&action=diff is one, the other one you can see in the _sortPermissions function here: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/components/preferences/sitePermissions.js#323 You should probably try to copy one of these two ways of doing that, I don't really have a preference for either :)

So, I tried copying from the __sortPermissions method to the current code, in synchronization with the different functions used in permission.js as compared to sitePermissions.js. For instance, the different type of sorting methods used. I would really be thankful for a feedback for this work-in-progress patch, so that I know how to proceed with this. Thanks a lot!

Comment on attachment 8959844 [details] [diff] [review] bug1442303.patch Review of attachment 8959844 [details] [diff] [review]: ----------------------------------------------------------------- This is a pretty good start, though looking at the way it works right now I think that we can actually simplify this code a bit. Considering that we already save the data we need in this._lastPermissionSortAscending and this._lastPermissionSortColumn, it seems like we can just remove all the code that sets or gets the data-* attributes on the column (since that data is kind of duplicated now). When we don't save the sortDirection as a data-* attribute string anymore, you could define it like let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; Then, the only code we would need is the one that sets the sortDirection attribute on aColumn, and which clears the sortDirection attribute from all other columns. I hope that makes sense :) Feel free to reach out if anything is unclear! Thanks ::: browser/components/preferences/permissions.js @@ +345,5 @@ > > > onPermissionSort(aColumn) { > + let permissions = Array.from(this._permissions.values()); > + There are trailing spaces here and in several other lines below. Please remove those :) @@ +365,5 @@ > + sortDirection = sortDirection === "ascending" ? "descending" : "ascending"; > + } > + > + if (sortDirection === "descending") { > + this._lastPermissionSortDescending = gTreeUtils.sort(this._tree, You don't need this._lastPermissionSortDescending, this._lastPermissionSortAscending == false is the equivalent of that. @@ +384,5 @@ > + column.setAttribute("data-isCurrentSortCol", "true"); > + column.setAttribute("sortDirection", sortDirection); > + column.setAttribute("data-last-sortDirection", sortDirection); > + > + return permissions; I don't think you need to return anything from this function, it works a little differently than the one in sitePermissions.js

Please let me know if there are any syntax changes to be made. Thanks!

Comment on attachment 8960702 [details] [diff] [review] bug1442303.patch Review of attachment 8960702 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, just a couple of small issues to fix before we should be ready for landing: ::: browser/components/preferences/permissions.js @@ +344,5 @@ > }, > > > + onPermissionSort(aColumn) { > + let permissions = Array.from(this._permissions.values()); I think this is unused, can you remove it please? @@ +354,5 @@ > this._lastPermissionSortColumn, > this._lastPermissionSortAscending); > this._lastPermissionSortColumn = aColumn; > + let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; > + trailing white space @@ +357,5 @@ > + let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; > + > + let cols = this._list.querySelectorAll("treecol"); > + cols.forEach(c => { > + c.removeAttribute("data-isCurrentSortCol"); I don't think you need to remove this attribute anymore, because we shouldn't be setting it :) @@ +360,5 @@ > + cols.forEach(c => { > + c.removeAttribute("data-isCurrentSortCol"); > + c.removeAttribute("sortDirection"); > + }); > + aColumn.setAttribute("data-isCurrentSortCol", "true"); See above, no need to set this anymore :) @@ +362,5 @@ > + c.removeAttribute("sortDirection"); > + }); > + aColumn.setAttribute("data-isCurrentSortCol", "true"); > + aColumn.setAttribute("sortDirection", sortDirection); > + aColumn.setAttribute("data-last-sortDirection", sortDirection); You can remove this as well! @@ +367,2 @@ > }, > + trailing white space

Noted your review. Changes are done. Thanks!

Comment on attachment 8961836 [details] [diff] [review] bug1442303.patch Review of attachment 8961836 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, either you removed a little too much code or you haven't correctly amended your commit. We still need to set the attribute on the selected column and remove the attribute from the other columns... Can you try to look into that? Thanks!

Sorry, I think I used the wrong command to amend the previously committed patch earlier. Hoping this is fine. Thanks :)

Comment on attachment 8962413 [details] [diff] [review] bug1442303.patch Review of attachment 8962413 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't actually work for me when trying it out (probably due to the below mentioned problmes). When you try it, please make sure that the little arrow icon actually appears when you click a column header. It would also be great if you could add an assertion that this class is set on the right column to https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/components/preferences/in-content/tests/browser_cookies_exceptions.js#212 Thanks! ::: browser/components/preferences/permissions.js @@ +353,5 @@ > this._lastPermissionSortColumn, > this._lastPermissionSortAscending); > this._lastPermissionSortColumn = aColumn; > + let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; > + let cols = this._list.querySelectorAll("treecol"); I don't think this._list is actually defined here. You can use document.querySelectorAll instead... @@ +357,5 @@ > + let cols = this._list.querySelectorAll("treecol"); > + cols.forEach(c => { > + c.removeAttribute("sortDirection"); > + }); > + aColumn.setAttribute("sortDirection", sortDirection); Ah, sorry, it turns out that aColumn is just the data-field-name string of that column. I didn't realize that. You obviously can't set an attribute to a string, so you could get the column like this: let column = document.querySelector(`treecol[data-field-name=${aColumn}]`); or you loop through the "cols" list and either removeAttribute or setAttribute, depending on whether the field name is the same as aColumn. I'd leave that up to you, I'm fine with both solutions :)

Should I attach another patch for adding an assertion that this class is set on the right column to the above mentioned file?

(In reply to Trisha from comment #15) > Created attachment 8963512 [details] [diff] [review] > bug1442303.patch > > Should I attach another patch for adding an assertion that this class is set > on the right column to the above mentioned file? You can either make a new patch or include it in the existing patch. I guess given that you've uploaded a patch for review now it might be easier to just add the assertion in a new patch :)

Comment on attachment 8963513 [details] [diff] [review] bug1442303.patch Review of attachment 8963513 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thank you! Please fix the two nits below and upload a new patch, but you won't need to get another review for that patch (unless you're unsure about anything). It would be great to have another patch for a test. Thanks! ::: browser/components/preferences/permissions.js @@ +343,3 @@ > return a.toLowerCase().localeCompare(b.toLowerCase()); > }, > nit: for some reason this changes makes my hg import behave weirdly. Can you just revert removing this line, please? Thanks :) @@ +355,5 @@ > + let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; > + let cols = document.querySelectorAll("treecol"); > + cols.forEach(c => { > + c.removeAttribute("sortDirection"); > + }); Nit: this can be on a single line: cols.forEach(c => c.removeAttribute("sortDirection"));

Comment on attachment 8963540 [details] [diff] [review] bug1442303.patch Review of attachment 8963540 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/preferences/permissions.js @@ +354,4 @@ > this._lastPermissionSortAscending); > this._lastPermissionSortColumn = aColumn; > + let sortDirection = this._lastPermissionSortAscending ? "descending" : "ascending"; > + let cols = this._list.querySelectorAll("treecol"); You should probably remove that line :)

Comment on attachment 8963888 [details] [diff] [review] bug1442303.patch Review of attachment 8963888 [details] [diff] [review]: ----------------------------------------------------------------- Hi Trisha, do you think this is ready to land? You can set checkin-needed or do a try run if you like. I'm happy to help...

The patch failed to apply, please take a look.

I can indeed see an arrow displayed in the table header (Websites or Status) depending on the field that is sorted when I run the patch. I re-submitted the patch using MozReview just now. Wonder why it failed to apply. Can you please review? Thanks :)

Comment on attachment 8980627 [details] Bug 1442303 - The arrow for sort is missing in Exceptions dialogues https://reviewboard.mozilla.org/r/246778/#review253264

Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cef2c764f717 The arrow for sort is missing in Exceptions dialogues r=johannh

I have reproduced this bug with Nightly 61.0a1 (2018-04-15) on Windows 10, 64 Bit! This bug's fix is verified with latest Nightly! Build ID 20180624100132 User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0