1556323 - Firefox dropdown and extensions dropdowns are trimmed in multi-screen setups (that include non-100%/1.0dpi)

Status: RESOLVED DUPLICATE of bug 1746774

(Firefox :: Menus, defect, P3)

Description

[Rémi]

Attachment: actual result

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Environment :
Windows 10, dual screen, both 1920x1080. One of them is zoomed to 125%

Open Firefox on the 125% screen
Click on the hamburger menu

Actual results:

Dropdown's height is very short, only the first four item are shown.
See screenshot

Expected results:

The dropdown should be taller

Comment 1

[:Gijs (out for now; he/him)]

This is the same as bug 1555928, but we don't have good steps to reproduce. Does this only happen in a multi-screen setup? Would it be possible for you to run mozregression ( https://mozilla.github.io/mozregression/ ) to find out when this broke?

Comment 2

[Rémi]

It works fine with only my 125% laptop screen so yes, i'd say this only happen on a multi-screen setup.

I did some other tests : it happens when the screen setup is top-bottom AND the main screen is top AND the 125% screen is bottom.

Also, when firefox is not maximized and at the very bottom of the screen - to force the menu to dropdown upwards - it shows at full size. (see screenshot "Actual-upwards.png")

mozregression shows that the bug appeared between 2017-06-15 and 2017-06-16, I'll attach the entire log (see line 362).

Comment 3

[Rémi]

Attachment: Actual-upwards.png

Comment 4

[Rémi]

Attachment: mozregression-log.txt

Comment 5

[:Gijs (out for now; he/him)]

So the regression window is:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=035c25bef7b5e4175006e63eff10c61c2eef73f1&tochange=fe809f57bf2287bb937c3422ed03a63740b3448b

I expect this is just from the photon enabling (bug 1372309) which unfortunately doesn't tell us very much...

Jared, are you in a position to reproduce this?

Comment 6

[:Gijs (out for now; he/him)]

FWIW, I expect the problem is at https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1060-1085 and https://bugzilla.mozilla.org/show_bug.cgi?id=1474783#c17 might help here, but I don't really have a set up to investigate this myself and work out exactly what's wrong at the moment. If nobody else does either I can see if I can get something to work next week.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I don't have a multimonitor setup handy at the moment.

Comment 8

[:Gijs (out for now; he/him)]

OK, I'll try to get back to this early next week.

Comment 10

[:Gijs (out for now; he/him)]

The comments here https://bugzilla.mozilla.org/show_bug.cgi?id=1474783#c17 are on point. The problem is the calculations here pass CSS pixel screen coordinates into APIs that expect desktop/screen/device ones.

The tricky thing is that there are no JS-accessible APIs to convert the two, and using window.devicePixelRatio as the comment suggests is not sufficient, because (a) the window's associated screen isn't necessarily where the click happens (if the window is partially on one screen and partially on another) and (b) the window screen's device pixel ratio can be different from that of other screens, which would mean the calculation would be wrong (e.g. if you had a screen at 200% scale, with CSS pixels being 1024x768, and another screen underneath that at 100% scale, the hamburger button close to the top of the second screen would be at around 8-900 css pixels but more like 15-1600 desktop pixels, despite the window's screen's dpi being 1.0 / 100%.

I think the simplest might be to have an API on the screen manager to get the right screen based on CSS pixel coordinates. :agashlin, does that sound sane or am I missing a more straightforward solution?

Comment 11

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I think you can scale by window.devicePixelRatio in the screenForRect args because the dimensions come from the anchor node, which I suspect is part of the same window and so should have the same scaling. _calculateMaxHeight can just use this to find the screen that the anchor is on, it has the logic for using that screen's scaling once it gets the right screen.

At least that's how I remember it, I haven't looked into this again in depth (trying to avoid leaving this waiting for months).

Adding this API to the screen manager might still be nice, but how would it be able to convert the CSS pixels without knowing what window's scaling to use?

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #11)

I think you can scale by window.devicePixelRatio in the screenForRect args because the dimensions come from the anchor node, which I suspect is part of the same window

Yes.

and so should have the same scaling.

Not necessarily, because the window could be split across screens. It'd probably still be a better trade-off than the status quo in the short term...

Adding this API to the screen manager might still be nice, but how would it be able to convert the CSS pixels without knowing what window's scaling to use?

Well... the openPopupAtScreen code takes CSS screen pixels, right? Does that not already work? Because then presumably this could do the same thing.

In general, I assume the screen manager has the complete list of screens and their size + position + scaling, which means it should be able to work out screens' position in CSS coordinates, too, not just in desktop pixels. Right?

Comment 13

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to :Gijs (he/him) from comment #12)

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #11)

I think you can scale by window.devicePixelRatio in the screenForRect args because the dimensions come from the anchor node, which I suspect is part of the same window

Yes.

and so should have the same scaling.

Not necessarily, because the window could be split across screens. It'd probably still be a better trade-off than the status quo in the short term...

But even if the window is split across screens, shouldn't the whole window have one scale, including the anchor (which I think is the button that was clicked that we're popping from)?

Adding this API to the screen manager might still be nice, but how would it be able to convert the CSS pixels without knowing what window's scaling to use?

Well... the openPopupAtScreen code takes CSS screen pixels, right? Does that not already work? Because then presumably this could do the same thing.

That uses the scale from presContext through DevPixelsToAppUnits: https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/layout/xul/nsMenuPopupFrame.cpp#1474-1477 , I don't think that's fundamentally different from getting it out of the window.

In general, I assume the screen manager has the complete list of screens and their size + position + scaling, which means it should be able to work out screens' position in CSS coordinates, too, not just in desktop pixels. Right?

I think it becomes ambiguous. With the following screens in desktop pixels:

• 0: (0, 0)-(100, 100) 1x
• 1: (100, 0)-(200, 100) 2x

they look like this in CSS pixels for a window on that screen:

• 0: (0, 0)-(100, 100)
• 1: (50, 0)-(100, 50)

They overlap at (50, 0)-(100, 50), e.g. CSS coords (75, 0) could be on either. That's my intuition anyway, please explain if I've messed it up.

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #13)

(In reply to :Gijs (he/him) from comment #12)

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #11)

I think you can scale by window.devicePixelRatio in the screenForRect args because the dimensions come from the anchor node, which I suspect is part of the same window

Yes.

and so should have the same scaling.

Not necessarily, because the window could be split across screens. It'd probably still be a better trade-off than the status quo in the short term...

But even if the window is split across screens, shouldn't the whole window have one scale, including the anchor (which I think is the button that was clicked that we're popping from)?

I... don't know. And at all-hands I don't have a good way of finding out. :-(

Adding this API to the screen manager might still be nice, but how would it be able to convert the CSS pixels without knowing what window's scaling to use?

Well... the openPopupAtScreen code takes CSS screen pixels, right? Does that not already work? Because then presumably this could do the same thing.

That uses the scale from presContext through DevPixelsToAppUnits: https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/layout/xul/nsMenuPopupFrame.cpp#1474-1477 , I don't think that's fundamentally different from getting it out of the window.

Oh, huh. OK...

In general, I assume the screen manager has the complete list of screens and their size + position + scaling, which means it should be able to work out screens' position in CSS coordinates, too, not just in desktop pixels. Right?

I think it becomes ambiguous. With the following screens in desktop pixels:

• 0: (0, 0)-(100, 100) 1x
• 1: (100, 0)-(200, 100) 2x

they look like this in CSS pixels for a window on that screen:

• 0: (0, 0)-(100, 100)
• 1: (50, 0)-(100, 50)

They overlap at (50, 0)-(100, 50), e.g. CSS coords (75, 0) could be on either. That's my intuition anyway, please explain if I've messed it up.

I would have assumed that the CSS pixels for screen 1 would be (100, 0)-(150-50). That is, I assume that the desktop coords of screen 1 don't get scaled with its own pixel ratio in order to get the css pixels, I'd assume we use the scale factors for the "earlier" screens for that.

Maybe Jonathan knows?

Comment 15

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to :Gijs (he/him) from comment #14)

But even if the window is split across screens, shouldn't the whole window have one scale, including the anchor (which I think is the button that was clicked that we're popping from)?

I... don't know. And at all-hands I don't have a good way of finding out. :-(

VirtualBox is pretty good for simulating multiple monitor setups, that's mostly what I've used to test these.

They overlap at (50, 0)-(100, 50), e.g. CSS coords (75, 0) could be on either. That's my intuition anyway, please explain if I've messed it up.

I would have assumed that the CSS pixels for screen 1 would be (100, 0)-(150-50). That is, I assume that the desktop coords of screen 1 don't get scaled with its own pixel ratio in order to get the css pixels, I'd assume we use the scale factors for the "earlier" screens for that.

A window can extend beyond a screen, though; if a window mostly on screen 1 extends off to the left, that scheme wouldn't be able to address the part extending beyond its "home" screen (or it would be indistinguishable from screen 0).

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to :Gijs (he/him) from comment #10)

The comments here https://bugzilla.mozilla.org/show_bug.cgi?id=1474783#c17 are on point. The problem is the calculations here pass CSS pixel screen coordinates into APIs that expect desktop/screen/device ones.

Note that desktop pixels is identical to device pixels on Windows 10 (and probably on Linux) while it is identical to CSS pixels on Windows 7 (and probably on macOS).

Comment 17

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Masatoshi Kimura [:emk] from comment #16)

Note that desktop pixels is identical to device pixels on Windows 10 (and probably on Linux) while it is identical to CSS pixels on Windows 7 (and probably on macOS).

Good point. I think dividing by screen.defaultCSSScaleFactor gets around this, to scale from device pixels to CSS pixels for the screen (bypassing desktop pixels).

But I realized there's still more potential confusion in _calculateMaxHeight if the anchor is scaled differently from the screen that its rect is mostly on: cssAvailTop and cssAvailHeight are un-scaled by the default factor for the rect's screen (so they're CSS pixels for a window on that screen, where the popup will go), but they're used with anchor.screenY and anchorRect.height, which is part of a window that could be on a different screen. I don't think it'll look good across scalings, though.

It'd probably be best to do all these calculations in device pixels, then convert using window.devicePixelRatio at the end.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch for 70 but since this is triaged and set to P3 priority I'm setting it as fix-optional.
That will remove the bug from weekly regression triage.

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #17)

It'd probably be best to do all these calculations in device pixels, then convert using window.devicePixelRatio at the end.

The problem is, I don't think there are JS-accessible APIs to get the device pixels for all this work...

Comment 22

[:Gijs (out for now; he/him)]

I believe this should be fixed in Nightly as a result of the work in bug 1746774, bug 1753836 and friends. Could you confirm if this works correctly for you on today's Nightly?

Comment 23

[Rémi]

Since I changed my setup (another job), I tried successfully to reproduce on Firefox 94.0.1 (Main monitor 1680x1050 100% top, secondary 1366x768 125% bottom)
Then, with Nightly (102.0a1 2022-05-04 64bits), it works correctly. See screenshot in attachment.

Thank you !

Comment 24

[Rémi]

Attachment: helloworlditworks.png

Comment 25

[:Gijs (out for now; he/him)]

Thanks very much for the quick response! Duping over to the last-most bug to show where/when this got fixed.