Issue 29901596: Issue 6493 - Ensure Firefox draws the "Block element" popup window (Closed)
DescriptionIssue 6493 - Ensure Firefox draws the "Block element" popup window
Patch Set 1 #
Total comments: 3
Patch Set 2 : Avoid hardcoding the width again #
Total comments: 4
Patch Set 3 : Keep the window's width correct #
Total comments: 13
Patch Set 4 : Only resize for Firefox users, but don't worry about the 2px #MessagesTotal messages: 18
Patch Set 1 https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: 422}); For some reason one pixel larger wasn't enough to have Firefox redraw the window, so I had to go with 422px.
https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: 422}); On 2018/10/04 15:30:37, kzar wrote: > For some reason one pixel larger wasn't enough to have Firefox redraw the > window, so I had to go with 422px. Would it work if we remove the width from the browser.windows.create() call, and then resize it to the correct width here? That way we would define the width only in one place, and we don't have to set it to a weird number like 422.
Hey, just wanted to say nice work here awesome catch and the fix seems pretty solid.
Patch Set 2 : Avoid hardcoding the width again > Hey, just wanted to say nice work here awesome catch and > the fix seems pretty solid. Thanks man :) https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29901597/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: 422}); On 2018/10/04 18:38:55, Sebastian Noack wrote: > On 2018/10/04 15:30:37, kzar wrote: > > For some reason one pixel larger wasn't enough to have Firefox redraw the > > window, so I had to go with 422px. > > Would it work if we remove the width from the browser.windows.create() call, and > then resize it to the correct width here? That way we would define the width > only in one place, and we don't have to set it to a weird number like 422. Well, I tried that but initially the width of the popup was much larger, then it shrunk down. It looked kind of janky! How about this instead? I avoid hard-coding the width again, instead just add 2px (the minimum required).
https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}); Still the window ends up 2 pixels larger as it meant to be. how about this: const WINDOW_WITH = 420; ... browser.window.create({..., width: WINDWOW_WIDTH - 2}, ...); ... browser.window.update(window.id, {width: WINDOW_WIDTH});
https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}); On 2018/10/08 15:11:53, Sebastian Noack wrote: > Still the window ends up 2 pixels larger as it meant to be. Well, I guess so, but I really don't think that matters. This approach also has the advantage that the workaround is in one place, so easier to remove in the future.
https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}); On 2018/10/08 15:58:28, kzar wrote: > On 2018/10/08 15:11:53, Sebastian Noack wrote: > > Still the window ends up 2 pixels larger as it meant to be. > > Well, I guess so, but I really don't think that matters. This approach also has > the advantage that the workaround is in one place, so easier to remove in the > future. I think this workaround should not change the effective size if not necessary.
Patch Set 3 : Keep the window's width correct https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904568/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}); On 2018/10/08 16:07:22, Sebastian Noack wrote: > On 2018/10/08 15:58:28, kzar wrote: > > On 2018/10/08 15:11:53, Sebastian Noack wrote: > > > Still the window ends up 2 pixels larger as it meant to be. > > > > Well, I guess so, but I really don't think that matters. This approach also > has > > the advantage that the workaround is in one place, so easier to remove in the > > future. > > I think this workaround should not change the effective size if not necessary. Done. https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } 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.
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } On 2018/10/08 17:23:15, kzar wrote: > 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. 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.
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } On 2018/10/08 17:33:56, Sebastian Noack wrote: > On 2018/10/08 17:23:15, kzar wrote: > > 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. > > 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. 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)
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } On 2018/10/08 17:33:56, Sebastian Noack wrote: > On 2018/10/08 17:23:15, kzar wrote: > > 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. > > 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. 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.
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:194: // https://issues.adblockplus.org/ticket/6493 Since this issue is specific to Firefox, mind if we avoid this workaround for other browsers? https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}).then( Detail: Do we even need to wait for the callback or can we just queue up another window update right away? https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } On 2018/10/09 09:58:47, kzar wrote: > I don't think an extra two pixels on the window width makes any difference. 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.
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:194: // https://issues.adblockplus.org/ticket/6493 On 2018/10/09 11:43:15, Thomas Greiner wrote: > Since this issue is specific to Firefox, mind if we avoid this workaround for > other browsers? I considered doing that, but it seems kind of inconsistent with the surrounding code. https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}).then( On 2018/10/09 11:43:15, Thomas Greiner wrote: > Detail: Do we even need to wait for the callback or can we just queue up another > window update right away? 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.
https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:194: // https://issues.adblockplus.org/ticket/6493 On 2018/10/09 11:48:28, kzar wrote: > I considered doing that, but it seems kind of inconsistent with the surrounding > code. 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. https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:195: browser.windows.update(window.id, {width: window.width + 2}).then( On 2018/10/09 11:48:28, kzar wrote: > 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. 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.
Patch Set 4 : Only resize for Firefox users, but don't worry about the 2px https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.js File lib/filterComposer.js (right): https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:194: // https://issues.adblockplus.org/ticket/6493 On 2018/10/09 12:00:27, Thomas Greiner wrote: > On 2018/10/09 11:48:28, kzar wrote: > > I considered doing that, but it seems kind of inconsistent with the > surrounding > > code. > > 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. Fair enough, Done. https://codereview.adblockplus.org/29901596/diff/29904573/lib/filterComposer.... lib/filterComposer.js:196: () => { browser.windows.update(window.id, {width: window.width}); } On 2018/10/09 11:43:15, Thomas Greiner wrote: > On 2018/10/09 09:58:47, kzar wrote: > > I don't think an extra two pixels on the window width makes any difference. > > 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. Alright, I won't bother shrink the window 2px again.
LGTM
LGTM 
|
