Closed Bug 861270 Opened 8 years ago Closed 8 years ago
menu behavior is unstable depending on value of dev
Pixels Per Px
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....
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.
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
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.
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.)
Attachment #739035 - Flags: review?(roc)
Attachment #739035 - Flags: review?(roc) → review+
(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!
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).
Target Milestone: --- → mozilla23
Thanks! Your try build appears to resolve all remaining issues for me! Everything looks perfect so far.
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.
Will test nightly tomorrow. Thanks again!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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 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
Nightly from Apr 19 also appears to work perfectly for me.
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 .
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/
Thanks Cornel for testing. Carl, rainmansr, do either of you have this problem with the latest Firefox Beta candidates?
The beta fixes the menu issues, but the related bug 861112 fixes are not there. Thanks!
(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).
I'm currently running the nightly (23.0a1) from Apr 19. That has everything fixed so I assume the subsequent ones are also fixed.
(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?
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.
(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!
(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
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?
Works great in lastest Aurora (23a2) and latest beta (22b2) on Linux hidpi (2880x1800, 15inch) display. Many thanks to all involved!
Thank you Carl! Marking issue verified fixed.
You need to log in before you can comment on or make changes to this bug.