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
@@ -511,7 +511,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;
@@ -542,7 +542,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?
@@ -555,7 +555,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)
@@ -570,16 +570,25 @@
       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();
         }