736572 - pageinfo columns should have arrows showing which column is sorted and sort direction

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[eyal gruss]

with firefox 12.0b1 pageinfo columns are now sortable, but missing the arrow indicators for showing by which column it is sorted and the sort direction (ascending/descending)

Comment 1

[Philip Chee]

You need something like:
http://mxr.mozilla.org/comm-central/source/suite/browser/pageinfo/pageInfo.js?mark=133-139#127
See: http://hg.mozilla.org/comm-central/rev/c36b1ed900b9
for details

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 3

[Philip Chee]

Comment on attachment 641378 [details] [diff] [review]
v1

Thanks for the patch! I'm not a Firefox peer, so forwarding to Dao.

> +  var treecolIndex = treecol.index;
var index = treecol.index;
"treecolIndex" is redundant since this method deals with tree columns.
f=me with this fixed.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 5

[Dão Gottwald [::dao]]

Comment on attachment 641517 [details] [diff] [review]
v2

>+    Array.forEach(tree.columns, function(col) {
>+      col.element.removeAttribute("sortActive");
>+      col.element.removeAttribute("sortDirection");
>+    });

for (let col of tree.columns) {
  col.element.removeAttribute("sortActive");
  col.element.removeAttribute("sortDirection");
}

>+    treecol.element.setAttribute("sortActive", true);

treecol.element.setAttribute("sortActive", "true");

Comment 6

[Philip Chee]

> for (let col of tree.columns) {
Back when I tried this in SeaMonkey it didn't work because tree.columns weren't iterable. I guess this has been fixed?

Comment 7

[Dão Gottwald [::dao]]

If it didn't work earlier, probably not.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

To fix this bug, you can use the patch that was started by Raymond and then address the feedback in comment #6.

Comment 9

[Michael Weisz]

Attachment: Show arrow after sorting column in pageInfo (by applying patch v2). Set metaNode name attribute to empty string when no name is provided.

When applying patch v2 by :raymondlee the arrow indication seemed to be working very well already. Only when sorting by the name column in the meta tag tree view the sorting itself did not work since some of the entries were null (e.g. as described in 782623) and the comparing function crashed. 
So I changed the metaNode entry rows to now contain an empty string when nothing else is set (I hope that is ok). 

I'm not entirely sure I fully understood the issues in #5 and #6 but I think it should be working now.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8571578 [details] [diff] [review]
Show arrow after sorting column in pageInfo (by applying patch v2). Set metaNode name attribute to empty string when no name is provided.

Review of attachment 8571578 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/pageinfo/pageInfo.js
@@ +106,5 @@
> +    Array.forEach(tree.columns, function(col) {
> +      col.element.removeAttribute("sortActive");
> +      col.element.removeAttribute("sortDirection");
> +    });
> +    treecol.element.setAttribute("sortActive", true);

Please pass in "true" here instead of `true`, which is what the second half of comment #5 was asking for.

@@ +216,5 @@
> +  Array.forEach(tree.columns, function(col) {
> +    col.element.removeAttribute("sortActive");
> +    col.element.removeAttribute("sortDirection");
> +  });
> +  treecol.element.setAttribute("sortActive", true);

Ditto.

@@ +529,5 @@
>      var metaTree = document.getElementById("metatree");
>      metaTree.view = gMetaView;
>  
>      for (var i = 0; i < length; i++)
> +      gMetaView.addRow([metaNodes[i].name || metaNodes[i].httpEquiv || metaNodes[i].getAttribute("property") || "",

Instead of passing an empty string here, please change the comparator functions to handle null as an empty string.

For example,
function textComparator(a, b) { return a.toLowerCase().localeCompare(b.toLowerCase()); },
would become:
function textComparator(a, b) { return (a || "").toLowerCase().localeCompare((b || "").toLowerCase()); },

Comment 11

[Michael Weisz]

Attachment: Show arrow after sorting column in pageInfo. Provide empty string for comparison function if no according attribute was set. r=jaws

Thank you for your feedback, Jared. 

I updated my patch accordingly (moved empty string creation to comparison function, set attribute properly) and hope it is now working correctly. 

Best regards
Michael

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(try push includes a patch that is not ready for check-in, but shouldn't affect test results)

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/e0abc8b99d18

Comment 14

[Phil Ringnalda (:philor)]

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