1442303 - The arrow for sort is missing in Exceptions dialogues

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P5)

Description

[roxleitan]

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

Comment 1

[Johann Hofmann [:johannh]]

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

Comment 2

[Johann Hofmann [:johannh]]

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

Comment 3

[Trisha Gupta [:trisha]]

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

Comment 4

[Johann Hofmann [:johannh]]

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

Comment 5

[Trisha Gupta [:trisha]]

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

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

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 8

[Johann Hofmann [:johannh]]

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

Comment 9

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

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

Comment 10

[Johann Hofmann [:johannh]]

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

Comment 11

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

Noted your review. Changes are done. Thanks!

Comment 12

[Johann Hofmann [:johannh]]

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!

Comment 13

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

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

Comment 14

[Johann Hofmann [:johannh]]

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

Comment 15

[Trisha Gupta [:trisha]]

Attachment: 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?

Comment 16

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

Comment 17

[Johann Hofmann [:johannh]]

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

[Johann Hofmann [:johannh]]

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 19

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

Comment 20

[Johann Hofmann [:johannh]]

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 21

[Trisha Gupta [:trisha]]

Attachment: bug1442303.patch

Comment 22

[Johann Hofmann [:johannh]]

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

Comment 23

[Natalia Csoregi [:nataliaCs]]

The patch failed to apply, please take a look.

Comment 24

[Trisha Gupta [:trisha]]

Attachment: Bug 1442303 - The arrow for sort is missing in Exceptions dialogues

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

Comment 25

[Trisha Gupta [:trisha]]

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 26

[Johann Hofmann [:johannh]]

Comment on attachment 8980627 [details]
Bug 1442303 - The arrow for sort is missing in Exceptions dialogues

https://reviewboard.mozilla.org/r/246778/#review253264

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cef2c764f717
The arrow for sort is missing in Exceptions dialogues r=johannh

Comment 28

[Andrei Ciure[:aciure]]

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

Comment 29

[Mohammad Maruf Rahman]

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