Issue 6645 - Collapse elements through user style sheets if possible

include.preload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 113 matching lines @@
 
   for (let i = 0; i < urls.length; i++)
   {
     if (/^(?!https?:)[\w-]+:/i.test(urls[i]))
       urls.splice(i--, 1);
   }
 
   return urls;
 }
 
-function isCollapsibleMediaElement(element, mediatype)
+function getSelectorForBlockedElement(element)
 {
-  if (mediatype != "MEDIA")
-    return false;
+  // Microsoft Edge does not support CSS.escape(). However, it doesn't
+  // support user style sheets either. So the selector would be added
+  // with an author style sheet anyway, which doesn't provide any benefits.
+  if (!("escape" in CSS))
+    return null;
 
-  if (!element.getAttribute("src"))
-    return false;
+  // Setting the "display" CSS property to "none" doesn't have any effect on
+  // <frame> elements (in framesets). So we have to hide it inline through
+  // the "visibility" CSS property.
+  if (element.localName == "frame")
+    return null;
 
-  for (let child of element.children)
+  // If the <video> or <audio> element contains any <source> or <track>
+  // children, we cannot address it in CSS by the source URL; in that case we
+  // don't "collapse" it using a CSS selector but rather hide it directly by
+  // setting the style="..." attribute.
+  if (element.localName == "video" || element.localName == "audio")
   {
-    // If the <video> or <audio> element contains any <source> or <track>
-    // children, we cannot address it in CSS by the source URL; in that case we
-    // don't "collapse" it using a CSS selector but rather hide it directly by
-    // setting the style="..." attribute.
-    if (child.localName == "source" || child.localName == "track")
-      return false;
+    for (let child of element.children)
+    {
+      if (child.localName == "source" || child.localName == "track")
+        return null;
+    }
   }
 
-  return true;
-}
+  let selector = element.localName;
+  let hasSrc = false;

[Manish Jethani, 2018/05/10 01:57:17]

Only one small nit, and this is totally a nit. We could avoid the hasSrc
variable here by setting selector to "" initially, then later:

  if (selector)
    return element.localName + selector;

  return null;

But anyway I don't care about this.

LGTM

[Sebastian Noack, 2018/05/10 11:45:33]

Done.

+  for (let attr of ["src", "srcset"])

[Manish Jethani, 2018/05/09 23:11:09]

If I understand correctly [1], srcset is only relevant for <img>. In that case,
the src attribute is treated as if it were part of srcset, and srcset can
override the value of src with a more suitable image.

We have two options:

 1.  Change the order here so that srcset takes precedence. This is problematic
because we may still be blocking the URL specified in src, but we'll end up
hiding based on srcset, and yet if the value of src changes to a non-blocked
URL, the element will remain hidden. Similarly, the element will get displayed
if the value of srcset changes, even though we've blocked the actual image URL
based on src.

 2.  Hide based on both src and srcset (i.e. img[src="foo"][srcset="bar"]. Here
we'll be hiding based on the "aggregate" of src and srcset so that if any of the
values changes, the element gets displayed again. Arguably this is better,
because if the image URL is blocked again after one of these values is updated,
the element will get hidden again.

What do you think?

[1]:
https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/...

[Sebastian Noack, 2018/05/10 01:08:22]

#2 is what my current implementation does. Also consider: If anything changes
that triggers another filter hit and therefore another error event, we just add
another selector to the style sheet with the current implementation.

[Manish Jethani, 2018/05/10 01:57:17]

Ah, I didn't realize you were not breaking from the loop when a selector was
added.

+  {
+    let value = element.getAttribute(attr);
+    if (value && attr in element)
+    {
+      selector += "[" + attr + "=" + CSS.escape(value) + "]";
+      hasSrc = true;
+    }
+  }
 
-function collapseMediaElement(element, srcValue)
-{
-  if (!srcValue)
-    return;
-
-  let selector = element.localName + "[src=" + CSS.escape(srcValue) + "]";
-
-  // Adding selectors is expensive so do it only if we really have a new
-  // selector.
-  if (!collapsingSelectors.has(selector))
-  {
-    collapsingSelectors.add(selector);
-    elemhide.addSelectors([selector], null, "collapsing", true);
-  }
+  return hasSrc ? selector : null;
 }
 
 function hideElement(element)
 {
   function doHide()
   {
     let propertyName = "display";
     let propertyValue = "none";
     if (element.localName == "frame")
     {
@@ skipping 19 matching lines @@
 function checkCollapse(element)
 {
   let mediatype = typeMap.get(element.localName);
   if (!mediatype)
     return;
 
   let urls = getURLsFromElement(element);
   if (urls.length == 0)
     return;
 
-  let collapsibleMediaElement = isCollapsibleMediaElement(element, mediatype);
-
-  // Save the value of the src attribute because it can change between now and
-  // when we get the response from the background page.
-  let srcValue = collapsibleMediaElement ? element.getAttribute("src") : null;
+  // Construct the selector here, because the attributes it relies on can change
+  // between now and when we get the response from the background page.
+  let selector = getSelectorForBlockedElement(element);
 
   browser.runtime.sendMessage(
     {
       type: "filters.collapse",
       urls,
       mediatype,
       baseURL: document.location.href
     },
     collapse =>
     {
       if (collapse)
       {
-        if (collapsibleMediaElement)
-          collapseMediaElement(element, srcValue);
+        if (selector)
+        {
+          if (!collapsingSelectors.has(selector))
+          {
+            collapsingSelectors.add(selector);
+            elemhide.addSelectors([selector], null, "collapsing", true);
+          }
+        }
         else
+        {
           hideElement(element);
+        }
       }
     }
   );
 }
 
 function checkSitekey()
 {
   let attr = document.documentElement.getAttribute("data-adblockkey");
   if (attr)
     browser.runtime.sendMessage({type: "filters.addKey", token: attr});
@@ skipping 359 matching lines @@
     let element = event.target;
     if (/^i?frame$/.test(element.localName))
       checkCollapse(element);
   }, true);
 }
 
 window.checkCollapse = checkCollapse;
 window.elemhide = elemhide;
 window.typeMap = typeMap;
 window.getURLsFromElement = getURLsFromElement;