Issue 5593 - Wait for prefs before updating popup UI

popup.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 62 matching lines @@
     "click", activateClickHide, false
   );
   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\./, "");
     let filter = Filter.fromText("@@||" + host + "^$document");
     if (filter.subscriptions.length && filter.disabled)
@@ skipping 34 matching lines @@
     window.clearTimeout(activateClickHide.timeout);
     activateClickHide.timeout = null;
   }
   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);