issue 1435 - Port popup.html from Chrome/Safari/Opera to Firefox

chrome/content/ui/ext/popup.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 (function()
 {
   var {UI} = require("ui");
   
   var iframe = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
     .getInterface(Components.interfaces.nsIWebNavigation)
     .QueryInterface(Components.interfaces.nsIDocShell)
     .chromeEventHandler;
   var topWindow = iframe.ownerDocument.defaultView;
-  
+  var panel = iframe.parentNode;
+
   // Firefox doesn't adjust the size of the popup automatically to the size
-  // of its content, like when the ad counter is expanded/collapsed. So we add
-  // event listeners to do so.

[Thomas Greiner, 2014/10/08 10:40:43]

It's true that we're not using event listeners for that but we are using
MutationObserver.

[saroyanm, 2014/10/10 12:05:58]

Done.

-  var mayResize = true;

[Thomas Greiner, 2014/09/29 16:19:49]

Is this variable ever set to false?

[saroyanm, 2014/10/02 07:53:56]

Good point, removed.

+  // of its content, like when the ad counter is expanded/collapsed. So we use 
+  // MutationObserver to do so.
   var resizingScheduled = false;
 
   function updateSize()
   {
-    if (mayResize && !resizingScheduled)
+    if (!resizingScheduled)
     {
       setTimeout(function()
       {
-        iframe.parentNode.sizeTo(document.body.scrollWidth, document.body.offsetHeight + 11);

[Thomas Greiner, 2014/09/29 16:19:49]

Why `+ 11`?

[saroyanm, 2014/10/02 07:53:56]

I guess here I should use scrollHeight instead of offsetHeight, but in both
cases the height is cutting down on arround 11 pixel, It fills like a size of
scroller, so in this case it's looks to be a bug while the height of scrollbar
also should be included in offsetHeight, I guess I need more investigations
here.

[saroyanm, 2014/10/07 13:02:05]

Thomas, so here are the results of investigation:
panel.sizeTo method
(https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/panel#m-sizeTo)
change the size of panel, which include also the arrow and panel container, so
in case we are changing the size of panel of type="arrow" using above method,
then we also should calculate current sizes:
1. arrow icon size ->
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop...
in linux it's 9px
2. panel content style size ->
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/pop...

for #2 we are overwriting the padding rule setting to 0 in overlay.css but we
still have border of 1px. So 11px is "arrow icon" size + "panel content" border
= 11px, in Linux.

I'm not sure if it's a bug in Firefox, or this is how sizeTo method should work.

[Thomas Greiner, 2014/10/08 10:40:43]

Couldn't we check for those values on runtime? Those values might differ
depending on the platform Firefox is running on.
It's not specified so it might be. Better check for existing issue reports on
that or further documentation.

[saroyanm, 2014/10/10 12:05:58]

Yes, looks like we can, while iframe is stretching to the content of panel, so
we can calculate difference of iframe height and panel's height. 
There is no existing issue, but also in documentation it's not mentioned that
the method should resize the content part of panel, but the panel itself.

         resizingScheduled = false;
+
+        // We need to calculate the height difference because the panel's size includes 
+        // the height of the arrow image and the border width of the panel content
+        var heightDifference = panel.clientHeight - iframe.clientHeight;
+        panel.sizeTo(document.body.scrollWidth, document.body.scrollHeight + heightDifference);
       }, 0);
 
       resizingScheduled = true;
     }
   }
 
   window.addEventListener("load", function()
   {
     updateSize();
     new MutationObserver(updateSize).observe(document, {
       childList: true, attributes: true,
       characterData: true, subtree: true
     });
   });
 
   ext = {
     __proto__: ext,
     closePopup: function()
     {
-      iframe.parentNode.hidePopup();
+      panel.hidePopup();
     },
     openBlockable: function()
     {
-      if (!UI.isBottombarOpen(topWindow))
-      {
-        UI.toggleBottombar(topWindow);
-        ext.closePopup();
-      }

[Thomas Greiner, 2014/09/29 16:19:49]

What about the "else" case? Please look at the current implementation which
toggles the bottom bar and changes the button text.

[saroyanm, 2014/10/02 07:53:56]

I guess if you can clarify on the comment in content/ui/popup.js I will
understand the idea.

+      UI.toggleBottombar(topWindow);
+      ext.closePopup();
     },
-    showBlockable: function()
+    isBlockableOpen: function()
     {
-      return !UI.isBottombarOpen(topWindow);

[Thomas Greiner, 2014/09/29 16:19:49]

This function should no longer be necessary if you implement the above mentioned
behavior.

[saroyanm, 2014/10/02 07:53:56]

Same as comment above.

+      return UI.isBottombarOpen(topWindow);
     },
     reportIssue: function()
     {
       UI.openReportDialog(topWindow);
     },
     showReportIssue: function()
     {
       var location = UI.getCurrentLocation(topWindow);
       return location && Policy.isBlockableScheme(location) && location.scheme != "mailto";
     }
   };
 })();