Issue 6510 - Configure notification opens options page incorrectly

js/desktop-options.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 1335 matching lines @@
 
 function getSubscriptionFilters(subscription)
 {
   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);
       return;
     }
@@ skipping 73 matching lines @@
     case "app.respond":
       switch (message.action)
       {
         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);
       break;
     case "subscriptions.respond":
@@ skipping 17 matching lines @@
            "ui_warn_tracking"]
 });
 port.postMessage({
   type: "subscriptions.listen",
   filter: ["added", "disabled", "homepage", "lastDownload", "removed",
            "title", "downloadStatus", "downloading"]
 });
 
 window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
 window.addEventListener("hashchange", onHashChange, false);