Closed Bug 603880 Opened 14 years ago Closed 12 years ago

Gecko doesn't apply system default scale on Windows

Categories

(Core :: Widget: Win32, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: at.light, Assigned: roc)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.
Attached image screenshot of bug
Sorry if anyone is offended by my use of the Wikipedia web page.
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".
Version: unspecified → Trunk
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.
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>
Attached patch Fix renderer DPISplinter Review
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
I put a version of above DPI test script online for easier access:
http://www.home.hs-karlsruhe.de/~fado0001/testdpi.html
(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!!!
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.)
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().
(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.
(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.
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.
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.
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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.
(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.
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?
We can, that is what this bug is about.
any news on this?
No.
Summary: Web content does not automatically scale to system DPI → Gecko doesn't apply system default scale on Windows
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.
Whiteboard: [mentor=roc@ocallahan.org]
(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.
Whiteboard: [mentor=roc@ocallahan.org] → [mentor=roc@ocallahan.org][lang=c++]
Blocking bug 378927?
Oh sorry, missed that this here wasn't a core bug.
it is really frustrating to still see this bug around.
when internet explorer can do it, firefox should, too.
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.
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.
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.
Assignee: nobody → roc
Attachment #667920 - Flags: review?
Attachment #667920 - Flags: review? → review?(jmathies)
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.
Attachment #667920 - Flags: review?(jmathies) → review+
Blocks: 797828
Blocks: 797829
How does layout.css.dpi fit in with this? In the old fennec code we're using for metrofx, this is set to 160.
(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?
(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.
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.)
(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.
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.
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.
Whiteboard: [mentor=roc@ocallahan.org][lang=c++]
https://hg.mozilla.org/mozilla-central/rev/6c0312e3fe59
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Component: Shell Integration → Widget: Win32
Product: Firefox → Core
Target Milestone: Firefox 18 → mozilla18
thank you to everyone involved fixing this issue.
Depends on: 798607
(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.
at least it looks alright if layout.css.devPixelsPerPx is set to -1.0 (just tried it). so we are on a good track :)
layout.css.devPixelsPerPx is still defaulting to 1.0 on fresh profiles. (Should be -1.0.)
(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
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.
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.
You need to log in before you can comment on or make changes to this bug.