Closed Bug 736572 Opened 9 years ago Closed 6 years ago

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

Categories

(Firefox :: Page Info Window, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: eyaler, Assigned: mail, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 3 obsolete files)

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
Attached patch v1 (obsolete) — Splinter Review
Attachment #641378 - Flags: review?(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.
Attachment #641378 - Flags: review?(philip.chee)
Attachment #641378 - Flags: review?(dao)
Attachment #641378 - Flags: feedback+
Attached patch v2 (obsolete) — Splinter Review
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+
> 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.
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+
Attachment #641517 - Attachment is obsolete: true
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+
(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
Attachment #8571578 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e0abc8b99d18
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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.