Make new options page compatible with edge

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 515 matching lines @@
                                             type + "_title");
           }
 
           addSubscription(subscription);
         }
       });
   }
 
   function findParentData(element, dataName, returnElement)
   {
     element = element.closest(`[data-${dataName}]`);

[Thomas Greiner, 2018/01/18 16:20:46]

Nice, I wasn't even aware of this method. :)

[saroyanm, 2018/02/08 14:59:09]

Detail: We are usually careful when it comes to experimental technologies.

Make sense to find out which Edge versions are we supporting.
This feature is only supported in edge 15 and up.

Ollie, can you please let us know which versions of Edge are we supporting ?

Probably make sense to update that info in as well:
https://adblockplus.org/en/requirements

[a.giammarchi, 2018/02/08 15:07:21]

FWIW closest is not really experimental, it has been around for years, but in
our edge metadata we target 40.15063.0.0 which is >= 15 so I hope there's really
nothing to worry about.

[saroyanm, 2018/02/08 15:20:18]

According to MDN it is ->
https://developer.mozilla.org/en-US/docs/Web/API/Element/closest 
But might be their page needs an update.
Noted.
Anyway I've created a ticket ->
https://gitlab.com/eyeo/web.adblockplus.org/issues/17

[a.giammarchi, 2018/02/08 15:27:29]

don't trust everything you read in MDN ... it's updated by devs that very often
forget to update.

closest is in the living standard since years now, it's been polyfilled 3+ years
ago -> https://github.com/WebReflection/dom4 and IE15 was last one to implement
it.

It was a good catch/safety call but also both me and I guess Thomas too checked
before shipping it.

I'd like to share this little CLI that is configured with our target browsers
and tell instantly if we can support this or that. Let's follow up in irc.

[saroyanm, 2018/02/08 15:38:30]

Thanks for the links and info.
Acknowledged.

-    if (element)

[Thomas Greiner, 2018/01/18 16:20:46]

Detail: We can save one level of indentation by returning `null` here already if
we couldn't find an element.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

-    {
-      if (returnElement) return element;

[Thomas Greiner, 2018/01/18 16:20:46]

Detail: We tend to avoid single-line if-statements to make it clear that, for
instance in this case, we're exiting the function.

However, to be fair I couldn't find it mentioned anywhere in our coding style.
So we may want to think about extending it since this is the way it's written
across all of our coide, as far as I'm aware of.

[a.giammarchi, 2018/01/19 11:16:48]

to be honest, there's no way I am going to remember all the rules so I have a
hook in VS Code that formats everything accordingly to the eyeo code style /
linting rules, as documented.

Do you know if there's a way to enforce this rule? Apparently ESLint does not
provide an option for this but I'd love to never care about coding style and
also, and of course, I want to preserve consistency.

If there's no way to enforce it, I might try to update the documentation
specifying that inline if statements are better off our code base.

Please let me know how to improve here, thanks.

[Thomas Greiner, 2018/01/19 13:55:16]

Our coding style wasn't written with compatibility with linters in mind so some
rules may not be enforceable in an automatic fashion. For instance, we require
usage of hexadecimal notation of CSS colors where possible since that depends on
what colors we want to define and what platforms we support.

We could investigate whether linters are willing to implement this in which case
we could provide this contribution to them. Otherwise we could either modify our
coding style or are left with keep doing it manually in which case we simply
have to accept that we'll never be able to catch all of the violations.

[a.giammarchi, 2018/01/19 14:16:28]

it's good you mentioned that ... 'cause I need a CSS linter too now :-)

However, since coding styles are always very subjective, and in my experience in
most cases a very pointless waste of everybody time when discussed, I'd love
instead to automate all rules and keep consistency.

Accordingly, I'll rather improve my automatic-fixing little snippet and maybe
share it with others at some point so that consistency and code style will be a
no-brainer for everyone, new comer or old.

Will keep this `return` rul in mind until I've done that.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

[a.giammarchi, 2018/01/19 17:02:28]

Done.

[Thomas Greiner, 2018/01/22 13:06:38]

That's a very noble approach. I'd be curious to hear what other devs in the
company think about that.
I'm not too convinced of auto-fixing code personally but detecting all possible
violations would be great. Anything we can do to make development more
attractive to both internal and external people sounds like a good time
investment.

-      return element.getAttribute(`data-${dataName}`);
-    }
-    return null;
+    if (!element)
+      return null;
+    if (returnElement)
+      return element;
+    return element.getAttribute(`data-${dataName}`);
   }
 
   function sendMessageHandleErrors(message, onSuccess)
   {
     browser.runtime.sendMessage(message, (errors) =>
     {
       if (errors.length > 0)
         alert(errors.join("\n"));
       else if (onSuccess)
         onSuccess();
@@ skipping 522 matching lines @@
     {
       acceptableAds.setAttribute("aria-checked", true);
       acceptableAdsPrivacy.setAttribute("aria-disabled", false);
     }
     else if (acceptableAdsPrivacyUrl in subscriptionsMap)
     {
       acceptableAds.setAttribute("aria-checked", true);
       acceptableAdsPrivacy.setAttribute("aria-checked", true);
       acceptableAdsPrivacy.setAttribute("aria-disabled", false);
 
       if (navigator.doNotTrack != 1)

[saroyanm, 2018/02/08 14:59:09]

Edge uses window.doNotTrack:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack#Browser...

         acceptableAdsForm.classList.add("show-dnt-notification");
     }
     else
     {
       // Using aria-disabled in order to keep the focus
       acceptableAdsPrivacy.setAttribute("aria-disabled", true);
       acceptableAdsPrivacy.setAttribute("tabindex", -1);
     }
   }
 
@@ skipping 368 matching lines @@
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }