Issue 6493 - Ensure Firefox draws the "Block element" popup window

lib/filterComposer.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 173 matching lines @@
         filters: message.filters
       }).then(response =>
       {
         // Sometimes sendMessage incorrectly reports a success on Firefox, so
         // we must check the response too.
         if (!response)
           throw new Error();
 
         // Sometimes Firefox doesn't draw the window's contents initially, so we
         // resize the window slightly as a workaround.
         // https://issues.adblockplus.org/ticket/6493

[Thomas Greiner, 2018/10/09 11:43:15]

Since this issue is specific to Firefox, mind if we avoid this workaround for
other browsers?

[kzar, 2018/10/09 11:48:28]

I considered doing that, but it seems kind of inconsistent with the surrounding
code.

[Thomas Greiner, 2018/10/09 12:00:27]

I'm not the one to judge but I'd prefer a good UX over small code
inconsistencies. Because it'd be a shame to punish all our users for something
that only occurs in Firefox - especially if we can easily avoid it.

[kzar, 2018/10/09 12:16:37]

Fair enough, Done.

-        browser.windows.update(window.id, {width: window.width + 2}).then(

[Thomas Greiner, 2018/10/09 11:43:15]

Detail: Do we even need to wait for the callback or can we just queue up another
window update right away?

[kzar, 2018/10/09 11:48:28]

Well, doing it this way seemed better since I worried that we might end up with
a race condition where if the +2 and -2 happened at the same time the window
wouldn't be redrawn. I've no idea if that's actually a problem however.

[Thomas Greiner, 2018/10/09 12:00:27]

Presumably, it's a similar assumption as whether +2 and -2 will be sufficient to
cover all cases.

But fair enough, unless someone looks into the Firefox code to find out more,
minimizing the amount of assumptions is likely not a bad idea.

-          () => { browser.windows.update(window.id, {width: window.width}); }

[kzar, 2018/10/08 17:23:15]

I checked and the `window.width` property isn't updated as the window's width
changes, so we know it's 420px even after the first resize.

[Sebastian Noack, 2018/10/08 17:33:56]

So now we are creating the window with a width of 420px, then changing it to
422px and then back to 420px. Is this really worth it, just to keep the logic
related to this workaround in subsequent lines? I think my suggested approach,
setting the width initially to 418px (intended width - 2px) and then changing it
only once to 420px is more straight forward.

[Jon Sonesen, 2018/10/08 18:59:06]

Yeah, since this code always ends up updating the window perhaps it does make
sense to initialize at 418px (assuming it doesn't make the logic/ui messy)

[kzar, 2018/10/09 09:58:47]

I don't think an extra two pixels on the window width makes any difference. I
don't think resizing the window twice vs once makes any difference. I want to
keep the workaround code together, so it's easier to remove again in the future.

[Thomas Greiner, 2018/10/09 11:43:15]

Indeed, 2px more or less shouldn't make much of a difference because the content
should be flexible enough to adapt to whichever size we prefer.

[kzar, 2018/10/09 12:16:37]

Alright, I won't bother shrink the window 2px again.

-        );
+        if (info.application == "firefox")
+          browser.windows.update(window.id, {width: window.width + 2});
       }).catch(e =>
       {
         // Firefox sometimes sets the status for a window to "complete" before
         // it is ready to receive messages[1]. As a workaround we'll try again a
         // few times with a second delay.
         // [1] - https://bugzilla.mozilla.org/show_bug.cgi?id=1418655
         setTimeout(doInit, 100);
       });
     };
     if (window.tabs[0].status != "complete")
@@ skipping 157 matching lines @@
 port.on("composer.isPageReady", (message, sender) =>
 {
   return readyActivePages.has(new ext.Page({id: message.pageId}));
 });
 
 port.on("composer.ready", (message, sender) =>
 {
   readyActivePages.set(sender.page, !checkWhitelisted(sender.page));
   updateContextMenu(sender.page);
 });