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 469 matching lines @@
       case "get-clickhide-state":
         sendResponse({active: clickHide_activated});
         break;
       case "clickhide-activate":
         clickHide_activate();
         break;
       case "clickhide-deactivate":
         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;
         // We hope the URL we are given is the same as the one in the element referenced
         // by lastRightClickEvent.target. If not, we just discard
         var target = lastRightClickEvent.target;
         var url = target.src;
         // If we don't have the element with a src URL same as the filter, look for it.
         // Chrome's context menu API is terrible. Why can't it give us the friggin' element
         // to start with?
@@ skipping 10 matching lines @@
               // This is hopefully our element. In case of multiple elements
               // with the same src, only one will be highlighted.
               target = elts[i];
               break;
             }
           }
         }
         // 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?
           clickHideFilters = [msg.filter];
           // Coerce red highlighted overlay on top of element to remove.
           // TODO: Wow, the design of the clickHide stuff is really dumb - gotta fix it sometime
           currentElement = addElementOverlay(target);
           currentElement_backgroundColor = target.style.backgroundColor;
           // clickHide_mouseOver(lastRightClickEvent);
           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)
         {
           sendResponse({filters: clickHide_filters});
 
           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) + 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();
         }
         break;
       default:
         sendResponse({});
         break;
     }
   });
 }