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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: eyaler, Assigned: mail, Mentored)

Tracking

Trunk
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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)
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Created attachment 641378 [details] [diff] [review]
v1
Attachment #641378 - Flags: review?(philip.chee)

Comment 3

6 years ago
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.
Attachment #641378 - Flags: review?(philip.chee)
Attachment #641378 - Flags: review?(dao)
Attachment #641378 - Flags: feedback+
Created attachment 641517 [details] [diff] [review]
v2
Attachment #641378 - Attachment is obsolete: true
Attachment #641378 - Flags: review?(dao)
Attachment #641517 - Flags: review?(dao)
Blocks: 736574
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");
Attachment #641517 - Flags: review?(dao) → review+

Comment 6

6 years ago
> 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?
If it didn't work earlier, probably not.
Status: ASSIGNED → NEW
Assignee: raymond → nobody
Mentor: jaws
Whiteboard: [lang=js]
Assignee: nobody → mail
Status: NEW → ASSIGNED
To fix this bug, you can use the patch that was started by Raymond and then address the feedback in comment #6.
(Assignee)

Comment 9

4 years ago
Created 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.

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.
Attachment #8571578 - Flags: review?(jaws)
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()); },
Attachment #8571578 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 11

4 years ago
Created attachment 8573412 [details] [diff] [review]
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
Attachment #8573412 - Flags: review?(jaws)
Attachment #8573412 - Flags: review?(jaws) → review+
Keywords: checkin-needed
(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
https://hg.mozilla.org/integration/fx-team/rev/e0abc8b99d18
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e0abc8b99d18
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 39
You need to log in before you can comment on or make changes to this bug.