861270 - menu behavior is unstable depending on value of devPixelsPerPx

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jonathan Kew [:jfkthame]]

Followup to bug 840881. The patch there intended to make the gtk2 widget backend take account of the devPixelsPerPx pref, if set to a non-default value, and apply the appropriate scaling. However, behavior is erratic depending on the actual value used.

In particular, with some devPixelsPerPx settings, popup menus such as those in the Preferences dialog will continually grow when pressed, to fill the entire extent of the screen; also, the URL bar dropdown grows to the full screen height. I see this with devPixelsPerPx=1.1, for example.

On the other hand, with devPixelsPerPx=1.2, the menus behave fine for me.

I suspect the problem is something along the lines of sizes getting rounded when converted between device and CSS pixels, and then converted back to a value that's off-by-one (for example), so that we think things have changed and do another resize, and the cycle repeats....

Comment 1

[Karl Tomlinson (:karlt)]

Yes, Gecko doesn't distinguish between resize notifications and resize requests, and so a notification will cause a resize if GetBounds doesn't return the same as the Resize parameters.  That seems quite likely to happen if one of those methods uses floating point arithmetic.

Comment 2

[rainmansr]

Follow-up on comment #41 on bug 840881 (sorry I initially wrongly posted there). 

I see three different types of behavior:
1) If I change devPixelsPerPx from a bigger to a smaller value e.g. from 1.3 to 1.1: the URL drop-down starts normal and then slowly resizes to size 0 (the lower edge starts going up at around 1 link per 2 seconds in a nice "animated" way)
2) The reverse of 1) when I change devPixelsPerPx from smaller to a bigger value e.g from 1.1 to 1.3: the drop-down window slowly resizes to take up all available vertical space.
3) The URL drop-down takes up all the vertical space - this happens after 2) once I open the drop-down 5-6 times without changing devPixelsPerPx

Comment 3

[Jonathan Kew [:jfkthame]]

It looks like the real cause of this problem is that nsView::DoResetWidgetBounds uses a scale factor based on the device context's UnscaledAppUnitsPerDevPixel() value to convert from device pixels to global display coordinates and reposition/size the widget. (See http://mxr.mozilla.org/mozilla-central/source/view/src/nsView.cpp#264.) But then the widget uses its GetDefaultScale() value to convert back to device pixels.

In the usual cases we encounter on Windows and OS X, such as 125%, 150%, and 200%, this is fine, because the scale factors can be exactly expressed via the adjustment to the appUnit value. But the (rounded) appUnit factor that results from a scale of 1.1 or 1.3 will mean that the inverse-scale used in DoResetWidgetBounds does not quite match the scale used in the widget's Move and Resize methods.

We may be able to fix this by simply using mWindow->GetDefaultScale() as the basis for the scaling in DoResetWidgetBounds. AFAICT, this gives correct, stable behavior on Linux for a variety of scale factors.

My one concern is whether there will be certain cases on OS X multi-screen configurations, where GetDefaultScale may vary as the window moves between screens, where this will lead to problems. So before proposing this for review/landing, I need to do some testing there.

Meanwhile, I've started a tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=b20892f70fd8. Once this is available, any testing and feedback would be welcome.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: prefer to use precise scale factor from GetDefaultScale() rather than approximate factor based on appUnits per devpix

This appears to fix the erratic behavior on Linux with arbitrary devPixelsPerPx settings, while avoiding any change to behavior on OS X. (Things are still less-than-perfect for the edge-case of popups in the smaller part of a window that straddles Retina and non-Retina displays - see bug 802336 - but that's a separate issue.)

Comment 5

[Carl Thompson]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

> ...

> Meanwhile, I've started a tryserver build:
> https://tbpl.mozilla.org/?tree=Try&rev=b20892f70fd8. Once this is available,
> any testing and feedback would be welcome.

Sorry for the silly question, but how do we help test the tryserver builds? I don't see a binary tarball anywhere... Would I need to build from source myself to test this? Thanks!

Comment 6

[Jonathan Kew [:jfkthame]]

If you click the green "B" for a successful Build job in tbpl, there'll be a link at the bottom left that takes you to the "build directory", where you'll find the various logs of the build process, etc., and also a .tar.bz2 that contains the actual build (in the case of Linux). You should be able to download and uncompress that, and run the version of firefox it contains (in the dist/bin directory within the archive).

Comment 7

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0d7f9cb43d

Comment 8

[Carl Thompson]

Thanks! Your try build appears to resolve all remaining issues for me! Everything looks perfect so far.

Comment 9

[Jonathan Kew [:jfkthame]]

Great - thanks for testing!

If you could reconfirm with the Nightly build once that comes out (probably tomorrow's build, unless there are delays getting the patch merged from inbound), that would be awesome. Then, once it's safely in Nightly, I'll nominate for uplift to earlier FF releases as well.

Comment 10

[Carl Thompson]

Will test nightly tomorrow. Thanks again!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c0d7f9cb43d

Comment 12

[Jonathan Kew [:jfkthame]]

This is essentially "bug 840881 part 2", hence nominating for the same tracking and uplift. This only affects users who are using a custom layout.css.devPixelsPerPx value (usually to account for a high-pixel-density display), but for them it can be a pretty crippling usability issue.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 739035 [details] [diff] [review]
prefer to use precise scale factor from GetDefaultScale() rather than approximate factor based on appUnits per devpix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): hi-dpi support

User impact if declined: many custom devPixelsPerPx values are essentially unusable due to erratic menu behavior (primarily on Linux/Gtk - can also affect Windows, but such custom values are rarely used there)

Testing completed (on m-c, etc.): fix confirmed using tryserver build (comment 8); now landed on mozilla-central

Risk to taking this patch (and alternatives if risky): minimal risk - just uses a more accurate scale value to avoid widget sizing discrepancy

String or IDL/UUID changes made by this patch: none

Comment 14

[Carl Thompson]

Nightly from Apr 19 also appears to work perfectly for me.

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 739035 [details] [diff] [review]
prefer to use precise scale factor from GetDefaultScale() rather than approximate factor based on appUnits per devpix

Approving in favor of completely resolving the blocker https://bugzilla.mozilla.org/show_bug.cgi?id=840881#c17

Please land by Monday evening PT on the beta branch, in preparation for this to go into FX21 beta 4 .

Comment 16

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/27174ab64bfc
https://hg.mozilla.org/releases/mozilla-beta/rev/aa1b0882ae31

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on FF 21 beta 4 (build ID: 20130423212553) using a non hi-dpi display.
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0

Can someone please verify if this is also fixed on a hi-dpi display?

ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/21.0b4-candidates/build1/

Comment 18

[u279076]

Thanks Cornel for testing. 

Carl, rainmansr, do either of you have this problem with the latest Firefox Beta candidates?

Comment 19

[Carl Thompson]

The beta fixes the menu issues, but the related bug 861112 fixes are not there.

Thanks!

Comment 20

[u279076]

(In reply to Carl Thompson from comment #19)
> The beta fixes the menu issues

Thanks Carl, if you have a chance could you also test the latest Aurora and Nightly builds?

> the related bug 861112 fixes are not there.

Yup, that's expected. The fix for bug 861112 is still being evaluated for uplift to Firefox 21 and 22 (it's only landed in Firefox 23 so far).

Comment 21

[Carl Thompson]

I'm currently running the nightly (23.0a1) from Apr 19. That has everything fixed so I assume the subsequent ones are also fixed.

Comment 22

[u279076]

(In reply to Carl Thompson from comment #21)
> I'm currently running the nightly (23.0a1) from Apr 19. That has everything
> fixed so I assume the subsequent ones are also fixed.

Yup, this landed in mozilla-central on April 18 so that would be correct. Would you be able to quickly check the latest Aurora build from aurora.mozilla.org?

Comment 23

[Carl Thompson]

Wouldn't it be better for me to wait for the fixes from bug 861112 to land so I can just test everything at once? Thanks.

Comment 24

[u279076]

(In reply to Carl Thompson from comment #23)
> Wouldn't it be better for me to wait for the fixes from bug 861112 to land
> so I can just test everything at once? Thanks.

I think that's reasonable enough, thanks!

Comment 25

[Paul Silaghi, QA [:pauly]]

(In reply to Carl Thompson from comment #23)
> Wouldn't it be better for me to wait for the fixes from bug 861112 to land
> so I can just test everything at once? Thanks.
bug 861112 is done. Carl, if you have any time, please give it a quick check on latest aurora 22. Thanks

Comment 26

[Cornel Ionce [:noni] [Hubs QA]]

I have verified this as fixed on FF 22 beta 2 (build ID: 20130521223249) with a non hi-dpi display.
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0

Can someone also verify this on a hi-dpi display?

Comment 27

[Carl Thompson]

Works great in lastest Aurora (23a2) and latest beta (22b2) on Linux hidpi (2880x1800, 15inch) display.

Many thanks to all involved!

Comment 28

[Cornel Ionce [:noni] [Hubs QA]]

Thank you Carl!

Marking issue verified fixed.