Last Comment Bug 603880 - Gecko doesn't apply system default scale on Windows
: Gecko doesn't apply system default scale on Windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows 7
: -- normal with 7 votes (vote)
: mozilla18
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
: 549919 (view as bug list)
Depends on: 798607
Blocks: 797829 win-hidpi 549919 797828
  Show dependency treegraph
 
Reported: 2010-10-12 23:20 PDT by Alan Thomas
Modified: 2013-12-27 14:36 PST (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
screenshot of bug (572.88 KB, image/png)
2010-10-12 23:21 PDT, Alan Thomas
no flags Details
Fix renderer DPI (3.83 KB, patch)
2011-03-23 18:25 PDT, Dominic Fandrey
no flags Details | Diff | Splinter Review
internet explorer showing webpage when windows dpi value is set to 150% (good behavior in my eyes) (512.74 KB, image/png)
2012-10-02 06:38 PDT, Martin Jürgens
no flags Details
firefox showing webpage when windows dpi value is set to 150% (not what I would expect) (402.71 KB, image/png)
2012-10-02 06:39 PDT, Martin Jürgens
no flags Details
firefox showing webpage when windows dpi value is set to 150% (having set layout.css.devPixelsPerPx to 2) (532.16 KB, image/png)
2012-10-02 06:40 PDT, Martin Jürgens
no flags Details
implement nsWindow::GetDefaultScale (3.27 KB, patch)
2012-10-04 05:36 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
jmathies: review+
Details | Diff | Splinter Review

Description Alan Thomas 2010-10-12 23:20:30 PDT
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre

When Firefox is first loaded (or a clean profile is created) on a high-DPI system (tested under 120 dpi), Firefox does not scale up web content.

Internet Explorer 8 scales up web content by applying a default zoom factor to all web pages.

Reproducible: Always

Steps to Reproduce:
1. Open Firefox under high-DPI system (e.g. set DPI to 250% for a clear demonstration)
2. browse to any page
3. compare to 96 dpi output

Actual Results:  
The two sets of output (96 dpi/high-DPI) are identical.

Expected Results:  
The high-DPI output should be enlarged according to the DPI.

See attached image, taken under Windows 7.
Comment 1 Alan Thomas 2010-10-12 23:21:58 PDT
Created attachment 482785 [details]
screenshot of bug

Sorry if anyone is offended by my use of the Wikipedia web page.
Comment 2 Alan Thomas 2010-10-13 02:06:35 PDT
Just to be clear, this bug is about Firefox's lack of *automatic* ability to adapt to the user's DPI preference on Windows. I know there are prefs you can change in about:config (which may or may not work), but most end users aren't going to do that. They want DPI adjustment to "just work".
Comment 3 Dominic Fandrey 2011-03-23 00:23:44 PDT
I can confirm this regression with X.Org X Server 1.7.7.

> xdpyinfo | grep -i dots
  resolution:    120x120 dots per inch

I have a JavaScript that draws a 100pt square and checks how many pixels it actually turned out. DPI = 100pt / X * 72

Firefox3 returns 120
Firefox4 returns 96

Setting layout.css.dpi has no effect. This is EXTREMELY annoying. I really wonder why there is no storm of complaints.
Comment 4 Dominic Fandrey 2011-03-23 05:26:52 PDT
Use this script to test it:

<html>
	<body>
		<div id="box"style="background: #000000; width: 100pt; height: 100pt;"></div>
		<script type="text/javascript">alert(eval(document.getElementById('box').offsetWidth * 72 / 100) + ' DPI');</script>
	</body>
</html>
Comment 5 Dominic Fandrey 2011-03-23 18:25:50 PDT
Created attachment 521375 [details] [diff] [review]
Fix renderer DPI

gfx/src/thebes/nsThebesDeviceContext.cpp
Issue:
- The PR_MAX macro is used in an unsafe fashion, with function calls as
  parameters. Every function in the call is thus called twice, leading
  to several variables being set to 0 (at least when compiled under
  FreeBSD)
Fix:
- Only forward variables to the PR_MAX macro

layout/style/nsCSSValue.h
layout/style/nsCSSValue.cpp
layout/mathml/nsMathMLFrame.cpp
layout/style/nsRuleNode.cpp
Issue:
- The GetPixelLength() function is hard-coded to 96 DPI
Fix:
layout/style/nsCSSValue.h
- Change signature of GetPixelLength() to take an nsPresContext argument
layout/style/nsCSSValue.cpp
- Use AppUnitsPerPhysicalInch() / AppUnitsPerDevPixel() from the
  context to determine the real DPI as determined by thebes
layout/mathml/nsMathMLFrame.cpp
layout/style/nsRuleNode.cpp
- Update all calls of GetPixelLength() to the new signature
Comment 6 Dominic Fandrey 2011-03-23 18:28:15 PDT
I put a version of above DPI test script online for easier access:
http://www.home.hs-karlsruhe.de/~fado0001/testdpi.html
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-25 03:22:56 PDT
(In reply to comment #5)
> Created attachment 521375 [details] [diff] [review]
> Fix renderer DPI
> 
> gfx/src/thebes/nsThebesDeviceContext.cpp
> Issue:
> - The PR_MAX macro is used in an unsafe fashion, with function calls as
>   parameters. Every function in the call is thus called twice, leading
>   to several variables being set to 0 (at least when compiled under
>   FreeBSD)

I don't understand this. Those function calls should not have side effects. Evaluating "NS_lround(AppUnitsPerCSSPixel() / devPixelsPerCSSPixel)" twice should change nothing, ditto for "NSToIntRound(float(mAppUnitsPerDevNotScaledPixel) / mPixelScale)".

Still, I'd take a patch that #includes "nsAlgorithm.h" and replaces PR_MAX with NS_MAX, which is a proper inline function that avoids the double evaluation. But I'd like that in its own patch, separate from the style system changes. Thanks!!!
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-25 03:27:32 PDT
As for this bug, I agree it's a real bug that we should fix, but it's not really about DPI. I will explain the DPI issues in bug 537890.

For this bug, we should have nsWindow::GetDefaultScale() on Windows return a value other than 1.0 when the user has set the Windows "DPI scale" to something other than 100%. (That Windows DPI setting is really misnamed, it's actually a user-controlled zoom setting.)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-25 03:30:56 PDT
On platforms other than Windows, GetDefaultScale() can similarly be modified to reflect settings where the user has opted into scaling the UI of all applications. As a rule of thumb, if a setting affects the display of the system UI, we can use it in GetDefaultScale() so that Firefox's UI and Web content match the system UI; but if a setting doesn't affect the display of the system UI, we shouldn't use it in GetDefaultScale().
Comment 10 Dominic Fandrey 2011-03-26 15:05:13 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > Created attachment 521375 [details] [diff] [review]
> > Fix renderer DPI
> > 
> > gfx/src/thebes/nsThebesDeviceContext.cpp
> > Issue:
> > - The PR_MAX macro is used in an unsafe fashion, with function calls as
> >   parameters. Every function in the call is thus called twice, leading
> >   to several variables being set to 0 (at least when compiled under
> >   FreeBSD)
> 
> I don't understand this. Those function calls should not have side effects.
> Evaluating "NS_lround(AppUnitsPerCSSPixel() / devPixelsPerCSSPixel)" twice
> should change nothing, ditto for
> "NSToIntRound(float(mAppUnitsPerDevNotScaledPixel) / mPixelScale)".
> 
> Still, I'd take a patch that #includes "nsAlgorithm.h" and replaces PR_MAX with
> NS_MAX, which is a proper inline function that avoids the double evaluation.
> But I'd like that in its own patch, separate from the style system changes.
> Thanks!!!

See bug 645398. Beware, the patch has 3241 lines.
Comment 11 Dominic Fandrey 2011-03-26 15:09:29 PDT
(In reply to comment #9)
> On platforms other than Windows, GetDefaultScale() can similarly be modified to
> reflect settings where the user has opted into scaling the UI of all
> applications. As a rule of thumb, if a setting affects the display of the
> system UI, we can use it in GetDefaultScale() so that Firefox's UI and Web
> content match the system UI; but if a setting doesn't affect the display of the
> system UI, we shouldn't use it in GetDefaultScale().

Just to make sure we don't misunderstand each other, I want 1px to be exactly 1px, as much as I want 1in to be 1in instead of 0.8in.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 16:41:30 PDT
It sounds like you want to break the invariant that 96px == 1in, but I'm afraid we can't do that, because many Web pages rely on it.
Comment 13 Felix Miata 2011-03-27 16:56:55 PDT
How many is many? When exactly did that reliance start? Do you have any prominent examples where the page is otherwise competent?

I have several hundred web pages built several to 10 or more years ago that when built relied on all the most common browsers to use desktop DPI as set, arbitrary or not, instead of an absolute arbitrary DPI. Now the originals are all broken except in old browsers. The handful I've rewritten to size objects in mozmm work as expected in the latest Geckos, as well old Geckos, and old others, but mozmm has no equivalent I'm aware of in any current of Opera, WebKit or IE.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 18:08:54 PDT
(In reply to comment #13)
> How many is many? When exactly did that reliance start? Do you have any
> prominent examples where the page is otherwise competent?

I don't have a list here. A very large fraction of the Web pages that use "pt" as a font-size unit (which is quite commonly done) display poorly or completely brokenly on mobile devices if you break the assumption that 3pt == 4px. I recall that some Amazon pages were on that list. You can probably find more examples in the relevant bugs. (We briefly considered breaking 72pt == 1in but decided that was even worse than the other options.)

Anyway, I don't want to re-argue this here. We had all the arguments, all major browsers have converged on this common solution, and so if you want to have the arguments all over again, please take it to www-style where you can talk to all the browser vendors.
Comment 15 Dominic Fandrey 2011-03-28 01:22:13 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > How many is many? When exactly did that reliance start? Do you have any
> > prominent examples where the page is otherwise competent?
> 
> ...
> 
> Anyway, I don't want to re-argue this here. We had all the arguments, all major
> browsers have converged on this common solution, and so if you want to have the
> arguments all over again, please take it to www-style where you can talk to all
> the browser vendors.

I understand (though disagree with, but I will bring this to www-style as suggested) the arguments. What puzzles me though is the break with ISO 9241-110 (general ergonomic principles). Among other things it demands controllability.

You could just have set the default to 96 and left the user in control of setting layout.css.dpit to 0 (system dpi) or any arbitrary value.

When I read the change was on purpose I was enraged, because something was taken out of my control. I'd thought it was an accident and taken an entire day to find out how to fix, just to learn that I'd been plunged into this state of impotency with full purpose and will have to maintain my patch for evermore and all the world will remain subjugated by your dictate despite my efforts.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-28 02:04:30 PDT
I actually appreciate your passion about this, and I'm sorry that the world isn't the way you want it to be, but your rage should really be directed at the masses of Web authors who have designed Web pages that require 3pt == 4px and 1in == 96px. It is their de-facto dictat, not ours, in force here.

As for controllability: we provide zoom control that scales all units the same way. I'm not aware of ergnonomic issues that would require different units to be scaled independently.
Comment 17 Dominic Fandrey 2011-03-28 04:31:56 PDT
(In reply to comment #16)
> As for controllability: we provide zoom control that scales all units the same
> way. I'm not aware of ergnonomic issues that would require different units to
> be scaled independently.

I don't want images to be misrendered. No matter the quality of your interpolation, it will not match the original image.

Look at deviantart.com for a page that is not quite as pleasant, but still completely readable at 200dpi. And monitors with 200dpi and more will come and I'll be the first to buy them.

At a moderate value like 140 deviantart looks near perfect and at 120dpi there are no issues at all. All without introducing assumptions like 1px = 1.25px.

(In reply to comment #16)
> I actually appreciate your passion about this, and I'm sorry that the world
> isn't the way you want it to be, but your rage should really be directed at the
> masses of Web authors who have designed Web pages that require 3pt == 4px and
> 1in == 96px. It is their de-facto dictat, not ours, in force here.

The difference is that I am free to ignore the mess they created. It's the same as with flash, if a page has flash, I simply move on and assume the page simply isn't meant to be viewed by me.

With all major browser vendors going the same way, I am not given much of a choice apart from abandoning the www entirely. I'd feel quite comfortable on usenet, if it wasn't for all the google mail spam on there. And they reacted by deactivating their abuse@ address, oh so mature.

I'm **** off that someone (or a group of people, apparently) decided to value the content provided by incompetent idiots, over the content provided by those who know the difference between 1pt and 1px (something I had no trouble with when I was 15).

However valid all your arguments, there is no reason not to give me a choice about this.
Comment 18 Notlost 2011-07-02 16:58:37 PDT
Since IE is just adjusting the default zoom factor for the web page, and FF has a zoom option, albeit hidden with the new menu, why can't we jsut set the zoom factor to the windows DPI % by default?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-03 22:34:52 PDT
We can, that is what this bug is about.
Comment 20 Martin Jürgens 2011-12-17 02:54:26 PST
any news on this?
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-05 00:14:39 PST
No.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-05 00:22:36 PST
This actually isn't all that hard to fix. Just need to implement nsIWidget::GetDefaultScale in widget/windows/nsWindow.h,.cpp and have it read the current font DPI setting from the registry, e.g.:
http://www.sevenforums.com/tutorials/443-dpi-display-size-settings-change.html
We should also listen for changes to that registry key. When it changes, treat it like a combination of WM_FONTCHANGE and WM_THEMECHANGED.

My main worry is that the Firefox theme will look crappy because all our icons will be scaled up a bit. To fix that we'd need to update the theme with new artwork for common scale factors and media queries using -moz-device-pixel-ratio to select the right resolution images ... all of which is far more work than this bug.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-05 00:28:54 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> We should also listen for changes to that registry key. When it changes,
> treat it like a combination of WM_FONTCHANGE and WM_THEMECHANGED.

Actually in Windows 7 font dpi changes don't take effect until the user logs out and in again, so this isn't needed.
Comment 24 [Baboo] 2012-06-12 00:36:10 PDT
Blocking bug 378927?
Comment 25 [Baboo] 2012-06-12 00:37:00 PDT
Oh sorry, missed that this here wasn't a core bug.
Comment 26 Martin Jürgens 2012-10-01 10:21:52 PDT
it is really frustrating to still see this bug around.
when internet explorer can do it, firefox should, too.
Comment 27 Greg Edwards 2012-10-01 13:42:45 PDT
Isn't it just a matter of setting the correct layout.css.devPixelsPerPx? I've had mine manually set to 2.0 for the past month and it's working great.
Sure there are bugs, but the bugs are much less significant than those that afflict Firefox without this setting enabled.

In other words, when layout.css.devPixelsPerPx is unset, it should be interpreted as LOGPIXELS / 96.0f, not 1.0f.
Comment 28 Martin Jürgens 2012-10-02 06:36:38 PDT
The results with custom setting layout.css.devPixelsPerPx have never been as good as with Internet Explorer for me. I have made some screenshots, which show the difference.

I take care of some pcs of people who have viewing disabilities. The windows setting is generally set to 150% which works fairly well for all applications (except Firefox and Thunderbird). First they used Mozilla products with a custom DPI setting set in about:config, but as it often broke after updates were installed, and after having seen Internet Explorer and Windows Live Mail, they wanted to switch to those products.

I really believe that how Internet Explorer and Windows Live Mail behave is a good default that probably works for most users and websites and just does what they expect it to do.
Especially because Mozilla is (supposedly) very commited to the accessibility of its product, I can not see why this keeps unsolved for so long, while some (in my eyes) unimportant features are continuously being introduced.
Comment 29 Martin Jürgens 2012-10-02 06:38:08 PDT
Created attachment 666965 [details]
internet explorer showing webpage when windows dpi value is set to 150% (good behavior in my eyes)
Comment 30 Martin Jürgens 2012-10-02 06:39:04 PDT
Created attachment 666966 [details]
firefox showing webpage when windows dpi value is set to 150% (not what I would expect)
Comment 31 Martin Jürgens 2012-10-02 06:40:25 PDT
Created attachment 666968 [details]
firefox showing webpage when windows dpi value is set to 150% (having set layout.css.devPixelsPerPx to 2)
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 05:36:10 PDT
Created attachment 667920 [details] [diff] [review]
implement nsWindow::GetDefaultScale

This implements the basic support for reading LOGPIXELSY in GetDefaultScale, to make the default scale track Windows "font DPI" settings.

But there are some issues, so the patch modifies all.js to avoid using GetDefaultScale by default (ironically). To undo that, set layout.css.devPixelsPerPx to -1.0 in about:config.

So far I've noticed a few issues, most of which I expected in advance:
-- Our bitmap artwork in the Firefox UI looks pretty bad when scaled by 125% (as the default font DPI is set to on my laptop). We can fix this by adding copies of the images for different resolutions and selecting them using media queries.
-- Web content images are all scaled by 125%, and some look a bit fuzzy. Not much we can do about that (except make sure we implement "responsive image" proposals and hope authors use them).
-- Web content HTML <canvas>es are scaled and look a bit fuzzy. We can mostly fix this by using a higher-DPI backing store. We'll want to do that for Retina Macs too.
-- Plugin sizing is broken. This is just a bug.
IE9 has the same issues with Web content images and <canvas>es.

I think we should take this patch so people can work on those issues.
Comment 33 Jim Mathies [:jimm] 2012-10-04 05:55:33 PDT
Comment on attachment 667920 [details] [diff] [review]
implement nsWindow::GetDefaultScale

A telemetry entry with this data would be useful, especially with win8 being released. I don't see anything like that currently in the tel data list.
Comment 34 Jim Mathies [:jimm] 2012-10-04 06:11:20 PDT
How does layout.css.dpi fit in with this? In the old fennec code we're using for metrofx, this is set to 160.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 06:11:41 PDT
(In reply to Jim Mathies [:jimm] from comment #33)
> A telemetry entry with this data would be useful, especially with win8 being
> released. I don't see anything like that currently in the tel data list.

What exactly would you record?
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 06:13:17 PDT
(In reply to Jim Mathies [:jimm] from comment #34)
> How does layout.css.dpi fit in with this? In the old fennec code we're using
> for metrofx, this is set to 160.

layout.css.dpi overrides the DPI of the screen. This is only used for converting "physical units", of which we currently only support one --- "mozmm" --- and that's not standard. So layout.css.dpi does very little currently.
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 06:18:26 PDT
One more issue with enabling GetDefaultScale:
-- Related to content HTML <canvas>: Panorama's thumbnail canvases look pretty bad, because they're being scaled by 125%. Using a high-resolution backing store would help, but we'd also have to implement toDataURLHD and have Panorama use it for its thumbnail storage. (Or, if Panorama uses some other API to manage image data these days, use the HD version of that API.)
Comment 38 Jim Mathies [:jimm] 2012-10-04 06:26:11 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> (In reply to Jim Mathies [:jimm] from comment #33)
> > A telemetry entry with this data would be useful, especially with win8 being
> > released. I don't see anything like that currently in the tel data list.
> 
> What exactly would you record?

dpi of the screen I guess. Seems like that would be useful since we have rendering issues when dpi is increased on Windows. If 99.999% of our users are at 96dpi, it's not a major issue. But if the number of users that set dpi higher is increasing, it's something we need to deal with via bug 549919.
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 06:39:02 PDT
*** Bug 549919 has been marked as a duplicate of this bug. ***
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 06:39:36 PDT
There are two notions of DPI here: the actual physical DPI of the screen, and the Windows "font DPI" settings. The two are only vaguely related.

I think you're talking about telemetry recording the font DPI setting.
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-04 19:48:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0312e3fe59

I guess we should close this bug and create a bug with a similar title to act as a tracking bug for turning this on by default.
Comment 42 Ed Morley [:emorley] 2012-10-05 03:58:42 PDT
https://hg.mozilla.org/mozilla-central/rev/6c0312e3fe59
Comment 43 Martin Jürgens 2012-10-05 06:43:13 PDT
thank you to everyone involved fixing this issue.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-10-07 19:50:48 PDT
(In reply to Martin Jürgens from comment #43)
> thank you to everyone involved fixing this issue.

Nothing is really fixed yet. This was not meant to change anything by default... Although it looks like something may have gone wrong.
Comment 45 Martin Jürgens 2012-10-08 02:22:59 PDT
at least it looks alright if layout.css.devPixelsPerPx is set to -1.0 (just tried it). so we are on a good track :)
Comment 46 Greg Edwards 2012-12-11 10:42:58 PST
layout.css.devPixelsPerPx is still defaulting to 1.0 on fresh profiles. (Should be -1.0.)
Comment 47 Virtual_ManPL [:Virtual] - (ni? me) 2012-12-11 11:05:08 PST
(In reply to Greg Edwards from comment #46)
> layout.css.devPixelsPerPx is still defaulting to 1.0 on fresh profiles.
> (Should be -1.0.)

Please file a bug on it then
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-12-11 20:26:08 PST
I filed bug 820679 to track turning HiDPI on by default for Windows (non-Metro). There are definitely still some things we need to fix first.
Comment 49 Martin Jürgens 2013-06-17 07:07:44 PDT
Good to see that this has been kind of fixed on Windows now (when setting an option in about:config). Sadly, the very same bug still appears on Linux/GTK (see bug 712898). If anyone could look into it, it would be very much appreciated.

Note You need to log in before you can comment on or make changes to this bug.