Issue 1751 - Highlight elements by injecting an overlay

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 30 matching lines @@
 function quote(value)
 {
   return '"' + value.replace(/["\\\{\}\x00-\x1F\x7F]/g, escapeChar) + '"';
 }
 
 function escapeCSS(s)
 {
   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()
     {
       this.style.removeProperty("box-shadow");
       this.style.setProperty(
         "box-shadow",
         originalBoxShadow,
         originalBoxShadowPriority
       );
 
       this.style.removeProperty("background-color");
       this.style.setProperty(
         "background-color",
         originalBackgroundColor,
         originalBackgroundColorPriority
       );
     };
   };
 
-  // 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();
 }
 
 
 function unhighlightElement(element)
 {
   if ("_unhighlight" in element)
   {
     element._unhighlight();
@@ skipping 114 matching lines @@
   if (element.id)
     return true;
   if (element.classList.length > 0)
     return true;
   if (getURLsFromElement(element).length > 0)
     return true;
 
   // 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;
 }
 
 // Gets the absolute position of an element by walking up the DOM tree,
 // adding up offsets.
 // I hope there's a better way because it just seems absolutely stupid
 // that the DOM wouldn't have a direct way to get this, given that it
 // has hundreds and hundreds of other methods that do random junk.
 function getAbsolutePosition(elt) {
   var l = 0;
   var t = 0;
   for(; elt; elt = elt.offsetParent) {
     l += elt.offsetLeft;
     t += elt.offsetTop;
   }
   return [l, t];
 }
 
 // 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)
     return;
 
   var thisStyle = getComputedStyle(elt, null);
   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";
 
   if (thisStyle.position != "static")
     overlay.style.zIndex = thisStyle.zIndex;
   else
     overlay.style.zIndex = getComputedStyle(elt.offsetParent).zIndex;
 
   // elt.parentNode.appendChild(overlay, elt);
   document.body.appendChild(overlay);
@@ skipping 38 matching lines @@
 
 // Turn on the choose element to create filter thing
 function clickHide_activate() {
   if(document == null)
     return;
 
   // If we are already selecting, abort now
   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);
   document.addEventListener("mouseout", clickHide_mouseOut, true);
   document.addEventListener("click", clickHide_mouseClick, true);
   document.addEventListener("keydown", clickHide_keyDown, true);
 }
 
 // Called when user has clicked on something and we are waiting for confirmation
 // on whether the user actually wants these filters
@@ skipping 149 matching lines @@
       continue;
     }
 
     if (url == elt.src)
       addSelector(escapeCSS(elt.localName) + '[src=' + quote(elt.getAttribute("src")) + ']');
   }
 
   // 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) + ']');
   }
 
   // Show popup
   clickHide_showDialog(e.clientX, e.clientY, clickHideFilters);
 
   // Highlight the elements specified by selector in yellow
   if (selectorList.length > 0)
     highlightElements(selectorList.join(","));
@@ skipping 119 matching lines @@
       case "clickhide-activate":
         clickHide_activate();
         break;
       case "clickhide-deactivate":
         clickHide_deactivate();
         break;
       case "clickhide-new-filter":
         if(lastRightClickEvent)
         {
           clickHide_activated = true;
-          currentElement = addElementOverlay(lastRightClickEvent.target);
+          currentElement = lastRightClickEvent.target;
           clickHide_mouseClick(lastRightClickEvent);
         }
         break;
       case "clickhide-init":
         if (clickHideFiltersDialog)
         {
           sendResponse({filters: clickHide_filters});
 
           clickHideFiltersDialog.style.width = msg.width + "px";
           clickHideFiltersDialog.style.height = msg.height + "px";
@@ skipping 19 matching lines @@
         break;
       default:
         sendResponse({});
         break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "report-html-page"});
 }