Closed Bug 1474783 Opened 6 years ago Closed 3 years ago

Autoscroll icon shows up on the far side of the primary monitor on when window is on secondary monitor

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
Unspecified
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
98 Branch
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox-esr91 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox95 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox98 --- verified

People

(Reporter: RyanVM, Assigned: emilio)

References

(Regression)

Details

(Keywords: multi-monitors, regression)

I recently got a new machine with an nVidia P2000 graphics card running two 4K monitors, each on their own displayport connection. Whenever I middle-click on a windows on the secondary monitor, the autoscroll icon appears on the right hand side of the primary monitor instead of underneath where I clicked. Using mozregression, I bisected it down to bug 1401477. Any ideas, Gijs?
Flags: needinfo?(gijskruitbosch+bugs)
And for the record, I'm using 200% DPI scaling. Let me know if you need any other diagnostic info.
What's the ordering of the screens? That is, is your secondary monitor to the right or to the left (or top/bottom?) of your primary (per the OS)? What OS is this on?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ryanvm)
I'm on Win10 1803. Primary is on the left, secondary on the right.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #3) > I'm on Win10 1803. Primary is on the left, secondary on the right. Can you run these blobs of code in the browser console: screen // this will output an object with some numeric properties l = {}; t = {}; h = {}; w = {}; s1 = Components.classes["@mozilla.org/gfx/screenmanager;1"].getService(Components.interfaces.nsIScreenManager).screenForRect(10, 10, 1, 1); s1.GetAvailRect(l, t, w, h); console.log([l,t,h,w].map(x => x.value), s1.defaultCSSScaleFactor); // this will output a list of left, top, width, height coordinates (I'd expect the first 2 to be 0, the next 2 to be your screen size) and the CSS scale factor for that screen Then run: l = {}; t = {}; h = {}; w = {}; s2 = Components.classes["@mozilla.org/gfx/screenmanager;1"].getService(Components.interfaces.nsIScreenManager).screenForRect(<width of first screen + left of first screen>, <top of first screen>, 1, 1); s2.GetAvailRect(l, t, w, h); console.log([l,t,h,w].map(x => x.value), s2.defaultCSSScaleFactor); where you replace the <> bits with width/height from the first screen plus some margin, so you get the other screen... This is all a bit messy because essentially, I guess there's something wrong with the math still (or perhaps with our screen APIs) but I'm not sure what. Are other popups (e.g. datetime pickers) positioned correctly?
Flags: needinfo?(ryanvm)
OS: Unspecified → Windows 10
>> screen Screen { availWidth: 1920, availHeight: 1040, width: 1920, height: 1080, colorDepth: 24, pixelDepth: 24, top: 0, left: 0, availTop: 0, availLeft: 0 } >> l = {}; t = {}; h = {}; w = {}; s1 = Components.classes["@mozilla.org/gfx/screenmanager;1"].getService(Components.interfaces.nsIScreenManager).screenForRect(10, 10, 1, 1); s1.GetAvailRect(l, t, w, h); console.log([l,t,h,w].map(x => x.value), s1.defaultCSSScaleFactor); Array(4) [ 0, 0, 2080, 3840 ] 2 >> l = {}; t = {}; h = {}; w = {}; s2 = Components.classes["@mozilla.org/gfx/screenmanager;1"].getService(Components.interfaces.nsIScreenManager).screenForRect(4100, 2080, 1, 1); s2.GetAvailRect(l, t, w, h); console.log([l,t,h,w].map(x => x.value), s2.defaultCSSScaleFactor); Array(4) [ 3840, 0, 2080, 3840 ] 2 So I guess the last run showing 3840 instead of 4100 coincides with the behavior I'm seeing with the autoscroll icon? As far as I can tell, other popups are positioned fine.
Flags: needinfo?(ryanvm)
Reminding myself to get back to this.
Flags: needinfo?(gijskruitbosch+bugs)
Can you try this try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25324bbcd295dd9e26f0f037ec6ea1a27e6d3cb0 It should log to the browser console the coordinates of the screen we think you clicked on, and the coordinates we then try to use to display the popup at.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ryanvm)
Screen coordinates; left: 0, top: 0, w: 3840, h: 2080 Constraints; minX: 16, maxX: 1904, minY: 16, maxY: 1024 Mouse coordinates we got, X: 2204; Y: 314 Opening at, X: 1904; Y: 314
Flags: needinfo?(ryanvm)
So, AFAICT: 1. the screens are set up to resolution of 3840x2160, with 200% "dpi" / zoom factor in Windows. 2. the screen manager reports those coordinates when asked. It also knows there's a scale factor of x2 going on. So in CSS coordinate space, it thinks the coordinates of the first screen are (0,0,1920,1080), with a slightly smaller height reported as available because of the windows taskbar. 3. for a click on the secondary (right) screen, we get an event.screenX coordinate of 2204. That's clearly in the first screen when looking at (0,0,3840,2160) but in the second screen for (0,0,1920,1080). 4. Every instance I can see of JS code using screenManager.screenForRect() passes in event.screenX/screenY without conversion. 5. the autoscroll code gets the event screenX/screenY over the message manager (sender: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/toolkit/modules/AutoScrollController.jsm#194-199 ; receiver: https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/toolkit/content/widgets/browser.xml#934-938 ) and then uses that to call screenForRect() ( https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/toolkit/content/widgets/browser.xml#1147 ). Yet it then gets the primary screen, instead of the secondary (as it should). It looks to me like the screen coordinates need to be multiplied by the scale factor, at least on Windows, before being passed to screenForRect - but in that case all the other callsites also look wrong to me (e.g. https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1032 , https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/browser/base/content/tabbrowser.xml#1493 , etc.). Jonathan, am I missing something obvious? Do you know if this works differently on other OSes?
Flags: needinfo?(jfkthame)
Hey Neil (or timdream), can you please triage this ASAP?
Flags: needinfo?(timdream)
Flags: needinfo?(enndeakin)
Gjis, are you the right person to work on this?
Flags: needinfo?(timdream)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11) > Gjis, are you the right person to work on this? Not without input from someone who understands layout. See comment #9. I don't know how important this is because I dunno how many people use autoscroll and how many people have multiple screens (perhaps, with non-1.0-dpi).
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(timdream)
Perhaps triage this to P5 as a minor regression then.
Flags: needinfo?(timdream)
Priority: -- → P5
Too late to fix in 63. We could still take a patch for 65 and potentially for 64.
status-firefox63: affected → wontfix
status-firefox64: --- → fix-optional
Adam, I see that you're active in some other windows scaling bugs. Any chance you could have a look at this one and let us know what part of the math this code is getting wrong?
Flags: needinfo?(agashlin)

I've looked at this a bit on Windows 10 with two monitors differently-scaled. Regarding the other places you cited in comment 9, Gijs:

  • The "all tabs" dropdown list is size limited incorrectly on parts of a second monitor in a similar way (due to PanelMultiView's _calculateMaxHeight getting the wrong screen, I think). So that's also wrong in using screenX, screenY as input to screenForRect(). The coordinates passed to that are supposed to be desktop pixels, not CSS pixels.
  • I'm not sure about the dragend handler in tabbrowser-tabs, but I think the screenX, screenY there are desktop pixels, which makes some sense as the destination is somewhere on the desktop. It seems to be consistently getting the correct screen, but what it's doing afterwards to limit the size and position of the window is wrong if the drag started on the second monitor (ending on either). There is some confusion of units; I'm not sure what availX.value = (availX.value - fullX.value) * scaleFactor + fullX.value; is supposed to be doing, does fullX need to be scaled or not?

screenManager.screenForRect(screenX * window.devicePixelRatio, screenY * window.devicePixelRatio, 1, 1) seems to pick the screen properly, but I don't know enough about how dPR maps to device/desktop pixels to conclude that it's the right thing to do. Setting const scaleFactor = window.devicePixelRatio seems to work for the rest, but just using screenX, screenY in openPopupAtScreen() seems to limit the icon to stay on the screen all by itself anyway.

Flags: needinfo?(agashlin)

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

just using screenX, screenY in openPopupAtScreen() seems to limit the icon to stay on the screen all by itself anyway.

Yeah, unfortunately this also resizes the popup (per bug 1401477) which is how we got into this mess in the first place. :-(

I'll try and look at updating all these bits of code to make more sense. Thanks for the explanations.

Flags: needinfo?(jfkthame) → needinfo?(gijskruitbosch+bugs)

This is still lingering around and still annoying to hit on a daily basis :(. GCP, is this something we could get on the OS Integration radar?

Severity: normal → --
status-firefox64: fix-optional → wontfix
status-firefox-esr60: affected → wontfix
Flags: needinfo?(gpascutto)
Priority: P5 → --
No longer blocks: 1401477
Regressed by: 1401477

Maybe! Let's move components, as I understand this is Windows specific.

Component: XUL Widgets → Widget: Win32
Flags: needinfo?(gpascutto)
Product: Toolkit → Core
Severity: -- → S3

Set release status flags based on info from the regressing bug 1401477

status-firefox95: --- → affected
status-firefox96: --- → affected
status-firefox97: --- → affected
status-firefox-esr91: --- → affected
Has Regression Range: --- → yes

Pretty sure this is likely bug 1741830 too.

Confirmed! Thank you very much for fixing this, Emilio!

Assignee: nobody → emilio
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox98: --- → fixed
Depends on: 1741830
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → FIXED
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1741830 →
Target Milestone: --- → 98 Branch
Flags: qe-verify+

I was able to reproduce the issue with an old Firefox Nightly build (2018-07-10) under WIndows 11 with the information provided in Comment 0 on two 4k monitors with 200% DPI.

The issue is fixed on Firefox 98.0 on the same system.

Status: RESOLVED → VERIFIED
status-firefox98: fixed → verified
Flags: qe-verify+
Duplicate of this bug: 1643644
You need to log in before you can comment on or make changes to this bug.