Issue 1325 - Use Shadow DOM if possible when highlighting page elements

include.postload.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 16 matching lines @@
 function supportsShadowRoot(element)
 {
   if (!("createShadowRoot" in element))
     return false;
 
   // There are some elements (e.g. <textarea>), which don't
   // support author created shadow roots and throw an exception.
   var clone = element.cloneNode(false);
   try
   {
-    var root = clone.createShadowRoot();

[Wladimir Palant, 2014/09/22 18:31:09]

Please don't use variables outside the block where they are defined, even if
JavaScript allows it.

[Sebastian Noack, 2014/09/23 11:30:07]

Done, though I don't like that policy. :/

[Wladimir Palant, 2014/09/23 13:29:03]

I think I explained my reasoning before - doing this makes the code less obvious
and more error-prone, developers don't usually assume that a variable declared
inside a block can have side-effects outside it. Besides, it will make switching
to block-scoped variables (something that will hopefully be possible for all our
code at some point) non-trivial.

[Sebastian Noack, 2014/09/23 15:11:39]

Yes, we did. And I think it is pointless to repeat that debate. That is why I
already addressed that comment.
If you come from C or Java this might be unexpected, like a lot of other things
in JavaScript. However, that is how variables work in (almost) all script
languages, including JavaScript, Python, Perl, PHP and Bash. So I think it is
more likely that this is exactly what somebody writing JavaScript assumes.
Just because there will be the "let" statement doesn't mean that we must adopt
it for every single variable declaration in our code. I find function-scoped
variables very useful in some cases like this. It feels hack-ish and stupid, to
add an additional line, to declare a variable in a different scope, just to deny
the concept of function-scoped variables.

[Wladimir Palant, 2014/09/23 15:41:45]

You are wrong about Perl, and in Python and PHP local variables simply aren't
being declared at all (never mind Bash, does it even have local variables?).
Function-scoped variables are one of the least obvious JavaScript features, and
quite a few JavaScript developers don't know about this at all.
Yes, we should - for the reasons outlined above, and to avoid common gotchas
like duplicate variable declarations.

[Sebastian Noack, 2014/09/23 16:37:33]

I don't think so. Run following code if you don't believe:

  sub foo
  {
    if (1)
    {
        my $foo = 42;
    }
    print $foo;
  }
  foo();
You could also say that they're automatically or implicit declared on first
initialization. But the point is that variables in Python and PHP are
function-scoped, too.
Yes, bash has local variables, and they are function-scoped:

  foo ()
  {
    if [ true ]
    then
      local x=42
    fi
    echo $x
  }
  foo
People are often confused with closures and function contexts in JavaScript, but
that is a different topic. But I never heard of somebody who calls himself a
JavaScript coder and has experience in at least one more dynamic language, who
assumed functions to be block-scoped. So I can only guess that this happend to
you at some point, and therefore you consider it a common pitfall. ;)
As outlined above, I don't agree to those reasons.
There is "use strict" and tools like JSLint and JSHint for that purpose.

[Wladimir Palant, 2014/09/23 23:07:39]

Please add "use strict;" at the top.
Still doesn't make it a programming language :)

+    clone.createShadowRoot();
   }
   catch (e)
   {
     return false;
   }
 
   // There are some elements (e.g. <input>), which support
   // author created shadow roots, but ignore insertion points.
-  var child = document.createTextNode('');

[Wladimir Palant, 2014/09/22 18:31:09]

Double quotation marks please (same two lines below).

[Sebastian Noack, 2014/09/23 11:30:07]

Done.

+  var child = document.createTextNode("");
   clone.appendChild(child);
 
-  var shadow = document.createElement('shadow');
-  root.appendChild(shadow);
+  var shadow = document.createElement("shadow");
+  clone.shadowRoot.appendChild(shadow);
 
   return shadow.getDistributedNodes()[0] == child;
 }
 
 function highlightElement(element, shadowColor, backgroundColor)
 {
   unhighlightElement(element);
 
   var originalBoxShadowPriority = element.style.getPropertyPriority("box-shadow");
   var originalBackgroundColorPriority = element.style.getPropertyPriority("background-color");
@@ skipping 36 matching lines @@
     var root = element.createShadowRoot();
     root.appendChild(document.createElement("shadow"));
     root.appendChild(style);
 
     element._unhighlight = function()
     {
       root.removeChild(style);
     };
   };
 
-  if (supportsShadowRoot(element) &&  // Use Shadow DOM if posibble to avoid
-                                      // side effects when the web page updates
-                                      // style while highlighted.

[Wladimir Palant, 2014/09/22 18:31:09]

Please put this comment before the line you are commenting on (and probably
combine it with the comment below) - it's too long to be placed inline.

[Sebastian Noack, 2014/09/23 11:30:07]

Done.

-
-      // However, if the element has important styles we have to replace
-      // the style attribute, and can't override them with shadow DOM.
-      originalBoxShadowPriority       != "important" &&
-      originalBackgroundColorPriority != "important")
+  // Use shadow DOM if posibble to avoid side effects when the
+  // web page updates style while highlighted. However, if the
+  // element has important styles we can't override them with shadow DOM.
+  if (supportsShadowRoot(element) && originalBoxShadowPriority       != "important" &&
+                                     originalBackgroundColorPriority != "important")
     highlightWithShadowDOM();
   else
     highlightWithStyleAttribute();
 }
 
 
 function unhighlightElement(element)
 {
   if ("_unhighlight" in element)
   {
@@ skipping 518 matching lines @@
 
           clickHide_deactivate();
         }
         break;
       default:
         sendResponse({});
         break;
     }
   });
 }