User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20121208 Firefox/20.0 Build ID: 20121208030937 Steps to reproduce: I opened a context menu or drop-down menu. When I did this, only the top-left region of the menu was shown. Also, it did not appear below the cursor. Note that I am using a Windows 8 PC with the DPI on the desktop set to 150%. I also have the pref layout.css.devPixelsPerPx. This started in today's nightly build (2012-12-08).

Regression range from: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/12/2012-12-07-03-07-41-mozilla-central/firefox-20.0a1.en-US.win32.txt to: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/12/2012-12-08-03-09-37-mozilla-central/firefox-20.0a1.en-US.win32.txt is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=739f20de3c1e&tochange=b7fab17d30b7 I don't see any obvious causes in that range.

Actually, https://hg.mozilla.org/mozilla-central/rev/ac48e5c365e2 seems like a likely possible cause.

(In reply to Josh Tumath from comment #0) > I also have the pref layout.css.devPixelsPerPx. Sorry, I meant to say "I also have the pref layout.css.devPixelsPerPx set to -1."

(In reply to David Baron [:dbaron] from comment #2) > Actually, https://hg.mozilla.org/mozilla-central/rev/ac48e5c365e2 seems like > a likely possible cause. Yup, this will no doubt be a result of that... (In reply to Josh Tumath from comment #3) > (In reply to Josh Tumath from comment #0) > > I also have the pref layout.css.devPixelsPerPx. > > Sorry, I meant to say "I also have the pref layout.css.devPixelsPerPx set to > -1." I don't think that's currently expected to work reliably on Windows. If you reset layout.css.devPixelsPerPx to the default value (1.0), do the context menus display correctly?

(In reply to Jonathan Kew (:jfkthame) from comment #4) > I don't think that's currently expected to work reliably on Windows. If you > reset layout.css.devPixelsPerPx to the default value (1.0), do the context > menus display correctly? Yes. I know it's not stable, but I need it. Firefox is barely usable, otherwise.

If you explicitly set devPixelsPerPx to a (larger) positive value, such as 1.5 or even 2.0, does it behave any differently?

(In reply to Jonathan Kew (:jfkthame) from comment #6) > If you explicitly set devPixelsPerPx to a (larger) positive value, such as > 1.5 or even 2.0, does it behave any differently? No. When setting it to 1.5, it behaves the same. It acts, in some ways, as if the scale is still 1.0.

OK, I can reproduce this problem on Win7 as well, by setting the "Text Size" to 150% instead of the default 100%. So it's not Win8-specific (thankfully), it's a problem with handling the Windows UI scaling factor. The default setting of layout.css.devPixelsPerPx = 1.0 overrides most aspects of that scaling, and hence masks the problem here, but using a non-1.0 factor there (including -1, which means "automatic") will trigger it.

There's a test build at http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/jkew@mozilla.com-41e2b4696e17/try-win32/ that includes a possible fix for this bug. I'm not attaching the patch here for review just yet, as it's a rather quick-n-dirty hack that wants some tidying up first, but if you're able to give the test build a try and confirm whether it fixes the problem, that would be great - thanks!

(In reply to Jonathan Kew (:jfkthame) from comment #9) > I'm not attaching the patch here for review just yet, as it's a rather > quick-n-dirty hack that wants some tidying up first, but if you're able to > give the test build a try and confirm whether it fixes the problem, that > would be great - thanks! Yes, it does. :)

The problem here is that in bug 814434, we switched the Move and Resize methods for top-level widgets (i.e. windows) to take "global display pixel" coordinates instead of window/screen-specific device pixels, because the latter may be ambiguous in multi-screen, mixed-resolution scenarios. However, within the Windows implementations of those methods, we need to apply the window's DefaultScale value to convert back to the appropriate device pixel values for Win32 APIs. Failing to do that (when the UI scale factor is not 100%) is giving us the mispositioned, mis-sized popups here. To resolve this, I propose to introduce "clones" of the nsIWidget Move and Resize APIs that take floating-point coordinates instead of integers. This is because otherwise, we'll be doing two successive potentially-lossy rounding operations as we go from the desired (device-pixel) size of the window's content to display pixels (e.g. at 150% scaling), and then back to device pixels. By allowing the widget Move and Resize operations to take floating-point coordinates, we can defer rounding until we've applied the DefaultScale, so that we don't accumulate errors. Without this, we'd risk ending up with window dimensions that differ by a pixel (or perhaps two) from their intended size. The default implementation of the floating-point methods will be to simply round the parameters and call the existing integer methods, so any platform that doesn't support varying default scale (or where the scale factor is always an integer, as is currently the case on OS X) need not do anything special here; but a platform with arbitrary scaling can override the floating-point method to do whatever it needs by way of coordinate scaling -before- any rounding happens. Only the "top-level" widget manipulation calls in nsXULWindow and nsView will be changed to pass floating-point coordinates (as they're converting device to display pixels with potentially non-integral scale factors); all manipulation of child widgets within a window can continue to use integer device-pixel coordinates, as at present. This is why I'm introducing parallel floating-point methods for the functions needed here, rather than proposing that we convert all the existing nsWidget APIs over to floating point.

Part 1 - this introduces the floating-point variants of nsIWidget Move and Resize methods, and converts nsXULWindow and nsView to use them. This should have no net effect on behavior; it just avoids rounding the coordinates until after they've been passed in to the widget code, in preparation for the following patch.

Part 2 - and this actually fixes the present bug, by applying the appropriate scaling to the display-pixel coordinates passed in to Move and Resize methods.

Tryserver run with these patches: https://tbpl.mozilla.org/?tree=Try&rev=e8d40ce50da4 and after fixing the windows build failure: https://tbpl.mozilla.org/?tree=Try&rev=10ab96b43ec9

Comment on attachment 690485 [details] [diff] [review] pt 1 - add floating-point versions of widget move & resize methods, to allow passing fractional coordinates without rounding. Review of attachment 690485 [details] [diff] [review]: ----------------------------------------------------------------- rev nsIWidget IID Is there a reason to not simply switch the existing APIs to take float/double?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15) > Is there a reason to not simply switch the existing APIs to take > float/double? I didn't want to force other uses of the Move/Resize methods (for child widgets within the window) to go through integer/float/integer conversion; that seemed like avoidable overhead.

With new nsIWidget IID.

(In reply to Jonathan Kew (:jfkthame) from comment #16) > I didn't want to force other uses of the Move/Resize methods (for child > widgets within the window) to go through integer/float/integer conversion; > that seemed like avoidable overhead. I don't think this overhead is significant; we don't manipulate widgets all that often. The API surface is a far more important consideration IMHO.

OK, here's a version that just switches the existing Move/Resize methods from int32_t to double coords, so that we can pass fractional-pixel values where required. This means changing all the widget subclasses that implement these methods; I've generally kept the changes to a minimum, so internally the floating-point parameters just end up getting assigned to integer fields (e.g. in mBounds). For existing uses that are always passing integers anyway, the result should be no change, but now we'll have the option of passing and maintaining fractional-pixel values further in to the widget code where that's actually needed.

This just removes the rounding from nsView and nsXULWindow where they convert device to display pixels in order to pass them to widget APIs. Where the scale factor is 1.0, this won't make any difference; but for non-100% scaling on Windows, it will allow the widget code to maintain more accurate dimensions.

Actually implement the proper scaling for non-100% scale factors on Windows - just rebased for the updated version of part 1, carrying forward r=roc.

Tryserver run in progress with these patches: https://tbpl.mozilla.org/?tree=Try&rev=2decee2c341f

Comment on attachment 690807 [details] [diff] [review] pt 1 - make widget move & resize methods take floating-point parameters, to allow passing fractional coordinates without rounding. Review of attachment 690807 [details] [diff] [review]: ----------------------------------------------------------------- I think you should introduce explicit rounding (NSToIntRound) instead of truncating coercion. Otherwise, this patch, combined with patch 1.1, changes behavior, does it not?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > Comment on attachment 690807 [details] [diff] [review] > pt 1 - make widget move & resize methods take floating-point parameters, to > allow passing fractional coordinates without rounding. > > Review of attachment 690807 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think you should introduce explicit rounding (NSToIntRound) instead of > truncating coercion. Otherwise, this patch, combined with patch 1.1, changes > behavior, does it not? I'm not sure it actually matters right now, because apart from the non-100% Windows case, which we're about to fix properly in patch 2, I think we're normally calling these APIs with integer coordinates, aren't we? (Except for the HiDPI Mac case, but there we already have the necessary scaling and rounding in the widget code.) But in principle I agree it's the right thing to do, so fine, let's include it here.

This includes rounding wherever widget code receives the newly-floating parameters and stores them (or passes them along) as integers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5cd9651e99a https://hg.mozilla.org/integration/mozilla-inbound/rev/b224adb34456 https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6a558cbebe

https://hg.mozilla.org/mozilla-central/rev/d5cd9651e99a https://hg.mozilla.org/mozilla-central/rev/b224adb34456 https://hg.mozilla.org/mozilla-central/rev/5e6a558cbebe

This has caused a regression with embedded plug-ins. They scroll quicker than the page and float outside of the box where they should be positioned in the page. Should I file a new bug?

Yikes - yes please, a new bug with a specific testcase/URL would be great. Thanks.

I wonder if this fix broke the non-Windows case? On my HiDPI Linux laptop (2880x1800, 220dpi) since updating to Firefox 20 I have the same problems as described here when layout.css.devPixelsPerPx is not set to 1. Normally, I set it to 1.5 because at 1 the sizes of various elements on the page don't match properly if I don't change it. Pop up menus have the problem as described here where they only show a small part of the menu and they pop up far away from the mouse cursor. It makes Firefox unusable for me. Whatever OpenSUSE calls Firefox 20.98 is also broken. Firefox 19 worked fine.

Screenshot showing problem on HiDPI Linux desktop after update to Firefox 20. The context menu shown is not shown completely and it opened far away from the mouse cursor.

That sounds like bug 840881. The default value for layout.css.devPixelsPerPx should be -1.0, isn't it? Are there "elements...[that] don't match properly" in this case? (A separate bug report on that would be best.)

Yes, this appears to be bug 840881. I know that the default for layout.css.devPixelsPerPx is -1. But things should still work properly if I choose not to use the default! Especially since it used to work. I'm just wondering if the fix for this very similar bug for Windows is what broke the non-Windows case which used to work.