The arrow for sort is missing in Exceptions dialogues

RESOLVED FIXED in Firefox 62

Status

()

defect
P5
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: roxana.leitan, Assigned: trisha, Mentored)

Tracking

60 Branch
Firefox 62
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

a year ago
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.
Whiteboard: [storage-v2][triage]
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
No longer blocks: storage-v2
Mentor: jhofmann
Priority: -- → P5
Summary: The arrow for sort is missing in Exceptions dialogue → The arrow for sort is missing in Exceptions dialogues
Whiteboard: [storage-v2][triage]
(Assignee)

Comment 3

a year ago
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!
Assignee: nobody → guptatrisha97
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a year ago
(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 :)
(Assignee)

Comment 7

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
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!
Attachment #8959844 - Flags: feedback?(jhofmann)
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
Attachment #8959844 - Flags: feedback?(jhofmann) → feedback+
(Assignee)

Comment 9

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Please let me know if there are any syntax changes to be made. Thanks!
Attachment #8959844 - Attachment is obsolete: true
Attachment #8960702 - Flags: review?(jhofmann)
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
Attachment #8960702 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 11

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Noted your review. Changes are done. Thanks!
Attachment #8960702 - Attachment is obsolete: true
Attachment #8961836 - Flags: review?(jhofmann)
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!
Attachment #8961836 - Flags: review?(jhofmann)
(Assignee)

Comment 13

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Sorry, I think I used the wrong command to amend the previously committed patch earlier. Hoping this is fine. Thanks :)
Attachment #8961836 - Attachment is obsolete: true
Attachment #8962413 - Flags: review?(jhofmann)
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 :)
Attachment #8962413 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 15

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Should I attach another patch for adding an assertion that this class is set on the right column to the above mentioned file?
Attachment #8962413 - Attachment is obsolete: true
Attachment #8963512 - Flags: review?(jhofmann)
(Assignee)

Comment 16

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Attachment #8963512 - Attachment is obsolete: true
Attachment #8963512 - Flags: review?(jhofmann)
Attachment #8963513 - Flags: review?(jhofmann)
(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"));
Attachment #8963513 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 19

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Attachment #8963513 - Attachment is obsolete: true
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 :)
(Assignee)

Comment 21

a year ago
Posted patch bug1442303.patch (obsolete) — Splinter Review
Attachment #8963540 - Attachment is obsolete: true
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...
Attachment #8963888 - Flags: review+
Flags: needinfo?(guptatrisha97)
(Assignee)

Updated

a year ago
Flags: needinfo?(guptatrisha97)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
The patch failed to apply, please take a look.
Flags: needinfo?(guptatrisha97)
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Flags: needinfo?(guptatrisha97)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

11 months ago
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 :)
Flags: needinfo?(jhofmann)
Attachment #8963888 - Attachment is obsolete: true
Flags: needinfo?(jhofmann)

Comment 26

11 months ago
mozreview-review
Comment on attachment 8980627 [details]
Bug 1442303 - The arrow for sort is missing in Exceptions dialogues

https://reviewboard.mozilla.org/r/246778/#review253264
Attachment #8980627 - Flags: review?(jhofmann) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 27

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

Comment 28

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cef2c764f717
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
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
QA Whiteboard: [bugday-20180620]
QA Whiteboard: [bugday-20180620] → [bugday-20180620] [good first verify]
You need to log in before you can comment on or make changes to this bug.