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 36 matching lines @@
 {
   return s.replace(/^[\d\-]|[^\w\-\u0080-\uFFFF]/g, escapeChar);
 }
 
 function highlightElement(element, shadowColor, backgroundColor)
 {
   unhighlightElement(element);
 
   var highlightWithOverlay = function()
   {
+    var overlay = addElementOverlay(element);
+
     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().

     {
       overlay.parentNode.removeChild(overlay);
     };
   };
 
   var highlightWithStyleAttribute = function()
   {
-    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 originalBoxShadow = element.style.getPropertyValue("box-shadow");
+    var originalBoxShadowPriority = element.style.getPropertyPriority("box-shadow");
+    var originalBackgroundColor = element.style.getPropertyValue("background-color");
     var originalBackgroundColorPriority = element.style.getPropertyPriority("background-color");
 
     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
       );
     };
   };
 
-  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.

+  if ("prisoner" in element)
+    highlightWithStyleAttribute();
+  else
     highlightWithOverlay();
-  else
-    highlightWithStyleAttribute();
 }
 
 
 function unhighlightElement(element)
 {
   if ("_unhighlight" in element)
   {
     element._unhighlight();
     delete element._unhighlight;
   }
@@ skipping 144 matching lines @@
 function addElementOverlay(elt) {
   // 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; 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;
@@ skipping 389 matching lines @@
       case "clickhide-close":
         if (clickHideFiltersDialog && msg.remove)
         {
           // Explicitly get rid of currentElement
           var element = currentElement.prisoner || currentElement;
           if (element && element.parentNode)
             element.parentNode.removeChild(element);
         }
         clickHide_deactivate();
         break;
-      default:
-        sendResponse({});
-        break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "report-html-page"});
 }