Issue 1601 - Generate blocking filters for all URLs associated with the selected element

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -198,13 +198,11 @@
 
   // 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
-  var url = getElementURL(elt);
-  if(!elt.className && !elt.id && !url)
+  if(!hasFilters(elt))
     return;
   var thisStyle = getComputedStyle(elt, null);
   var overlay = document.createElement('div');
   overlay.prisoner = elt;
-  overlay.prisonerURL = url;
   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; z-index: 99999');
   var pos = getAbsolutePosition(elt);
@@ -265,7 +263,7 @@
     clickHide_deactivate();
 
   // Add overlays for elements with URLs so user can easily click them
-  var elts = document.querySelectorAll('object,embed,img,iframe');
+  var elts = document.querySelectorAll('object,embed,img,iframe,video,audio,picture');
   for(var i=0; i<elts.length; i++)
     addElementOverlay(elts[i]);
 
@@ -333,7 +331,7 @@
     return;
 
   var target = e.target;
-  while (target.parentNode && !(target.id || target.className || target.src || /:.+:/.test(target.getAttribute("style"))))
+  while (target.parentNode && !hasFilters(target))
     target = target.parentNode;
   if (target == document.documentElement || target == document.body)
     target = null;
@@ -377,6 +375,8 @@
   }
 }
 
+
+
 // When the user clicks, the currentElement is the one we want.
 // We should have ABP rules ready for when the
 // popup asks for them.
@@ -386,20 +386,17 @@
     return;
 
   var elt = currentElement;
-  var url = null;
   if (currentElement.classList.contains("__adblockplus__overlay"))
-  {
     elt = currentElement.prisoner;
-    url = currentElement.prisonerURL;
-  }
-  else if (elt.src)
-    url = elt.src;
 
   clickHideFilters = new Array();
   selectorList = new Array();
 
   var addSelector = function(selector)

[kzar, 2014/11/25 18:10:11]

Why we don't just do "function addSelector(selector)" instead?

[Sebastian Noack, 2014/11/25 19:01:15]

The only reason that comes to my mind would be that the function can not be
called before the two variables above have been defined. Note that normal
function definitions are automatically moved to the top of the scope. Not
necessarily reason enough to leave it that way. However, those lines didn't
change, and I rather don't want to put even more unrelated changes into this
patch.

   {
+    if (selectorList.indexOf(selector) != -1)
+      return;
+
     clickHideFilters.push(document.domain + "##" + selector);
     selectorList.push(selector);
   };
@@ -417,20 +414,31 @@
     addSelector(selector);
   }
 
-  if (url)
+  var urls = getElementURLs(elt);
+  for (var i = 0; i < urls.length; i++)
   {
-    var src = elt.getAttribute("src");
-    var selector = src && escapeCSS(elt.localName) + '[src=' + quote(src) + ']';
+    var url = urls[i];
+    var isHTTP = /^https?:/i.test(url);
 
-    if (/^https?:/i.test(url))
+    if (isHTTP)

[kzar, 2014/11/25 18:10:11]

I found this line a little bit confusing to start with as you're checking if the
URL is HTTP or HTTPS but it looked like you intended to just match HTTP.

[Sebastian Noack, 2014/11/25 19:01:15]

What I actually want, is excluding URLs that can't be handled by request
blocking. So yes, both http:// and https:// are fine, however file://, data:,
javascript: about:blank other special URLs aren't.

I would consider isHTTPorHTTPS as variable name unnecessarily long. And since
HTTPS is just an abbreviation for HTTP+TLS/SSL, it's still HTTP, and therefore I
don't see any wrong implications here.

     {
-      clickHideFilters.push(url.replace(/^[\w\-]+:\/+(?:www\.)?/, "||"));
+      var filter = url.replace(/^[\w\-]+:\/+(?:www\.)?/, "||");
 
-      if (selector)
+      if (clickHideFilters.indexOf(filter) != -1)
+        continue;
+
+      clickHideFilters.push(filter);
+    }
+
+    if (url == elt.src)
+    {
+      var selector = escapeCSS(elt.localName) + '[src=' + quote(elt.getAttribute("src")) + ']';
+
+      if (isHTTP)
         selectorList.push(selector);
+      else
+        addSelector(selector);
     }
-    else if (selector)
-      addSelector(selector);
   }
 
   // restore the original style, before generating the fallback filter that
@@ -458,29 +466,93 @@
   e.stopPropagation();
 }
 
-// Extracts source URL from an IMG, OBJECT, EMBED, or IFRAME
-function getElementURL(elt) {
-  // Check children of object nodes for "param" nodes with name="movie" that specify a URL
-  // in value attribute
-  var url;
-  if(elt.localName.toUpperCase() == "OBJECT" && !(url = elt.getAttribute("data"))) {
-    // No data attribute, look in PARAM child tags for a URL for the swf file
-    var params = elt.querySelectorAll("param[name=\"movie\"]");
-    // This OBJECT could contain an EMBED we already nuked, in which case there's no URL
-    if(params[0])
-      url = params[0].getAttribute("value");
-    else {
-      params = elt.querySelectorAll("param[name=\"src\"]");
-      if(params[0])
-        url = params[0].getAttribute("value");
-    }
+function parseSrcSet(element)
+{
+  if (!element.srcset)
+    return [];
 
+  var urls = element.srcset.split(",");
+  for (var i = 0; i < urls.length; i++)
+  {
+    var url = urls[i].replace(/^\s+/, "").replace(/(\s+\S+)?\s*$/, "");
     if (url)
-      url = resolveURL(url);
-  } else if(!url) {
-    url = elt.src || elt.href;
+      urls[i] = resolveURL(url);
+    else
+      urls.splice(i--, 1);

[kzar, 2014/11/25 18:10:11]

Does removing an element from the array you're iterating over cause any problems
in Javascript?

[Sebastian Noack, 2014/11/25 19:01:15]

Not as long as you decrease the iteration counter (that i will have the same
value during next iteration) as done here. Not that this is problamatic in any
programming language, when iterating by index (as opposed to using iterators).

[Sebastian Noack, 2014/11/25 19:03:11]

s/is problamatic/isn't problamatic/

   }
-  return url;
+
+  return urls;
+}
+
+function getElementURLs(elt) {
+  var urls = [];
+
+  if (elt.src)
+    urls.push(elt.src);
+
+  switch (elt.localName)
+  {
+    case "object":
+      var url = elt.getAttribute("data");
+      if (url)
+        return [resolveURL(url)];
+
+      for (var i = 0; i < elt.children.length; i++)
+      {
+        var child = elt.children[i];
+        if (child.localName != "param")
+          continue;
+
+        var name = child.getAttribute("name");
+        if (name != "movie" && name != "src")
+          continue;
+
+        var value = child.getAttribute("value");
+        if (!value)
+          continue;
+
+        return [resolveURL(value)];
+      }
+
+      return [];
+
+    case "video":
+    case "audio":
+    case "picture":
+      for (var i = 0; i < elt.children.length; i++)
+      {
+        var child = elt.children[i];
+
+        if (child.localName != "source")
+          continue;
+
+        if (child.src)
+          urls.push(child.src);
+
+        urls = urls.concat(parseSrcSet(child));
+      }
+
+      break;
+
+    case "img":
+      urls = urls.concat(parseSrcSet(elt));
+  }
+
+  return urls;
+}
+
+function hasFilters(element)

[kzar, 2014/11/26 10:51:22]

Maybe a name like "isFilterable" would be better than "hasFilters"?

[kzar, 2014/11/26 10:51:22]

How come you don't put this function above where it's used? Wouldn't that be
clearer?

[Sebastian Noack, 2014/11/26 11:08:14]

As discussed on IRC, it's called "isBlockable" now.

[Sebastian Noack, 2014/11/26 11:08:14]

Since it relies on "getElementURLs()", I just put it below that function. I
moved both function above now.

+{
+  if (element.id)
+    return true;
+  if (element.classList.length > 0)
+    return true;
+  if (getElementURLs(element).length > 0)
+    return true;
+  if (/:.+:/.test(element.getAttribute("style")))

[kzar, 2014/11/26 10:51:22]

Maybe comment this line to explain what you told me? I don't think it's so
obvious at first glance.

[Sebastian Noack, 2014/11/26 11:08:14]

Well, this logic isn't new. But I guess I can add a comment though.

+    return true;
+
+  return false;
 }
 
 // This function Copyright (c) 2008 Jeni Tennison, from jquery.uri.js