Issue 6510 - Configure notification opens options page incorrectly

js/desktop-options.js

Index: js/desktop-options.js
===================================================================
--- a/js/desktop-options.js
+++ b/js/desktop-options.js
@@ -1348,21 +1348,26 @@
 {
   return browser.runtime.sendMessage({
     type: "filters.get",
     subscriptionUrl: subscription.url});
 }
 
 function hidePref(key, value)
 {
-  let element = document.querySelector("[data-pref='" + key + "']");
+  let element = getPrefElement(key);

[a.giammarchi, 2018/03/22 18:43:28]

let is OK, but why not `const` here ?

[saroyanm, 2018/03/22 20:43:21]

Done.

   if (element)
     element.setAttribute("aria-hidden", value);
 }
 
+function getPrefElement(key)
+{
+  return document.querySelector("[data-pref='" + key + "']");

[a.giammarchi, 2018/03/22 18:43:28]

out of curiosity: are keys always sanitized or some key might contain a double
quote char, as example?

[saroyanm, 2018/03/22 20:43:21]

They can contain, I don't think we are  sanitizing keys.

+}
+
 function getPref(key, callback)
 {
   let checkPref = getPref.checks[key] || getPref.checkNone;
   checkPref((isActive) =>
   {
     if (!isActive)
     {
       hidePref(key, !isActive);
@@ -1447,17 +1452,26 @@
         case "addSubscription":
           let subscription = message.args[0];
           let dialog = E("dialog-content-predefined");
           dialog.querySelector("h3").textContent = subscription.title || "";
           dialog.querySelector(".url").textContent = subscription.url;
           openDialog("predefined");
           break;
         case "focusSection":

[saroyanm, 2018/03/22 16:15:53]

We used to set the tab view to the notifications ->
https://hg.adblockplus.org/adblockplusui/file/tip/js/desktop-options.js#l1454
The actual argument is sent from here ->
https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/notificationHelper....

-          document.body.setAttribute("data-tab", message.args[0]);
+          let section = message.args[0];
+          if (section == "notifications")
+          {
+            section = "advanced";
+            let elem = getPrefElement("notifications_ignoredcategories");
+            elem.classList.add("highlight-animate");

[a.giammarchi, 2018/03/22 18:43:28]

so, in hidePref you check if elem is there before using it. Is it needed to be
checked in here too?

[saroyanm, 2018/03/22 20:43:21]

Not necessarily, because "notifications_ignoredcategories" is hardcoded in
desktop-options.html, we know that it always exist.

+            elem.querySelector("button").focus();
+          }
+
+          selectTabItem(section, document.body, false);
           break;
       }
       break;
     case "filters.respond":
       onFilterMessage(message.action, message.args[0]);
       break;
     case "prefs.respond":
       onPrefMessage(message.action, message.args[0], false);