1097897 - popup menus appear on wrong monitor when devPixelsPerPx is not unit

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

When a browser window is on the monitor logically below another and
layout.css.devPixelsPerPx is 2.0, popups appear on the top monitor.

Menubar, location bar, context menu, are doorhangers affected.
comboboxes dropdowns are not affected.

Popups get shorter if the window is further down the lower monitor, and
sometimes end up looking like attachment 713295 [details], but on the other monitor, or
disappearing entirely.  With a value of 1.5, menus start working as expected
again once far enough down the screen.

Noticed in 2012.  Bug 712898 comment 20.

Comment 1

[Karl Tomlinson (:karlt)]

This may be related to nsIScreenManager::ScreenForRect().

Contrary to documentation, nsIScreenManager::ScreenForRect() is implemented
assuming device-independent Gecko screen coordinates, not device pixels on
other platforms, but is assuming device pixels in the GTK port.

Also nsScreenGtk doesn't yet implement the DisplayPix versions.

nsWindow::ConstrainPosition() hasn't been updated to take in account the
difference between Gecko device-independent "display" pixels and Gecko screen
coords.

ConstrainPosition() should probably use nsIScreen::GetAvailRectDisplayPix().

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch

What about this one? It also contains a fix for Bug 1081142 comment 25. I'm not entirely sure how do you mean the scaling for GTK3. 

Right now the Gtk3 scaling (GDK_SCALE) is applied on top of the DPI scaling. so for instance system DPI 150 and GDK_SCALE = 2 results to 4x scaling factor in GTK3. Is that intended or should be those two settings concurrent?

Comment 3

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8558497 [details] [diff] [review]
patch

Note: Most of the code is derived from widget/windows because it seems to use a similar logic.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch v.2

Sorry, an updated one without debugging code.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8558499 [details] [diff] [review]
patch v.2

>+double
>+gfxPlatformGtk::GetDPIScale()
>+{
>+    // We want to set the default CSS to device pixel ratio as the
>+    // closest _integer_ multiple, so round the ratio of actual dpi
>+    // to CSS dpi (96)
>+    return (sDPI > 96) ? round(sDPI/96.0) : 1.0;
>+}

Call GetDPI() instead of using sDPI directly, as sDPI may not yet be
initialized.

>+NS_IMETHODIMP
>+nsScreenGtk :: GetRectDisplayPix(int32_t *outLeft,  int32_t *outTop, int32_t *outWidth, int32_t *outHeight)
>+{
>+  int32_t left, top, width, height;
>+
>+  nsresult rv = GetRect(&left, &top, &width, &height);
>+  if (NS_FAILED(rv)) {
>+    return rv;
>+  }

GetRect() and GetAvailRect() don't fail, so either ignore the return code, or
use DebugOnly<nsresult> rv and assert NS_SUCCEEDED(rv) if you'd like to guard
against changes to GetRect().

>+
>+  double scaleFactor = 1.0 / gfxPlatformGtk::GetDPIScale();
>+  *outLeft = NSToIntRound(left * scaleFactor);
>+  *outTop = NSToIntRound(top * scaleFactor);
>+  *outWidth = NSToIntRound(width * scaleFactor);
>+  *outHeight = NSToIntRound(height * scaleFactor);

DefaultScaleOverride() should be considered here too.
That could be here, like nsScreenManagerWin::ScreenForRect(), but putting it
in GetDPIScale() is probably better as then the logic is all in one place.

>+nsScreenManagerGtk :: ScreenForRect( int32_t aX, int32_t aY,
>+                                     int32_t aWidth, int32_t aHeight,
>+                                     nsIScreen **aOutScreen )
>+{
>+  uint32_t scale = gfxPlatformGtk::GetDPIScale();
>+  return ScreenForRectPix(aX*scale, aY*scale, aWidth*scale, aHeight*scale,
>+                          aOutScreen);
>+}

DefaultScaleOverride() should be considered here too.
Having it in GetDPIScale() would address this case too.

>+//
>+// ScreenForRectPix
>+//
>+// Returns the screen that contains the rectangle. If the rect overlaps
>+// multiple screens, it picks the screen with the greatest area of intersection.
>+//
>+// The coordinates are in Gdk pixels (not app units) and in screen coordinates.

The intention is actually to make these consistently device (X11) pixels.  See
bug 1126094 and bug 975919 comment 15.

> NS_IMETHODIMP
> nsWindow::ConstrainPosition(bool aAllowSlop, int32_t *aX, int32_t *aY)

>+    double dpiScale = GetDefaultScale().scale;
>+  
>+    // we need to use the window size in logical screen pixels
>+    int32_t logWidth = std::max<int32_t>(NSToIntRound(mBounds.width / dpiScale), 1);
>+    int32_t logHeight = std::max<int32_t>(NSToIntRound(mBounds.height / dpiScale), 1);  

No need for the int32_t cast, as NSToIntRound returns int32_t.

>+    /* get our playing field. use the current screen, or failing that
>+      for any reason, use device caps for the default screen. */
>+    nsIntRect screenRect;
>+    
>+    nsCOMPtr<nsIScreenManager> screenmgr = do_GetService("@mozilla.org/gfx/screenmanager;1");
>+    if (screenmgr) {
>+        nsCOMPtr<nsIScreen> screen;
>+        screenmgr->ScreenForRect(*aX, *aY, logWidth, logHeight,
>+                                 getter_AddRefs(screen));
>+        if (screen) {
>+            if (mSizeMode != nsSizeMode_Fullscreen) {
>+              // For normalized windows, use the desktop work area.
>+              screen->GetAvailRectDisplayPix(&screenRect.x, &screenRect.y,
>+                                             &screenRect.width, &screenRect.height);
>+            } else {
>+              // For full screen windows, use the desktop.
>+              screen->GetRectDisplayPix(&screenRect.x, &screenRect.y,
>+                                        &screenRect.width, &screenRect.height);
>+            }
>         }

>+    } else { 
>+        screenRect.x = screenRect.y = 0;
>+        screenRect.width = NSToIntRound(GdkCoordToDevicePixels(gdk_screen_width()) / dpiScale);
>+        screenRect.height = NSToIntRound(GdkCoordToDevicePixels(gdk_screen_height()) / dpiScale);
>+    }

I don't think we need this block to handle null screenmgr.
If XPCOM services were not available, then I don't think we would get a useful
window.
Just avoid crashing with the null check screenmgr and use the empty rect, as
for a null |screen|, or check for a null screen and return NS_OK early.

>+  
>+    if (aAllowSlop) {
>+      if (*aX < screenRect.x - logWidth + kWindowPositionSlop)
>+          *aX = screenRect.x - logWidth + kWindowPositionSlop;
>+      else if (*aX >= screenRect.x + screenRect.width - kWindowPositionSlop)
>+          *aX = screenRect.x + screenRect.width - kWindowPositionSlop;

Use screenRect.XMost() instead of .x + .width.  Similarly below.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #2)
> Right now the Gtk3 scaling (GDK_SCALE) is applied on top of the DPI scaling.
> so for instance system DPI 150 and GDK_SCALE = 2 results to 4x scaling
> factor in GTK3. Is that intended or should be those two settings concurrent?

Yes, that is what I meant.  If the system reports DPI=192 and GDK_SCALE=2,
then GDK will adjust the DPI by the scale and report 96.  Gecko usually works
in device pixels, so we undo GDK's adjustment.

I don't see that behavior in the patch here, but GTK3 can be addressed
separately.  The patch is the right kind of thing for GTK2.  For GTK3, it
always uses the same scale on all monitors, with X11, so GetDPIScale() can
include the scale factor from monitor 0.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: scale v.3

Thanks, this one should address that.

Try build: https://tbpl.mozilla.org/?tree=Try&rev=3151c272853d

> DefaultScaleOverride() should be considered here too.
> That could be here, like nsScreenManagerWin::ScreenForRect(), but putting it
> in GetDPIScale() is probably better as then the logic is all in one place.

Added as a static member of nsScreenGtk.

> >+//
> >+// ScreenForRectPix
> >+//
> >+// Returns the screen that contains the rectangle. If the rect overlaps
> >+// multiple screens, it picks the screen with the greatest area of intersection.
> >+//
> >+// The coordinates are in Gdk pixels (not app units) and in screen coordinates.
> 
> The intention is actually to make these consistently device (X11) pixels. 
> See
> bug 1126094 and bug 975919 comment 15.

Updated the comment. It comes true when Bug 1126094 is checked in, right?

Comment 8

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> (In reply to Martin Stránský from comment #2)
> > Right now the Gtk3 scaling (GDK_SCALE) is applied on top of the DPI scaling.
> > so for instance system DPI 150 and GDK_SCALE = 2 results to 4x scaling
> > factor in GTK3. Is that intended or should be those two settings concurrent?
> 
> Yes, that is what I meant.  If the system reports DPI=192 and GDK_SCALE=2,
> then GDK will adjust the DPI by the scale and report 96.  Gecko usually works
> in device pixels, so we undo GDK's adjustment.
> 
> I don't see that behavior in the patch here, but GTK3 can be addressed
> separately.  The patch is the right kind of thing for GTK2.  For GTK3, it
> always uses the same scale on all monitors, with X11, so GetDPIScale() can
> include the scale factor from monitor 0.

Yes, I'll address that in another patch.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8561390 [details] [diff] [review]
scale v.3

>+#include "prdtoa.h"

Please remove this, now that it is not used.

(In reply to Martin Stránský from comment #7)
> > >+// ScreenForRectPix
> > >+//
> > >+// Returns the screen that contains the rectangle. If the rect overlaps
> > >+// multiple screens, it picks the screen with the greatest area of intersection.
> > >+//
> > >+// The coordinates are in Gdk pixels (not app units) and in screen coordinates.
> > 
> > The intention is actually to make these consistently device (X11) pixels. 
> > See
> > bug 1126094 and bug 975919 comment 15.
> 
> Updated the comment. It comes true when Bug 1126094 is checked in, right?

Yes, GetRect(), as used by ScreenForRectPix() should then return device (X11)
pixels.

ScreenForNativeWidget() will still need some adjustment for GDK scaling, but
that's a separate issue and what you have here should be good for GTK2.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: xscale v.3 for check-in

Patch for check-in. Try: https://tbpl.mozilla.org/?tree=Try&rev=8def22a4c2f1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc6332c88bc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> (In reply to Martin Stránský from comment #2)
> > Right now the Gtk3 scaling (GDK_SCALE) is applied on top of the DPI scaling.
> > so for instance system DPI 150 and GDK_SCALE = 2 results to 4x scaling
> > factor in GTK3. Is that intended or should be those two settings concurrent?
> 
> Yes, that is what I meant.  If the system reports DPI=192 and GDK_SCALE=2,
> then GDK will adjust the DPI by the scale and report 96.  Gecko usually works
> in device pixels, so we undo GDK's adjustment.
> 
> I don't see that behavior in the patch here, but GTK3 can be addressed
> separately.  The patch is the right kind of thing for GTK2.  For GTK3, it
> always uses the same scale on all monitors, with X11, so GetDPIScale() can
> include the scale factor from monitor 0.

Filed as Bug 1131978