1240749 - Popup windows are sometimes opened on the wrong screen

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Marco Castelluccio [:marco]]

I haven't tried to find a regression range, but this could have the same cause as bug 1239855.

Popup windows are sometimes opened on the wrong screen, in a multi-monitor setting. For example, if I right click near to the right edge of my primary screen (so that the popup window doesn't have enough room on the right of the cursor), the popup window is opened on the secondary screen.

Comment 1

[Marco Castelluccio [:marco]]

I've skipped the builds where the popup was always in a wrong position (bug 1239855).
This is the regression range that I found using mozregression: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b706331377c59bd1f3101f4bfdc81618b1e3dfe9&tochange=f4bd1fc36f919836ad9b8ee5cb8b89a16aa4968c

Comment 2

[Marco Castelluccio [:marco]]

Basically this was working correctly before bug 890156, it's untestable between the merging of bug 890156 and bug 1239855, and it is broken after the merging of bug 1239855.
So, I think the issue has been introduced with bug 890156.

Comment 4

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7124cbe3da79

Comment 5

[Jonathan Kew [:jfkthame]]

:marco, I have a patch that seems to make things a lot better in my local testing, at least; see tryserver run in comment 4. If you could try this build when it's ready, and confirm whether it fixes this for you, that would be really helpful - thanks.

Comment 6

[Marco Castelluccio [:marco]]

This seems to fix both this and bug 1248427!

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Fix up screen DPI support in Gtk widget interface

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8723684 [details] [diff] [review]
Fix up screen DPI support in Gtk widget interface

I'm not sure I have enough experience about Gtk. Karlt, could you look at this?

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8723684 [details] [diff] [review]
Fix up screen DPI support in Gtk widget interface

> Bug 1240749 - Fix up screen DPI support in Gtk widget interface.

Please explain in the commit message why this fix up is necessary.
If another change has made this necessary, then please indicate.

>+NS_IMETHODIMP
>+nsScreenGtk::GetDefaultCSSScaleFactor(double* aScaleFactor)
>+{
>+  *aScaleFactor = GetGtkMonitorScaleFactor() * gfxPlatformGtk::GetDPIScale();
>+  return NS_OK;
>+}

Please share code with GetDPIScale().
Why is nsIWidget::DefaultScaleOverride() not involved here?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to Karl Tomlinson (ni?:karlt) from comment #9)
> Why is nsIWidget::DefaultScaleOverride() not involved here?

I wasn't sure it was appropriate/necessary to incorporate the potential override here, but further testing confirms that it is the right thing to do -- without it, desktop notifications are misplaced with non-default devPixelsPerPx values, for example.

So that means GetDefaultCSSScaleFactor() can simply use GetDPIScale() directly here. I'll update the patch accordingly.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Fixes for DPI support in Gtk widget interface: remove incorrect Get[Avail]RectDisplayPix overrides, as desktop pixels == device pixels for the Gtk widget backend, and implement nsScreenGtk::GetDefaultCSSScaleFactor, required by nsGlobalWindow since per-mo

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc33885d20340641e06cc40031618dd30ccd6567
Bug 1240749 - Fixes for DPI support in Gtk widget interface: remove incorrect Get[Avail]RectDisplayPix overrides, as desktop pixels == device pixels for the Gtk widget backend, and implement nsScreenGtk::GetDefaultCSSScaleFactor, required by nsGlobalWindow since per-monitor DPI patches in bug 890156. r=karlt

Comment 13

[Carsten Book [:Tomcat]]

backed for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=22610651&repo=mozilla-inbound 

07:33:25 INFO - /home/worker/workspace/build/src/widget/gtk/nsScreenGtk.h:34:14: error: 'GetRect' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] 
 07:33:25 INFO - /home/worker/workspace/build/src/widget/gtk/nsScreenGtk.h:35:14: error: 'GetAvailRect' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] 
 07:33:25 INFO - /home/worker/workspace/build/src/widget/gtk/nsScreenGtk.h:36:14: error: 'GetPixelDepth' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a76dfcce45

Comment 15

[Jonathan Kew [:jfkthame]]

Ah..... so including the 'override' annotation for a newly-added override means that we have to add it for all the existing ones as well; it's all-or-nothing.

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea315d62656857d0e16cfde598ba14028813f2fc
Bug 1240749 - Fixes for DPI support in Gtk widget interface: remove incorrect Get[Avail]RectDisplayPix overrides, as desktop pixels == device pixels for the Gtk widget backend, and implement nsScreenGtk::GetDefaultCSSScaleFactor, required by nsGlobalWindow since per-monitor DPI patches in bug 890156. r=karlt

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea315d626568

Comment 18

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

Karl, Jonathan: Are you ok with us shipping 46 with this issue in gtk3? Or, might it be safe to uplift for beta 11 tomorrow?

Comment 19

[Karl Tomlinson (:karlt)]

I think what caused the regression was backed out of 46, but I'll let Jonathan confirm by marking 46 unaffected.

Comment 20

[Jonathan Kew [:jfkthame]]

Right -- we backed out the offending patches (in bug 890156 comment 168), so 46 is no longer affected here.