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

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 201 matching lines @@
   document.body.appendChild(overlay);
   return overlay;
 }
 
 // Show dialog asking user whether she wants to add the proposed filters derived
 // from selected page element
 function clickHide_showDialog(left, top, filters)
 {
   // 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;
 
   clickHideFiltersDialog = document.createElement("iframe");
   clickHideFiltersDialog.src = ext.getURL("block.html");
   clickHideFiltersDialog.setAttribute("style", "position: fixed !important; visibility: hidden; display: block !important; border: 0px !important;");
   clickHideFiltersDialog.style.WebkitBoxShadow = "5px 5px 20px rgba(0,0,0,0.5)";
   clickHideFiltersDialog.style.zIndex = 0x7FFFFFFF;
 
   // Position in upper-left all the time
@@ skipping 41 matching lines @@
 // on whether the user actually wants these filters
 function clickHide_rulesPending() {
   clickHide_activated = false;
   document.removeEventListener("mouseover", clickHide_mouseOver, true);
   document.removeEventListener("mouseout", clickHide_mouseOut, true);
   document.removeEventListener("click", clickHide_mouseClick, true);
   document.removeEventListener("keydown", clickHide_keyDown, true);
 }
 
 // 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)
   {
     document.body.removeChild(clickHideFiltersDialog);
     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)
     return; // This can happen inside a nuked iframe...I think
   document.removeEventListener("mouseover", clickHide_mouseOver, true);
   document.removeEventListener("mouseout", clickHide_mouseOut, true);
   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) {
   ev.preventDefault();
   ev.stopPropagation();
   clickHide_mouseClick(ev);
 }
 
 // Hovering over an element so highlight it
 function clickHide_mouseOver(e)
@@ skipping 383 matching lines @@
         break;
       default:
         sendResponse({});
         break;
     }
   });
 
   if (window == window.top)
     ext.backgroundPage.sendMessage({type: "report-html-page"});
 }