Issue 1610 - Don't remove overlays before the "Block element" dialog is closed

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -219,11 +219,7 @@
 {
   // If we are already selecting, abort now
   if (clickHide_activated || clickHideFiltersDialog)
-  {
-    var savedElement = (currentElement.prisoner ? currentElement.prisoner : currentElement);
-    clickHide_deactivate();
-    currentElement = savedElement;
-  }
+    clickHide_deactivate(true);
 
   clickHide_filters = filters;
 
@@ -285,7 +281,7 @@
 }
 
 // Turn off click-to-hide
-function clickHide_deactivate()
+function clickHide_deactivate(keepOverlays)

[kzar, 2014/11/26 14:14:04]

We're relying on the legacy code not passing this parameter and it therefore
being undefined?

[Sebastian Noack, 2014/11/26 14:41:39]

Correct. I tried to keep the patch as compact as possible this time.

 {
   if (clickHideFiltersDialog)
   {
@@ -293,15 +289,6 @@
     clickHideFiltersDialog = null;
   }
 
-  if(currentElement) {
-    currentElement.removeEventListener("contextmenu", clickHide_elementClickHandler, false);
-    unhighlightElements();
-    unhighlightElement(currentElement);
-    currentElement = null;
-    clickHideFilters = null;
-  }
-  unhighlightElements();
-
   clickHide_activated = false;
   clickHide_filters = null;
   if(!document)
@@ -311,11 +298,21 @@
   document.removeEventListener("click", clickHide_mouseClick, true);
   document.removeEventListener("keydown", clickHide_keyDown, true);
 
-  // Remove overlays
-  // For some reason iterating over the array returend by getElementsByClassName() doesn't work

[Sebastian Noack, 2014/11/26 12:03:29]

It's not an Array, but a NodeList, and therefore lazy evaluated. Hence when you
remove the first element, the second element becomes the first element. But you
can still use getElementsByClassName() and iterate over the first element of the
NodeList until its empty. While this is unrelated to what this patch is actually
about, I felt free to address this below.

-  var elt;
-  while(elt = document.querySelector('.__adblockplus__overlay'))
-    elt.parentNode.removeChild(elt);
+  if (!keepOverlays)
+  {
+    if(currentElement) {

[kzar, 2014/11/26 14:14:04]

Nitpick: Should be space between "if" and "("?

[Sebastian Noack, 2014/11/26 14:41:39]

I just copied the code from above, but I have added the space now.

+      currentElement.removeEventListener("contextmenu", clickHide_elementClickHandler, false);

[kzar, 2014/11/26 14:14:04]

Last parameter should be true?

[Sebastian Noack, 2014/11/26 14:41:39]

Yes, this patch was created on top of a version, were it still be false. Should
be fixed after rebase.

+      unhighlightElements();
+      unhighlightElement(currentElement);
+      currentElement = null;
+      clickHideFilters = null;
+    }
+    unhighlightElements();
+
+    var overlays = document.getElementsByClassName("__adblockplus__overlay");
+    while (overlays.length > 0)
+      overlays[0].parentNode.removeChild(overlays[0]);
+  }
 }
 
 function clickHide_elementClickHandler(ev) {