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
  *
  * 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 493 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 + 5) + "px";
           clickHideFiltersDialog.style.height = (msg.height + 5) + "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)

[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
-          if (msg.remove && currentElement && currentElement.parentNode)
-            currentElement.parentNode.removeChild(currentElement);
+          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.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.

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