Issue 5593 - Wait for prefs before updating popup UI

popup.js

Index: popup.js
===================================================================
--- a/popup.js
+++ b/popup.js
@@ -75,27 +75,30 @@
   document.getElementById("clickhide-cancel").addEventListener(
     "click", cancelClickHide, false
   );
   document.getElementById("options").addEventListener("click", () =>
   {
     ext.showOptions();
   }, false);
 
-  // Set up collapsing of menu items
-  for (let collapser of document.getElementsByClassName("collapse"))
+  Prefs.untilLoaded.then(() =>

[Manish Jethani, 2017/08/31 15:56:57]

Wait for the prefs to load.

   {
-    collapser.addEventListener("click", toggleCollapse, false);
-    if (!Prefs[collapser.dataset.option])
+    // Set up collapsing of menu items
+    for (let collapser of document.getElementsByClassName("collapse"))
     {
-      document.getElementById(
-        collapser.dataset.collapsable
-      ).classList.add("collapsed");
+      collapser.addEventListener("click", toggleCollapse, false);
+      if (Prefs[collapser.dataset.option])

[Manish Jethani, 2017/08/31 15:56:57]

Since it's now collapsed by default, uncollapse it if the option is true.

+      {
+        document.getElementById(
+          collapser.dataset.collapsable
+        ).classList.remove("collapsed");
+      }

[Thomas Greiner, 2017/08/31 16:39:08]

FYI: IIRC we've avoided using `Element.classList.toggle(string, boolean)` until
now due to cross-browser compatibility issues but according to
https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_co...
it seems like all browsers we need to support are covered. So if you want you
can use it instead of `Element.classList.remove()` here.

     }
-  }
+  });
 }
 
 function toggleEnabled()
 {
   let disabled = document.body.classList.toggle("disabled");
   if (disabled)
   {
     let host = getDecodedHostname(page.url).replace(/^www\./, "");
@@ -140,13 +143,13 @@
   }
   document.body.classList.remove("clickhide-active");
   page.sendMessage({type: "composer.content.finished"});
 }
 
 function toggleCollapse(event)
 {
   let collapser = event.currentTarget;
-  Prefs[collapser.dataset.option] = !Prefs[collapser.dataset.option];
-  collapser.parentNode.classList.toggle("collapsed");
+  let collapsable = document.getElementById(collapser.dataset.collapsable);

[Thomas Greiner, 2017/08/31 16:39:08]

Typo: Replace "collapsable" with "collapsible"

+  Prefs[collapser.dataset.option] = !collapsable.classList.toggle("collapsed");

[Manish Jethani, 2017/08/31 15:56:57]

The pref value should follow the UI so they never go out of sync. The toggle
function will return a boolean value indicating whether it's collapsed, simply
set the "show" value to the opposite.

[Thomas Greiner, 2017/08/31 16:39:08]

Usually we do it the other way around to ensure that the UI always reflects the
current state of the extension. This is helpful, for instance, to avoid
inconsistency if an error occurs during a state change.

For that we use `Prefs.on()` to keep the UI up-to-date irregardless of where the
preference was changed. The messaging API equivalent of that is "prefs.listen"
which is what we've already been using in all other UIs.

Feel free to change that later on though if there are reasons that neither
`Prefs.on()` nor "prefs.listen" are feasible at the moment.

 }
 
 document.addEventListener("DOMContentLoaded", onLoad, false);