Issue 370 - Right-clicked element is removed independent of created filter

include.postload.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2013 Eyeo GmbH
+ * 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
@@ skipping 278 matching lines @@
   if (elt.className)
     elementClasses = elt.className.replace(/\s+/g, ' ').replace(/^\s+|\s+$/g, '').split(' ');
 
   clickHideFilters = new Array();
   selectorList = new Array();
   if (elementId)
   {
     clickHideFilters.push(document.domain + "###" + elementId);
     selectorList.push("#" + elementId);
   }
-  if (elementClasses)
-  {
-    for(var i = 0; i < elementClasses.length; i++)
+  if (elementClasses && elementClasses.length > 0)
+  {
+    var selector = elementClasses.map(function(elClass)
     {
-      clickHideFilters.push(document.domain + "##." + elementClasses[i]);
-      selectorList.push("." + elementClasses[i]);
-    }
+      return "." + elClass.replace(/([^\w-])/, "\\$1");
+    }).join("");
+
+    clickHideFilters.push(document.domain + "##" + selector);
+    selectorList.push(selector);
   }
   if (url)
   {
     clickHideFilters.push(relativeToAbsoluteUrl(url));
     selectorList.push(elt.localName + '[src="' + url + '"]');
   }
 
   // Show popup
   clickHide_showDialog(e.clientX, e.clientY, clickHideFilters);
 
@@ skipping 25 matching lines @@
       url = params[0].getAttribute("value");
     else {
       params = elt.querySelectorAll("param[name=\"src\"]");
       if(params[0])
         url = params[0].getAttribute("value");
     }
   } else if(!url) {
     url = elt.getAttribute("src") || elt.getAttribute("href");
   }
   return url;
-}
-
-// Converts relative to absolute URL
-// e.g.: foo.swf on http://example.com/whatever/bar.html
-//  -> http://example.com/whatever/foo.swf
-function relativeToAbsoluteUrl(url)
-{
-  // If URL is already absolute, don't mess with it
-  if (!url || /^[\w\-]+:/i.test(url))
-    return url;
-
-  // Leading / means absolute path
-  // Leading // means network path
-  if (url[0] == '/')
-  {
-    if (url[1] == '/')
-      return document.location.protocol + url;
-    else
-      return document.location.protocol + "//" + document.location.host + url;
-  }
-
-  // Remove filename and add relative URL to it
-  var base = document.baseURI.match(/.+\//);
-  if (!base)
-    return document.baseURI + "/" + url;
-  return base[0] + url;
 }
 
 // This function Copyright (c) 2008 Jeni Tennison, from jquery.uri.js
 // and licensed under the MIT license. See jquery-*.min.js for details.
 function removeDotSegments(u) {
   var r = '', m = [];
   if (/\./.test(u)) {
     while (u !== undefined && u !== '') {
       if (u === '.' || u === '..') {
         u = '';
@@ skipping 165 matching lines @@
           clickHide_mouseClick(lastRightClickEvent);
         }
         else
           console.log("clickhide-new-filter: URLs don't match. Couldn't find that element.", msg.filter, url, lastRightClickEvent.target.src);
         break;
       case "clickhide-init":
         if (clickHideFiltersDialog)
         {
           sendResponse({filters: clickHide_filters});
 
-          clickHideFiltersDialog.style.width = (msg.width + 5) + "px";
-          clickHideFiltersDialog.style.height = (msg.height + 5) + "px";
+          clickHideFiltersDialog.style.width = msg.width + "px";
+          clickHideFiltersDialog.style.height = msg.height + "px";
           clickHideFiltersDialog.style.visibility = "visible";
         }
         break;
       case "clickhide-move":
         if (clickHideFiltersDialog)
         {
           clickHideFiltersDialog.style.left = (parseInt(clickHideFiltersDialog.style.left, 10) + msg.x) + "px";
           clickHideFiltersDialog.style.top = (parseInt(clickHideFiltersDialog.style.top, 10) + msg.y) + "px";
         }
         break;
       case "clickhide-close":
         if (clickHideFiltersDialog)

[Wladimir Palant, 2014/01/10 16:28:56]

You should add |&& currentElement| check here which was performed by the
original code - it might have been unset somewhere else already.

         {
           // Explicitly get rid of currentElement
-          var element = currentElement;
-          ext.backgroundPage.sendMessage({
-            type: "should-collapse",
-            url: element.src,
-            documentUrl: document.URL,
-            mediatype: typeMap[element.localName]
-          }, function(response)
+          var filters = msg.filters;
+          if (filters)
           {
-            if (response && element.parentNode)
-              element.parentNode.removeChild(element);
-          });

[Wladimir Palant, 2014/01/10 16:28:56]

What if an element hiding filter was added? Not only will
typeMap[element.localName] be bogus then, the element won't match any filters -
even if it has a URL.

[Sebastian Noack, 2014/06/25 16:40:59]

The problem is not (only) that the element is removed when the filter was edited
and doesn't match currentElement anymore.

What is if an other element matches after the filter was edited? What is if the
filter matches multiple elements? Also the wrong elements are highlighted when
the filter has been edited.

So what we actually should do, is updating the highlighted elements as soon as
the filter is edited. And finally removing the highlighted elements instead
currentElement.

[Wladimir Palant, 2014/06/25 18:49:34]

We don't care, going through the entire document looking for matching elements
would go too far. We merely want to give immediate feedback - and we do it by
removing the element that the user selected, *if* it actually should be removed
of course.

[Sebastian Noack, 2014/06/25 19:05:05]

Actually we exactly do that already, when the matching elements are highlighted
when the dialog pops up. See highlightElements().
IMO users would rather expect all matching elements to be removed immediately.
Alternatively, we could call the button "Save and reload page" and doing so,
which would also have immediate effect, fix this issue and lets us eliminate
some code.

[Thomas Greiner, 2014/07/17 15:35:49]

This issue only addresses the case when a filter doesn't match but the selected
element getting removed regardless. I can prepend a check that tests whether an
element hiding filter matches the selected element.

Any further changes should be discussed separately because if we only want to
allow removal of the selected element then a text input field is clearly the
wrong UI element. So either we allow the input of any kind of filters and show
the results to the user or we limit its capabilities e.g. by changing it to
checkboxes.

[Wladimir Palant, 2014/07/17 15:55:10]

That would be the best solution. If that's too complicated we could continue
always removing the element if an element hiding filter is added.

+            var isHidden = false;
+            var selectors = [];
+            for (var i = 0; i < filters.length; i++)
+            {
+              var selector = filters[i].match(/##(.*)$/);
+              if (selector)
+              {
+                if (currentElement.matches(selector[1]))
+                  isHidden = true;
+                
+                selectors.push(selector[1]);
+              }
+            }
+            if (setElemhideCSSRules)
+              setElemhideCSSRules(selectors);

[Wladimir Palant, 2014/11/17 19:53:57]

The code above has multiple issues:

* It doesn't consider domain restrictions of element hiding rules.
* It doesn't consider existing whitelisting rules.
* It uses the setElemhideCSSRules function to insert a second stylesheet and
potentially create a second shadow root, something that is known to have
side-effects like resetting all CSS transitions and such.

After thinking through it, the best solution seems to be sending a new
"get-selectors" message and updating the stylesheet we have in this document
already (which might not exist so we'll have to create one, but some
side-effects in this rare scenario should be ok).

[Thomas Greiner, 2014/11/27 14:46:07]

What we could do is instead of just forwarding the "clickhide-close" message
through the background page we could reuse the same logic used for
"get-selectors" to filter them right there and only return the ones to the
content script that apply.

+
+            if (!isHidden)
+            {
+              var element = currentElement;
+              ext.backgroundPage.sendMessage({
+                type: "should-collapse",
+                url: element.src,
+                documentUrl: document.URL,
+                mediatype: typeMap[element.localName]
+              }, function(response)
+              {
+                if (response && element.parentNode)
+                  element.style.setProperty("display", "none", "important");

[Wladimir Palant, 2014/11/17 19:53:57]

We don't need to check the element's parent node if we aren't removing it.
Either way, I think that calling checkCollapse() here makes more sense than
duplicating the logic (that function also treats frames correctly).

[Sebastian Noack, 2014/11/27 12:43:40]

What is if element collapsing is disabled in the options? Currently the element
is removed regardless, which is a bug (#1626). The behavior with this patch
would be that the element keeps showing, which isn't any better.

How about cloning the element and replacing the element with its clone (in case
a blocking filter were added), that it gets blocked via the webRequest API, but
only collapsed if supposed to?

[Thomas Greiner, 2014/11/27 14:46:07]

That's a valid comment and it doesn't seem like we can find a non-hacky solution
for this. To accomplish to show the placeholder, I'd still prefer to simply
change the URL of the element though.

Personally, I'd rather remove the "remove placeholders" option from the options
page since it causes so much confusion for the user and unnecessary logic in
this case. But that's a discussion that should be held outside of this review,
of course.

[Sebastian Noack, 2014/11/27 15:15:48]

If that is sufficient, fine for me.
That's what I thought. But then I remembered that there is also a filter list
option to disable element collapsing. :/

+              });
+            }
+          }
 
           clickHide_deactivate();
         }
         break;
       default:
         sendResponse({});
         break;
     }
   });
 }