Issue 1751 - Highlight elements by injecting an overlay

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -48,77 +48,29 @@
   return s.replace(/^[\d\-]|[^\w\-\u0080-\uFFFF]/g, escapeChar);
 }
 
-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
-  {
-    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("");
-  clone.appendChild(child);
-
-  var shadow = document.createElement("shadow");
-  clone.shadowRoot.appendChild(shadow);
-
-  return shadow.getDistributedNodes()[0] == child;
-}
-
-function getOriginalStyle(element)
-{
-  if ("_originalStyle" in element)
-    return element._originalStyle;
-
-  return element.getAttribute("style");
-}
-
 function highlightElement(element, shadowColor, backgroundColor)
 {
   unhighlightElement(element);
 
-  var originalBoxShadowPriority = element.style.getPropertyPriority("box-shadow");
-  var originalBackgroundColorPriority = element.style.getPropertyPriority("background-color");
-
-  var boxShadow = "inset 0px 0px 5px " + shadowColor;
-
-  var highlightWithShadowDOM = function()
+  var highlightWithOverlay = function()
   {
-    var style = document.createElement("style");
-    style.textContent = ":host {" +
-      "box-shadow:" + boxShadow + " !important;" +
-      "background-color:" + backgroundColor + " !important;" +
-    "}";
-
-    var root = element.createShadowRoot();
-    root.appendChild(document.createElement("shadow"));
-    root.appendChild(style);
+    highlightElement(overlay, shadowColor, backgroundColor);
+    overlay.style.pointerEvents = "none";
 
     element._unhighlight = function()

[kzar, 2015/01/13 11:04:24]

Could this cause a memory leak as _unhighlight functions are added but never
removed?

[Sebastian Noack, 2015/01/13 13:03:53]

It is actually removed, see unhighlightElement().

     {
-      root.removeChild(style);
+      overlay.parentNode.removeChild(overlay);
     };
   };
 
   var highlightWithStyleAttribute = function()
   {
-    var originalBoxShadow = element.style.getPropertyValue("box-shadow");
-    var originalBackgroundColor = element.style.getPropertyValue("background-color");
+    var originalBoxShadow               = element.style.getPropertyValue   ("box-shadow");

[kzar, 2015/01/13 11:04:24]

Why extra spacing before opening bracket? (Maybe it makes sense aesthetically
when not viewed in rietveld? 

[Sebastian Noack, 2015/01/13 13:03:53]

I find it more readable, when similar code is aligned. But unfortunately, this
is against our coding style guide. So I removed the redundant whitespaces.

+    var originalBoxShadowPriority       = element.style.getPropertyPriority("box-shadow");
+    var originalBackgroundColor         = element.style.getPropertyValue   ("background-color");
+    var originalBackgroundColorPriority = element.style.getPropertyPriority("background-color");
 
-    element._originalStyle = getOriginalStyle(element);
-
-    element.style.setProperty("box-shadow", boxShadow, "important");
+    element.style.setProperty("box-shadow", "inset 0px 0px 5px " + shadowColor, "important");
     element.style.setProperty("background-color", backgroundColor, "important");
 
     element._unhighlight = function()
@@ -139,12 +91,9 @@
     };
   };
 
-  // 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();
+  var overlay;

[kzar, 2015/01/13 11:04:24]

Seems confusing to me that overlay is declared here, below where it's first used
inside the above functions. Could you move this declaration above?

[Sebastian Noack, 2015/01/13 13:03:53]

I moved the declaration into highlightWithOverlay(). Frankly, I didn't like that
construct too. Since this method is only called on hover, the element is always
rendered and addElementOverlay() won't return null.

+  if (!("prisoner" in element) && (overlay = addElementOverlay(element)))

[kzar, 2015/01/13 11:04:24]

Could you add a comment before this if statement explaining why we sometimes
need to highlight elements with a style attribute still?

[Sebastian Noack, 2015/01/13 13:03:53]

We only highlight elements with style attributes if they are already overlays we
have created before. I think this should be more clear now.

+    highlightWithOverlay();
   else
     highlightWithStyleAttribute();
 }
@@ -279,7 +228,7 @@
   // We only generate filters based on the "style" attribute,
   // if this is the only way we can generate a filter, and
   // only if there are at least two CSS properties defined.
-  if (/:.+:/.test(getOriginalStyle(element)))
+  if (/:.+:/.test(element.getAttribute("style")))
     return true;
 
   return false;
@@ -302,15 +251,6 @@
 
 // Adds an overlay to an element, which is probably a Flash object
 function addElementOverlay(elt) {
-  // If this element is enclosed in an object tag, we prefer to block that instead
-  if(!elt)
-    return null;
-
-  // If element doesn't have at least one of class name, ID or URL, give up
-  // because we don't know how to construct a filter rule for it
-  if(!isBlockable(elt))
-    return;
-
   // If the element isn't rendered (since its or one of its ancestor's
   // "display" property is "none"), the overlay wouldn't match the element.
   if (!elt.offsetParent)
@@ -320,8 +260,10 @@
   var overlay = document.createElement('div');
   overlay.prisoner = elt;
   overlay.className = "__adblockplus__overlay";
-  overlay.setAttribute('style', 'opacity:0.4; background-color:#ffffff; display:inline-box; ' + 'width:' + thisStyle.width + '; height:' + thisStyle.height + '; position:absolute; overflow:hidden; -webkit-box-sizing:border-box;');
+  overlay.setAttribute('style', 'opacity:0.4; display:inline-box; position:absolute; overflow:hidden; box-sizing:border-box;');

[kzar, 2015/01/13 11:04:24]

Are these changes relevant to the issue?

[kzar, 2015/01/13 13:26:34]

What about these changes?

[Sebastian Noack, 2015/01/13 13:36:11]

Kinda; it already prior to my changes here, it were a bug that the computed
value of the "height" and "width" CSS properties were used, instead the actual
dimensions as provided by the offsetWidth and offsetHeight. However, since
overlays were only injected for images, <object> elements and in a few other
cases, it usually didn't show any undesired effect. This issue became more
critical now where we always use overlays to highlight elements.
The problem is that you can't rely on the "width" and "height" CSS properties,
since they don't consider border and padding (except "box-sizing: border-box" is
used). Hence the overlay would be too small.

   var pos = getAbsolutePosition(elt);
+  overlay.style.width = elt.offsetWidth + "px";
+  overlay.style.height = elt.offsetHeight + "px";
   overlay.style.left = pos[0] + "px";
   overlay.style.top = pos[1] + "px";
 
@@ -380,10 +322,14 @@
   if (clickHide_activated || clickHideFiltersDialog)
     clickHide_deactivate();
 
-  // Add overlays for elements with URLs so user can easily click them
-  var elts = document.querySelectorAll('object,embed,img,iframe,video,audio,picture');
+  // Add overlays for blockable elements that don't emit mouse events that they can still be selected
+  var elts = document.querySelectorAll('object,embed,iframe');
   for(var i=0; i<elts.length; i++)
-    addElementOverlay(elts[i]);
+  {
+    var element = elts[i];
+    if (isBlockable(element))
+      addElementOverlay(element);
+  }
 
   clickHide_activated = true;
   document.addEventListener("mouseover", clickHide_mouseOver, true);
@@ -553,7 +499,7 @@
   // as last resort, create a filter based on inline styles
   if (clickHideFilters.length == 0)
   {
-    var style = getOriginalStyle(elt);
+    var style = elt.getAttribute("style");
     if (style)
       addSelector(escapeCSS(elt.localName) + '[style=' + quote(style) + ']');
   }
@@ -693,7 +639,7 @@
         if(lastRightClickEvent)
         {
           clickHide_activated = true;
-          currentElement = addElementOverlay(lastRightClickEvent.target);
+          currentElement = lastRightClickEvent.target;
           clickHide_mouseClick(lastRightClickEvent);
         }
         break;