Bug 1612055 - Restore default order of thread pane columns after bug 1607575. r=mkmelin a=wsmwk
authorAlessandro Castellani <alessandro@thunderbird.net>
Tue, 09 Jun 2020 13:46:09 +0300
changeset 39360 2240fa33634c08d4980a7db4feca4b42ea7016d8
parent 39359 ff65eea9ddd548ee9737bfd1eb4f3398f659cd7d
child 39361 0525eca6f121e05cb4df0a5f7e0786188c0c1334
push id402
push userclokep@gmail.com
push dateMon, 29 Jun 2020 20:48:04 +0000
reviewersmkmelin, wsmwk
bugs1612055, 1607575
Bug 1612055 - Restore default order of thread pane columns after bug 1607575. r=mkmelin a=wsmwk
calendar/base/content/calendar-task-tree.js
mail/base/content/folderDisplay.js
mail/base/content/threadTree.inc.xhtml
--- a/calendar/base/content/calendar-task-tree.js
+++ b/calendar/base/content/calendar-task-tree.js
@@ -329,17 +329,17 @@
      * This function reads data from column attributes and sets it on several attributes on the
      * task tree element, which are persisted because they are in the "persist" attribute of
      * the task tree element.
      * (E.g. `persist="visible-columns ordinals widths sort-active sort-direction"`.)
      */
     persistColumnState() {
       const columns = Array.from(this.querySelectorAll("treecol"));
       const widths = columns.map(col => col.width || 0);
-      const ordinals = columns.map(col => col.getAttribute("ordinal"));
+      const ordinals = columns.map(col => col.ordinal);
       const visibleColumns = columns
         .filter(col => col.getAttribute("hidden") != "true")
         .map(col => col.getAttribute("itemproperty"));
 
       this.setAttribute("widths", widths.join(" "));
       this.setAttribute("ordinals", ordinals.join(" "));
       this.setAttribute("visible-columns", visibleColumns.join(" "));
 
@@ -368,17 +368,17 @@
       this.querySelectorAll("treecol").forEach(col => {
         const itemProperty = col.getAttribute("itemproperty");
         if (visibleColumns.includes(itemProperty)) {
           col.removeAttribute("hidden");
         } else {
           col.setAttribute("hidden", "true");
         }
         if (ordinals && ordinals.length > 0) {
-          col.setAttribute("ordinal", ordinals.shift());
+          col.ordinal = ordinals.shift();
         }
         if (widths && widths.length > 0) {
           col.width = Number(widths.shift());
         }
         if (sorted && sorted == itemProperty) {
           this.mTreeView.sortDirection = sortDirection;
           this.mTreeView.selectedColumn = col;
         }
--- a/mail/base/content/folderDisplay.js
+++ b/mail/base/content/folderDisplay.js
@@ -674,21 +674,18 @@ FolderDisplayWidget.prototype = {
           // if it doesn't have preserved state it should be hidden
           let shouldBeHidden = true;
           // restore state
           if (colChild.id in aColumnStates) {
             let colState = aColumnStates[colChild.id];
             if ("visible" in colState) {
               shouldBeHidden = !colState.visible;
             }
-            if (
-              "ordinal" in colState &&
-              colChild.getAttribute("ordinal") != colState.ordinal
-            ) {
-              colChild.setAttribute("ordinal", colState.ordinal);
+            if ("ordinal" in colState && colChild.ordinal != colState.ordinal) {
+              colChild.ordinal = colState.ordinal;
             }
           }
           let isHidden = colChild.getAttribute("hidden") == "true";
           if (isHidden != shouldBeHidden) {
             if (shouldBeHidden) {
               colChild.setAttribute("hidden", "true");
             } else {
               colChild.removeAttribute("hidden");
@@ -732,49 +729,49 @@ FolderDisplayWidget.prototype = {
     let colChildren = cols.children;
     for (let iKid = 0; iKid < colChildren.length; iKid++) {
       let colChild = colChildren[iKid];
       if (colChild.tagName != "treecol") {
         continue;
       }
       columnStates[colChild.id] = {
         visible: colChild.getAttribute("hidden") != "true",
-        ordinal: colChild.getAttribute("ordinal"),
+        ordinal: colChild.ordinal,
       };
     }
 
     return columnStates;
   },
 
   /**
    * For now, just save the visible columns into a dictionary for use in a
    *  subsequent call to |setColumnStates|.
    */
   _saveColumnStates() {
-    // In the actual nsITreeColumn, the index property indicates the column
-    //  number.  This column number is a 0-based index with no gaps; it only
-    //  increments the number each time it sees a column.
+    // In the actual TreeColumn, the index property indicates the column
+    // number. This column number is a 0-based index with no gaps; it only
+    // increments the number each time it sees a column.
     // However, this is subservient to the 'ordinal' property which
-    //  defines the _apparent content sequence_ provided by GetNextSibling.
-    //  The underlying content ordering is still the same, which is how
-    //  restoreNaturalOrder can reset things to their XUL definition sequence.
-    //  The 'ordinal' stuff works because nsBoxFrame::RelayoutChildAtOrdinal
-    //  messes with the sibling relationship.
-    // Ordinals are 1-based.  restoreNaturalOrder apparently is dumb and does
-    //  not know this, although the ordering is relative so it doesn't actually
-    //  matter.  The annoying splitters do have ordinals, and live between
-    //  tree columns.  The splitters adjacent to a tree column do not need to
-    //  have any 'ordinal' relationship, although it would appear user activity
-    //  tends to move them around in a predictable fashion with oddness involved
-    //  at the edges.
+    // defines the _apparent content sequence_ provided by GetNextSibling.
+    // The underlying content ordering is still the same, which is how
+    // _ensureColumnOrder() can reset things to their XUL definition sequence.
+    // The 'ordinal' stuff works because nsBoxFrame::RelayoutChildAtOrdinal
+    // messes with the sibling relationship.
+    // Ordinals are 1-based. _ensureColumnOrder() apparently is dumb and does
+    // not know this, although the ordering is relative so it doesn't actually
+    // matter. The annoying splitters do have ordinals, and live between
+    // tree columns. The splitters adjacent to a tree column do not need to
+    // have any 'ordinal' relationship, although it would appear user activity
+    // tends to move them around in a predictable fashion with oddness involved
+    // at the edges.
     // Changes to the ordinal attribute should take immediate effect in terms of
-    //  sibling relationship, but will merely invalidate the columns rather than
-    //  cause a re-computation of column relationships every time.
-    // restoreNaturalOrder invalidates the tree when it is done re-ordering; I'm
-    //  not sure that's entirely necessary...
+    // sibling relationship, but will merely invalidate the columns rather than
+    // cause a re-computation of column relationships every time.
+    // _ensureColumnOrder() invalidates the tree when it is done re-ordering;
+    // I'm not sure that's entirely necessary...
     this._savedColumnStates = this.getColumnStates();
   },
 
   /**
    * Restores the visible columns saved by |_saveColumnStates|.
    */
   _restoreColumnStates() {
     if (this._savedColumnStates) {
--- a/mail/base/content/threadTree.inc.xhtml
+++ b/mail/base/content/threadTree.inc.xhtml
@@ -18,16 +18,26 @@
                           onkeydown="ThreadPaneKeyDown(event);"
                           onselect="ThreadPaneSelectionChanged();">
 #ifdef MAIN_WINDOW
                       <treecols is="thread-pane-treecols" id="threadCols"
 #else
                       <treecols id="threadCols"
 #endif
                                 pickertooltiptext="&columnChooser2.tooltip;">
+
+                         <!--
+                           The below code may suggest that 'ordinal' is still a supported XUL
+                           XUL attribute. It is not. This is a crutch so that we can
+                           continue persisting the CSS -moz-box-ordinal-group attribute,
+                           which is the appropriate replacement for the ordinal attribute
+                           but cannot yet be easily persisted. The code that synchronizes
+                           the attribute with the CSS lives in
+                           toolkit/content/widget/tree.js and is specific to tree elements.
+                         -->
                         <treecol is="treecol-image" id="selectCol"
                                  class="treecol-image selectColumnHeader"
                                  persist="hidden ordinal"
                                  fixed="true"
                                  cycler="true"
                                  currentView="unthreaded"
                                  hidden="true"
                                  closemenu="none"