Closed Bug 1373238 Opened 3 years ago Closed 3 years ago

browser_ext_browserAction_popup_resize.js complains when run on hidpi machines

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [test], triaged)

Attachments

(1 file)

Spinning this off from bug 1372309, given the concerns Andrew raised that the changes might be masking a real bug where the fact that there's a scrollMax of 1 might be indicating that the contents of the panel are indeed scrollable.
(In reply to Andrew Swan [:aswan] from bug 1372309 comment #21)
> (In reply to :Gijs from comment #19)
> > (In reply to Andrew Swan [:aswan] from comment #16)
> > > > -  is(win.scrollMaxY, 0, "Document should not be vertically scrollable");
> > > > +  Assert.lessOrEqual(win.scrollMaxY, 1, "Document should not be vertically scrollable");
> > > 
> > > nit: should this (and the others below) be `Assert.less()`?  If scrollMaxY
> > > is exactly 1, it will be scrollable yes?
> > 
> > That would be better, but AFAICT scrolMaxY is rounded to ints. And the item
> > is sometimes scrollable, IIRC, but I guess that's a rounding error
> > somewhere, if it's literally exactly 1px? We can file a followup for this if
> > you prefer, it doesn't happen in automation but it happens for me both pre-
> > and post-photon when running locally on a hidpi machine... Let me know what
> > you want me to do...
> 
> I'm happy to keep it as-is but I haven't worked much with this test, I'm
> going to redirect to Kris who has...

Transferring this ni to this bug, with the patch now attached here.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P3
Whiteboard: [test], triaged
Mark, could you take a look at this one? I think its a matter of trying and reviewing Gijs patch at this point.
Flags: needinfo?(mstriemer)
Comment on attachment 8878027 [details]
Bug 1373238 - make popup resize test not care about rounding errors so it works on local hidpi machines

https://reviewboard.mozilla.org/r/149444/#review161684

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:32
(Diff revision 1)
>    let browser = await openPanel(extension, undefined, true);
>  
>    async function checkSize(expected) {
>      let dims = await promiseContentDimensions(browser);
>  
> -    is(dims.window.innerHeight, expected, `Panel window should be ${expected}px tall`);
> +    Assert.lessOrEqual(Math.abs(dims.window.innerHeight - expected), 1,

Line 38 is basically the same but for width and uses `ok(Math.abs(...) <= 1, ...)`. Either version seems fine to me but it would be nice to be consistent here.
So this fixes the test for me on my MBP, that seems reasonable.

I cannot comment on if this is hiding a bug though, I've never looked at this code before. It seems like we don't want it to scroll, so scrolling 1 pixel would be a bug.
Comment on attachment 8878027 [details]
Bug 1373238 - make popup resize test not care about rounding errors so it works on local hidpi machines

https://reviewboard.mozilla.org/r/149444/#review162094

I can't reproduce this with an add-on using what seems similar to this test case that fails. Seems like it should be fine to merge to me.
Attachment #8878027 - Flags: review+
Flags: needinfo?(mstriemer)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4434cdfcb3
make popup resize test not care about rounding errors so it works on local hidpi machines, r=mstriemer
https://hg.mozilla.org/mozilla-central/rev/7c4434cdfcb3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.