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 = 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.

+  const element = getPrefElement(key);
   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);
@@ skipping 75 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....

           let section = message.args[0];
           if (section == "notifications")
           {
             section = "advanced";
-            let elem = getPrefElement("notifications_ignoredcategories");
+            const 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;
@@ skipping 21 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);