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-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 536 matching lines @@
         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)
         {
           // Explicitly get rid of currentElement
-          var element = currentElement;
-          currentElement.parentNode.removeChild(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);
-          });
+            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;
     }
   });
 }