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

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -487,7 +487,7 @@
         clickHide_deactivate();
         break;
       case "clickhide-new-filter":
-        // The request is received by all frames, so ignore it if we're not the frame the
+        // The message is received by all frames, so ignore it if we're not the frame the
         // user right-clicked in
         if(!lastRightClickEvent)
           return;
@@ -518,7 +518,7 @@
         // Following test will be true if we found the element with the filter URL
         if(msg.filter === url)
         {
-          // This request would have come from the chrome.contextMenu handler, so we
+          // This message would have come from the chrome.contextMenu handler, so we
           // simulate the user having chosen the element to get rid of via the usual means.
           clickHide_activated = true;
           // FIXME: clickHideFilters is erased in clickHide_mouseClick anyway, so why set it?
@@ -531,7 +531,7 @@
           clickHide_mouseClick(lastRightClickEvent);
         }
         else
-          console.log("clickhide-new-filter: URLs don't match. Couldn't find that element.", request.filter, url, lastRightClickEvent.target.src);
+          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)
@@ -546,16 +546,48 @@
       case "clickhide-move":
         if (clickHideFiltersDialog)
         {
-          clickHideFiltersDialog.style.left = (parseInt(clickHideFiltersDialog.style.left, 10) + request.x) + "px";
-          clickHideFiltersDialog.style.top = (parseInt(clickHideFiltersDialog.style.top, 10) + request.y) + "px";
+          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
-          if (msg.remove && currentElement && currentElement.parentNode)
-            currentElement.parentNode.removeChild(currentElement);
+          var filters = msg.filters;
+          if (filters)
+          {
+            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();
         }