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

lib/filterComposer.js

Index: lib/filterComposer.js
diff --git a/lib/filterComposer.js b/lib/filterComposer.js
index fe5dcdf3071e1762e31cfb4e4bd461a67bbcac23..14c07776c2d5a8cf0d206d52cd3b01bbd38675b6 100644
--- a/lib/filterComposer.js
+++ b/lib/filterComposer.js
@@ -188,6 +188,13 @@ port.on("composer.openDialog", (message, sender) =>
         // 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.

+        );
       }).catch(e =>
       {
         // Firefox sometimes sets the status for a window to "complete" before