1323391 - Sort sites listed in Settings of Site Data

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Fischer [:Fischer]]

Should be able to sort sites listed in Settings of Site Data by usage

Comment 1

[Fischer [:Fischer]]

Attachment: Bug 1323391 - Sort sites listed in Settings of Site Data,

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

Comment 2

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #1)
> Created attachment 8820119 [details]
> Bug 1323391 - Sort sites listed in Settings of Site Data by usage,
> 
> Review commit: https://reviewboard.mozilla.org/r/99640/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/99640/

Hi Gijs,

This patch adds the feature of sorting sites listed in the Settings subdialog by the storage usage column [1].

One test case is added as well.
Here is the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=877d93d99e4d18983f780dd2233738197a62ff67

[1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637924

Comment 3

[:Gijs (he/him)]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review100192

I noted some trivial issues below. But more generally, we should suport sorting on all columns, not just the size one, and I am not convinced that sorting on size is the right default. Alphabetical by hostname seems like it would be easier for users to understand. Either way, we should implement sort for all of these. Perhaps you can reuse/abstract some of the sorting code as used in onPermissionsSort in permissions.js.

::: browser/components/preferences/siteDataSettings.js:81
(Diff revision 1)
> +      this._sortSites(this._sites, "decending");
> +      usageCol.setAttribute("sortDirection", "decending");

Same spelling mistakes here.

::: browser/components/preferences/siteDataSettings.xul:34
(Diff revision 1)
>      <richlistbox id="sitesList" orient="vertical" flex="1">
>        <listheader>
>          <treecol flex="4" width="50" label="&hostCol.label;"/>
>          <treecol flex="2" width="50" label="&statusCol.label;"/>
> -        <treecol flex="1" width="50" label="&usageCol.label;"/>
> +        <treecol flex="1" width="50" label="&usageCol.label;" accesskey="&usageCol.accesskey;"
> +                 id="usageCol" sortDirection="decending"/>

"descending".

Also, I don't think you want an access key here. Or, if you do, we should have one for each column.

I'm also not convinced that *defaulting* the dialog to sort by usage is the right decision.

Comment 4

[Fischer [:Fischer]]

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8820119 [details]
> Bug 1323391 - Sort sites listed in Settings of Site Data by usage,
> 
> https://reviewboard.mozilla.org/r/99640/#review100192
> 
> I noted some trivial issues below. But more generally, we should suport
> sorting on all columns, not just the size one, and I am not convinced that
> sorting on size is the right default. Alphabetical by hostname seems like it
> would be easier for users to understand. Either way, we should implement
> sort for all of these. Perhaps you can reuse/abstract some of the sorting
> code as used in onPermissionsSort in permissions.js.
> 
Check with the UX team. Will follow other tables, like cookies table, to sort on all columns and default by hostname. The specs will be updated too.

> ::: browser/components/preferences/siteDataSettings.js:81
> (Diff revision 1)
> > +      this._sortSites(this._sites, "decending");
> > +      usageCol.setAttribute("sortDirection", "decending");
> 
> Same spelling mistakes here. 
Sorry for this typo and thanks for spotting it.
Just found why the UI works correctly and I didn't find it in the first place is because the CSS rule[1] only checks "ascending":
```
... > xul|*.treecol-sortdirection[sortDirection] {
  ...
}

... > xul|*.treecol-sortdirection[sortDirection="ascending"] {
  ...
}
```

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#789

> ::: browser/components/preferences/siteDataSettings.xul:34
> (Diff revision 1)
> > +        <treecol flex="1" width="50" label="&usageCol.label;" accesskey="&usageCol.accesskey;"
> > +                 id="usageCol" sortDirection="decending"/>
> 
> "descending".
> 
> Also, I don't think you want an access key here. Or, if you do, we should
> have one for each column.
> 
OK, will update.

Comment 5

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #0)
> Should be able to sort sites listed in Settings of Site Data by usage
Update the description:
---------------------------------------------------------------
Should be able to sort sites listed in Settings of Site Data by
  - host (default in ascending order)
  - permission status
  - usage

Comment 6

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/1-2/

Comment 7

[Fischer [:Fischer]]

Hi Gijs,

Update patch to enable to sort sites by host(default), permission status, or usage.
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4653ff96c25b920e61db4129ee7fc560b51b872

Thanks

Comment 8

[:Gijs (he/him)]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102446

This is basically ready besides the issues below. I would have given r+ except I really don't follow the data-last-sortDirection thing. Can you explain / fix? :-)

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:294
(Diff revision 2)
> +    let actual = siteItems[i].getAttribute("data-origin");
> +    is(actual, expected, "Should reverse sites to the descending order by usage");
> +  }
> +
> +  mockSiteDataManager.unregister();
> +  gBrowser.removeCurrentTab();

Nit: yield BTU.removeTab(gBrowser.selectedTab);

::: browser/components/preferences/siteDataSettings.js:36
(Diff revision 2)
> +    }
> +
>      this._list = document.getElementById("sitesList");
>      SiteDataManager.getSites().then(sites => {
>        this._sites = sites;
> -      this._sortSites(this._sites, "decending");
> +      let sortCol = this._list.querySelector("#hostCol");

Nit: document.getElementById("hostCol");

Note that this would cause the sort to always reset when the dialog is reopened. Normally speaking, we persist the sort direction (using the XUL persist mechanism) and reuse it once the dialog is reopened. Is there a particular reason not do this here?

::: browser/components/preferences/siteDataSettings.js:52
(Diff revision 2)
>     * @param sites {Array}
> -   * @param order {String} indicate to sort in the "decending" or "ascending" order
> +   * @param col {XULElement} the <treecol> being sorted on
>     */
> -  _sortSites(sites, order) {
> -    sites.sort((a, b) => {
> -      if (order === "ascending") {
> +  _sortSites(sites, col) {
> +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";

Why do we need the extra `data-last-sortDirection` attribute? Why isn't just the `sortDirection` attribute enough?

::: browser/components/preferences/siteDataSettings.js:76
(Diff revision 2)
> +    sites.sort(sortFunc);
> +    if (sortDirection === "descending") {
> +      sites.reverse();
> -      }
> +    }

Instead of reversing, make it part of the sort:

```js
if (sortDirection == "descending") {
  sites.sort((a, b) => sortFunc(b, a));
} else {
  sites.sort(sortFunc);
}
```

Comment 9

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/2-3/

Comment 10

[Fischer [:Fischer]]

(In reply to :Gijs from comment #8)
> :::
> browser/components/preferences/in-content/tests/browser_advanced_siteData.js:
> > +  gBrowser.removeCurrentTab();
> 
> Nit: yield BTU.removeTab(gBrowser.selectedTab);
Updated, thanks.

> ::: browser/components/preferences/siteDataSettings.js:36
> > +      let sortCol = this._list.querySelector("#hostCol");
> 
> Nit: document.getElementById("hostCol");
Updated, thanks.

> Note that this would cause the sort to always reset when the dialog is
> reopened. Normally speaking, we persist the sort direction (using the XUL
> persist mechanism) and reuse it once the dialog is reopened. Is there a
> particular reason not do this here?
The cookies list in Privacy and the saved logins list in Security all reset to sort on the host column on loading. This is to be consistent with them.

> ::: browser/components/preferences/siteDataSettings.js:52
> > -  _sortSites(sites, order) {
> > -    sites.sort((a, b) => {
> > -      if (order === "ascending") {
> > +  _sortSites(sites, col) {
> > +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> > +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";
> 
> Why do we need the extra `data-last-sortDirection` attribute? Why isn't just
> the `sortDirection` attribute enough?
This is to remember each column's sort direction. Fox example,
1. User sorts on usage in descending order to spot the most usage used.
2. User switches to status column to take a look
3. User switches back to usage column and can spot the most usage used instantly (because sort direction is remembered).

> ::: browser/components/preferences/siteDataSettings.js:76
> (Diff revision 2)
> > +    sites.sort(sortFunc);
> > +    if (sortDirection === "descending") {
> > +      sites.reverse();
> > -      }
> > +    }
> 
> Instead of reversing, make it part of the sort:
> 
> ```js
> if (sortDirection == "descending") {
>   sites.sort((a, b) => sortFunc(b, a));
> } else {
>   sites.sort(sortFunc);
> }
> ```
Updated, thanks.

Comment 11

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #10)
> > ::: browser/components/preferences/siteDataSettings.js:52
> > > -  _sortSites(sites, order) {
> > > -    sites.sort((a, b) => {
> > > -      if (order === "ascending") {
> > > +  _sortSites(sites, col) {
> > > +    let isCurrentSortCol = col.getAttribute("data-isCurrentSortCol")
> > > +    let sortDirection = col.getAttribute("data-last-sortDirection") || "ascending";
> > 
> > Why do we need the extra `data-last-sortDirection` attribute? Why isn't just
> > the `sortDirection` attribute enough?
> This is to remember each column's sort direction. Fox example,
> 1. User sorts on usage in descending order to spot the most usage used.
> 2. User switches to status column to take a look
> 3. User switches back to usage column and can spot the most usage used
> instantly (because sort direction is remembered).

OK, but then, can't we simply not remove the sortDirection attribute all the time?

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs from comment #11)
> OK, but then, can't we simply not remove the sortDirection attribute all the
> time?

... or does that have other, undesirable, side-effects, like maybe styling changes or something?

Comment 13

[Fischer [:Fischer]]

(In reply to :Gijs from comment #12)
> (In reply to :Gijs from comment #11)
> > OK, but then, can't we simply not remove the sortDirection attribute all the
> > time?
> 
> ... or does that have other, undesirable, side-effects, like maybe styling
> changes or something?
Yes, you are right. The little "v" sorting icon would appear based on the sortDirection attribute.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102816

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:235
(Diff revisions 2 - 3)
>      is(actual, expected, "Should sort sites in the ascending order by host");
>    }
>  
>    // Test reversing to the descending order by host
>    hostCol.click();
> -  mockSites.reverse();
> +  mockSites.sort((a, b) => sortByHost(b ,a));

I actually thought we could leave it as .reverse() in the test, as you already have a sorted array, so you don't need to do the sort again... does the test pass if you use .reverse()? If so, I guess I don't really mind either way... 

I ask because keeping the implementation of the test in sync with the implementation of the feature is odd in cases like this, because it shouldn't make any difference to the test how the sorting is implemented - it should only verify that it works. :-)

Comment 15

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #13)
> (In reply to :Gijs from comment #12)
> > (In reply to :Gijs from comment #11)
> > > OK, but then, can't we simply not remove the sortDirection attribute all the
> > > time?
> > 
> > ... or does that have other, undesirable, side-effects, like maybe styling
> > changes or something?
> Yes, you are right. The little "v" sorting icon would appear based on the
> sortDirection attribute.

Ah, OK then. Let's leave it like this.

Comment 16

[Fischer [:Fischer]]

(In reply to :Gijs from comment #14)
> > -  mockSites.reverse();
> > +  mockSites.sort((a, b) => sortByHost(b ,a));
> 
> I actually thought we could leave it as .reverse() in the test, as you
> already have a sorted array, so you don't need to do the sort again... does
> the test pass if you use .reverse()? If so, I guess I don't really mind
> either way... 
> 
> I ask because keeping the implementation of the test in sync with the
> implementation of the feature is odd in cases like this, because it
> shouldn't make any difference to the test how the sorting is implemented -
> it should only verify that it works. :-)
> 
When, for example, a.usage equals to b.usage, then the order of a and b by sorting ascending then ".reverse()" would be different from by sorting descending. On the UI, the both results fulfill the demand of sorting on some column, however, there could be a not-main-focus difference for the above example.

Maybe, we could define more details on the sorting rules for this kind of case. But I am just not sure if this would make things too complex since for the most daily usage simply sorting on one column as commanded is enough. Like just want to find and clear sites using the most usage.

Comment 17

[:Gijs (he/him)]

(In reply to Fischer [:Fischer] from comment #16)
> Maybe, we could define more details on the sorting rules for this kind of
> case. But I am just not sure if this would make things too complex since for
> the most daily usage simply sorting on one column as commanded is enough.
> Like just want to find and clear sites using the most usage.

Then the test isn't verifying what it wants to verify - it's too strict. It should just check that after being sorted, the items in the DOM list are sorted correctly (ie every item is greater-or-equal (or smaller-or-equal) than the next). Then the test will pass independently from how the sorting algorithm is implemented.

Comment 18

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/3-4/

Comment 19

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Hi, Updated the test to make it independent from how the sorting algorithm is implemented, thanks.

Comment 20

[:Gijs (he/him)]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

https://reviewboard.mozilla.org/r/99640/#review102886

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:37
(Diff revisions 3 - 4)
>    }
>  };
>  
>  const mockSiteDataManager = {
> -  sites: [
> +  sites: new Map([
> +    [ 

Nit: trailing whitespace here and below.

Comment 21

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/4-5/

Comment 22

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/5-6/

Comment 23

[Fischer [:Fischer]]

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a136209754dce7450b3deeaed7f7dd7779d4b533

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a4988f51726
Sort sites listed in Settings of Site Data, r=Gijs

Comment 25

[Iris Hsiao [:ihsiao]]

sorry had to back out this for eslint failure, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=66754872&repo=autoland&lineNumber=8459

Comment 26

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d31a5d94ce9
Backed out changeset 2a4988f51726 for eslint failure

Comment 27

[Fischer [:Fischer]]

Comment on attachment 8820119 [details]
Bug 1323391 - Sort sites listed in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99640/diff/6-7/

Comment 28

[Fischer [:Fischer]]

(In reply to Iris Hsiao [:ihsiao] from comment #25)
> sorry had to back out this for eslint failure, i.e.,
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=66754872&repo=autoland&lineNumber=8459
It looks like the eslint rules updated on the latest central. Just fix the eslint issue.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/278daccd7e54
Sort sites listed in Settings of Site Data, r=Gijs

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/278daccd7e54