Closed Bug 851520 Opened 11 years ago Closed 11 years ago

Default zoom too large on hi-dpi display with 200% font size

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
22 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 23
Tracking Flags:
Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 --- verified

People

(Reporter: ed, Assigned: jfkthame)

References

Details

(Keywords: addon-compat, regression)

Attachments

(3 files, 7 obsolete files)

pt 1 - expose systemDefaultScale attribute on nsIScreenManager.
7.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
part 2 - remove browser.content.full-zoom prefs during UI migration if running on windows/hi-dpi.
2.44 KB, patch
Details | Diff | Splinter Review
add systemDefaultScale attribute to nsIScreenManager, and use it to decide when to remove browser.content.full-zoom prefs during UI migration if running on windows/hi-dpi.
10.36 KB, patch
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:22.0) Gecko/20130315 Firefox/22.0
Build ID: 20130315030943

Steps to reproduce:

This is new with the latest nightly build 22.0a1 (2013-03-15).  I have a Windows 7 (x64) PC with a high-res (200dpi) monitor and therefore have set text size 200% in Window's Control Panel.  With the 200% scaling, in most applications fonts appear at the same size as they would on a 100dpi monitor under normal scaling.

Therefore I would expect Firefox to work the same way, with the default zoom level (View->Zoom->Reset) giving pixel sizes twice those you'd normally get (where 'normal' is a computer with the Windows font size set at its default 100%).


Actual results:

However, in the latest nightly, the fonts are even bigger than would be expected.  For example the Google home page is rendered so large that it doesn't fit horizontally even though the screen is 2400 physical pixels wide.  I suspect that the 200% scaling is being double-counted somehow, so Firefox is now rendering everything at *four* times the number of pixels.


Expected results:

The default zoom should render text at exactly 200% of the pixel size when Windows Control Panel is set to 200% text zoom.  This will match what other applications do.  Please revert back to the behaviour that applied before the most recent nightly build (2013-03-15).  If you can't reproduce this or would like a screenshot let me know.
Confirming.
Nightly 14.03 -good
Nightly 15.03 -bad

range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b672877ed046&tochange=0f7261e288f

Tentatively moving this to layout...

If nobody has any clue on what triggered this, I'll further bisect tomorrow.
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Ah, that's this bug. I understand now why my icons in URL bar and tabs are so blurry, it regressed! I confirm the regression range too.
status-firefox21: --- → unaffected
tracking-firefox22: --- → ?
Keywords: regression
Given that range, presumably bug 844604?

And note bug 852522.

Ed, what does IE do on your system?  The new behavior is supposed to more closely match what IE does...
Blocks: 844604
Flags: needinfo?(ed)
Can't replicate on latest Nightly (2013-03-18), Windows set to 200%, layout.css.devPixelsPerPx at -1.0, font sizes are the same as on Aurora when the pref is set.

Ed: Check that you don't have NoSquint or similar applying a 200% zoom, since this is unnecessary now that Bug 844604 is implemented.
Loic: You are describing Bug 851268.
(In reply to Ed Avis from comment #0)
> Expected results:
> 
> The default zoom should render text at exactly 200% of the pixel size when
> Windows Control Panel is set to 200% text zoom.  This will match what other
> applications do.  Please revert back to the behaviour that applied before
> the most recent nightly build (2013-03-15).  If you can't reproduce this or
> would like a screenshot let me know.

Do you have some kind of add-on installed such as NoSquint that is adjusting your default zoom? When I set the Windows control panel to 200%, I do -not- see appropriately-sized text on older Nightly builds - I see tiny text (within the web content, although the UI text is indeed larger, as expected).

Please check with a fresh profile to see what the new default behavior is; if you've been comfortably reading web pages on a 200dpi screen, my guess is that you've been using some technique (such as a default-zoom-tweaking addon) to achieve this.
(In reply to Greg Edwards from comment #5)
> Loic: You are describing Bug 851268.

Exact, sorry for mixing both bugs. :|
Attachment #726703 - Attachment is obsolete: true
With a fresh profile I don't see the bug.  It was an extension (Default Zoom) that I forgot I had installed.  Presumably some change in Firefox changed the way the extension worked.  I humbly apologize.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(ed)
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Component: Layout → Extension Compatibility
Product: Core → Firefox
Resolution: INVALID → ---
Ed, is the bug caused by this add-on?

https://addons.mozilla.org/firefox/addon/default-fullzoom-level/

If not, do you remember where you installed it from or do you have any more information about it?
Flags: needinfo?(ed)
Yes I think that was the addon I used.
Flags: needinfo?(ed)
Alice, this is your add-on.
I do not think the addon cause the bug.

This also happens the following STR without any addon

0. Set "200%" in Windows's control panel
1. Start Nightly 14.03 with clean profile
2. Open a webpage
3. Zoom in several times  --- remember The size of text and images
4. Quit

5 Start Nightly 15.03 with same profile
6. Open the webpage of Step.2

Actual results:
The size of text and images are twice now

 So this is Firefox's behavior.
What you're describing is nonetheless correct behaviour given the nature of the patch. You start with a wrongly sized page in 14.03 and zoom it in to the correct size (but Nightly thinks it's 200%). You've now told Nightly you want this page to be zoomed in 2x. Then you open 15.03 which knows the correct zoom, but it still remembers you told it you want the page displayed at 2x, so it displays it at 2x which is now correctly very big.

All it could do is forget/scale existing zoom settings, but even this has problems. What happens to pages you never applied a zoom to? Do they now show at 50%? This defeats the purpose of Bug 844604.

The only other thing I can think of is to forget all manual zoom levels, both in Firefox and in addons, but this needlessly burdens 96dpi users.

There's no perfect solution.
(In reply to Greg Edwards from comment #13)
> The only other thing I can think of is to forget all manual zoom levels,
> both in Firefox and in addons, but this needlessly burdens 96dpi users.

It could be done depending on the DPI setting.
Does this only occur with bug layout.css.devPixelsPerPx set to -1? If so, bug 820679 would imply that this regression has not yet landed (and likely shouldn't land) while we're still shaking out regressions.
Jonathan - I believe this is your bug, given assignee of bug 844604.
Assignee: nobody → jfkthame
tracking-firefox22: ? → +
Well this isn't a regression, per se, but rather, changed (and more correct) behaviour which could be sufficiently confusing to existing users who are accustomed to the previous, wrong behaviour.

People were using add-ons to workaround Bug 844604. (NoSquint, Default Zoom Level) With 844604 landed, on these configurations, the bug has been corrected for *twice*, leading to correct but unexpected behaviour--huge websites. (see my previous comment #13)

This bug is about coming up with ways to soften the blow so there isn't public outcry from confused users.
While I agree this is arguably not a bug, it is still an issue that we need to address. I'm experimenting with a patch that I think will allow us to discard zoom levels that the user had set manually prior to the browser becoming dpi-aware. I'm not sure yet how it will interact with zoom add-ons, though.
As nsIDOMWindowUtils.displayDPI does not expose the "logical" DPI, we need another way for browser front-end code (and add-ons?) to know about Windows logical DPI or (equivalently) the widget default scale factor, in order to make decisions about managing the zoom factor. Simplest option seems to be adding a (read-only) defaultScale attribute to nsIDOMWindowUtils, which will simply return the result of nsIWidget::GetDefaultScale. Browser code can use this to determine what kind of lo- or hi-dpi environment it's in.
Attachment #732984 - Flags: review?(roc)
Oops, attached the wrong patch. The preceding comment was meant to accompany -this- patch; the browser JS patch that uses it will follow in a moment.
Attachment #732986 - Flags: review?(roc)
Comment on attachment 732984 [details] [diff] [review]
pt 2 - on Windows systems with logical DPI > 96, adjust old page-zoom values to account for new DPI-aware scaling behavior

I don't really have much understanding of this code, but here's an attempt at a possible way forward... alternative ideas and approaches welcomed, too.

The idea here is that if we have a saved zoom level (>1.0) that was set in an older (non-hidpi-aware) version of the browser, and now the user reopens the page on a hidpi system where we now apply resolution-appropriate scaling, we should make a corresponding reduction to the saved zoom level. That way, the actual rendered size of the content will remain as it was in the older version.
Attachment #732984 - Flags: review?(roc) → review?(gavin.sharp)
Comment on attachment 732984 [details] [diff] [review]
pt 2 - on Windows systems with logical DPI > 96, adjust old page-zoom values to account for new DPI-aware scaling behavior

As I understand it, browser.content.full-zoom is a default value for zooming that is not user-configurable in Firefox proper, right? And the issue is that some add-ons adjust it, which leads to trouble now that we changed our behavior?

Assuming that understanding is correct, I think it would make more sense to simply change the pref name (effectively resetting its value for everyone), without the attempts to migrate the old preference.

Drew's been patching this code recently so is probably a better choice for review.
Attachment #732984 - Flags: review?(gavin.sharp) → review?(adw)
Moved this to Firefox General, as it's not just about extensions... maybe we should have a separate bug on those interactions.

It's not just about browser.content.full-zoom; as I understand it, we "remember" zoom on a per-domain basis in content prefs, so even in the absence of any add-ons or default-pref changes, a user who has been in the habit of zooming in on pages to make them readable on a hi-dpi display will see those sites oversized in the new browser.

If we simply rename the prefs (effectively discarding existing values), however, we'd hurt users who have been zooming on 96-dpi systems, e.g. because of poor eyesight or because certain pages are simply designed with too-small fonts; they'd find their settings "forgotten". The patch here attempts to mitigate this by migrating the old settings, and only scaling them down if the environment is >96 dpi.

Tryserver job in progress: https://tbpl.mozilla.org/?tree=Try&rev=4b0c5df6a089

Once this build is ready, it should be possible to test it with a scenario such as this:

1. Set the Windows resolution scale to 150% or even 200% (if your screen is big enough for this to be usable).

2. Run the current release version of Firefox. (Ensure the layout.css.devPixelsPerPx preference is left at its default setting.) Visit various sites, zooming in on each page so as to make the text size suitable for the hi-dpi environment.

3. Quit, and launch current Nightly (i.e. hidpi-aware, but without the patches here). Note how the previously-visited sites now display way too big, because the saved zoom factors are applied, but we're now applying them -on top of- the dpi-based scaling.

4. Without resetting those zoom factors, quit the browser; now launch the Try build instead. The previously-zoomed sites should now display at the proper scale.
Component: Extension Compatibility → General
Another possibility, perhaps, would be a "hybrid" kind of approach where we'd use a new pref name -if- we're running with >96dpi, so effectively discarding existing zoom settings for hidpi users; but continue to use the existing pref name for 96dpi users. I.e., no attempt to migrate and adjust existing settings; just a binary choice of whether to use them or ignore them. Hidpi users get a clean slate; non-hidpi users see no change.

This would be simpler to implement (I think - but I have no idea whether I'm approaching this code in the right way at all!) If you think that's a reasonable compromise, I'll try to put together a patch along those lines.
I'd prefer if we just threw the old data away for >96 dpi settings e.g. in nsBrowserGlue.js / _migrateUI rather than migrating values in browser-fullZoom.js or introducing a new pref name.
That's certainly another possibility - discard the prefs up-front when migrating to the new version, rather than deciding to ignore them at use-time. I wasn't sure how expensive this might be, if there are saved zoom settings for many domains, but maybe it'd be the better option. Would you be prepared to work up a patch on those lines?
Accessing content-prefs.sqlite directly with the Storage API should be feasible. I don't know if there's a reasonable higher-level API to do remove content prefs.
nsIContentPrefService2 certainly supports removing prefs.
It supports removing them; I haven't looked to see how easily it supports iterating over them to find all the full-zoom prefs for individual domains, so as to remove them all.

Re comment 25: a disadvantage of that approach, compared to using a new pref name, is that it won't "defeat" add-ons that work (AIUI) by resetting the zoom value for the user, based on their own extension prefs. If we use a new pref name, we'll simply ignore what the existing add-ons are doing - which is probably a good thing.
Here's an alternate (and very simple) approach - just check for a hidpi setup in FullZoom_init(), and change the pref name there. I haven't tested this on Windows and together with add-ons yet, but it seems like it should solve most of the issues, while leaving 96dpi users unaffected.
Attachment #733066 - Flags: review?(adw)
(In reply to Jonathan Kew (:jfkthame) from comment #29)
> It supports removing them; I haven't looked to see how easily it supports
> iterating over them to find all the full-zoom prefs for individual domains,
> so as to remove them all.

Right, that's what I meant. We can bulk-remove them with SQL.
Comment on attachment 732984 [details] [diff] [review]
pt 2 - on Windows systems with logical DPI > 96, adjust old page-zoom values to account for new DPI-aware scaling behavior

Review of attachment 732984 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Dão.  You should be able to easily remove zoom settings for all domains by calling nsIContentPrefService2.removeByName("browser.content.full-zoom").
Attachment #732984 - Flags: review?(adw) → review-
(In reply to Jonathan Kew (:jfkthame) from comment #29)
> Re comment 25: a disadvantage of that approach, compared to using a new pref
> name, is that it won't "defeat" add-ons that work (AIUI) by resetting the
> zoom value for the user, based on their own extension prefs. If we use a new
> pref name, we'll simply ignore what the existing add-ons are doing - which
> is probably a good thing.

Aren't these add-ons unnecessary now that the aforementioned bugs have been fixed?
(In reply to Drew Willcoxon :adw from comment #33)
> (In reply to Jonathan Kew (:jfkthame) from comment #29)
> > Re comment 25: a disadvantage of that approach, compared to using a new pref
> > name, is that it won't "defeat" add-ons that work (AIUI) by resetting the
> > zoom value for the user, based on their own extension prefs. If we use a new
> > pref name, we'll simply ignore what the existing add-ons are doing - which
> > is probably a good thing.
> 
> Aren't these add-ons unnecessary now that the aforementioned bugs have been
> fixed?

It depends what people are using them for. In many cases, yes - the add-on was being used to work around the browser's failure to honor the Windows settings for appropriate scaling on a hi-dpi display.

However, I expect there are also people who simply want to zoom in everywhere; e.g. if you're browsing the web on a physically large (lots of pixels, but still 96dpi) screen with a maximized browser window, many websites will only use a part of the available width. Some users may prefer to set a default zoom factor such as 120% or 150% to make sites fit their huge window better.
I agree with throwing old data away (or scaling it) on migration. Leaving useless prefs behind in the database as bloat doesn't sound like a good idea.

Another usage case of addons is when users want things bigger but don't want to change Windows's DPI settings because of poor 3rd party app support.
(In reply to Dão Gottwald [:dao] from comment #25)
> I'd prefer if we just threw the old data away for >96 dpi settings e.g. in
> nsBrowserGlue.js / _migrateUI rather than migrating values in
> browser-fullZoom.js or introducing a new pref name.

If we bump the UI version and use a version-specific code block in _migrateUI, it looks to me like this will only be done the first time the new browser version is run.

So if a user tries a Nightly/Aurora/Beta build that has this feature, and then for any reason goes back to an older release, the prefs will likely get reinstated by the older version; but then next time the user migrates (e.g. when the new version hits the release channel), browser.migration.version will already have been updated and so the migration code won't be run again - and so the unwanted prefs will stay untouched.

I suppose the majority of users only ever make a single migration (forwards), but anyone who ever runs one of the pre-release channels would be liable to be bitten by this. It would appear to work fine the first time they try the new browser, but would then be broken on every subsequent attempt - even for the official release-channel update. That sounds like a pretty negative (and confusing) experience. :(
For what it's worth, I've already been "bitten" by Nightly removing my custom devPixelsPerPx=-1.0 pref whenever I go back to Aurora. It comes with the territory and isn't something we should worry about.
I think that's a somewhat different case... if you're poking around in about:config, you're assumed to be a "higher level" of user than most. The scenario I'm concerned about could affect a much wider range of users: anyone who tries the Beta or Aurora channel, but then returns to their Release version, without ever delving into about:config.

(Regarding devPixelsPerPx, I assume it's getting reset because when you run Nightly, "-1.0" is the default, so we "forget" that it was explicitly set; then when you return to Aurora, you get Aurora's default. You could overcome this by setting it to something like "-1" instead of "-1.0", so that it doesn't actually match the default, but still has the same effect.)
Moreover, I think it's significantly worse in that it leads to "broken" behavior in the -new- version, and the user will only be able to fix it by manually changing/clearing prefs; until they figure this out, their Firefox will be permanently messed-up. In your case, it's your customization of the older version that's being lost, but looking forward, that's going to be obsolete in a few weeks anyway.
(In reply to Jonathan Kew (:jfkthame) from comment #36)
> If we bump the UI version and use a version-specific code block in
> _migrateUI, it looks to me like this will only be done the first time the
> new browser version is run.
> 
> So if a user tries a Nightly/Aurora/Beta build that has this feature, and
> then for any reason goes back to an older release, the prefs will likely get
> reinstated by the older version; but then next time the user migrates (e.g.
> when the new version hits the release channel), browser.migration.version
> will already have been updated and so the migration code won't be run again
> - and so the unwanted prefs will stay untouched.

To some extent this is true for all migrations taking place there. I think it's acceptable in this case as well. I'd rather not over-engineer this, given that:

* you use beta software at your own risk, things can break
* the breakage isn't catastrophic and can be dealt with manually (the easiest way would be to stay on beta until the new DPI handling moves to the release channel)
* only a small minority is affected (people with high DPI on Windows × people that adjusted the zoom level for many sites × beta/alpha users × people that switch back and forth between channels)
Comment on attachment 733066 [details] [diff] [review]
pt 2 - use a different name for the full-zoom content pref on hidpi windows systems

Review of attachment 733066 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Dão.  I'll r+ a patch that does what he suggests.
Attachment #733066 - Flags: review?(adw) → review-
For the remove-prefs-during-migration approach, we'll need to expose logicalDPI on nsIScreenManager (or something equivalent), I think, as we don't have a window available when running the UI migration code.
Attachment #733432 - Flags: review?(roc)
Comment on attachment 732986 [details] [diff] [review]
pt 1 - expose default scale factor from widget code to chrome JS as nsIDOMWindowUtils.defaultScale

Cancelling r? on this, as we won't need it if we're going with the UI-migration approach.
Attachment #732986 - Flags: review?(roc)
Attachment #733433 - Flags: review?(adw) → review+
Tryserver build with the patches to discard zoom prefs during UI migration on win-hidpi:
https://tbpl.mozilla.org/?tree=Try&rev=9c383ceb6e2e
Blocks: 858185
So this should largely fix the problem for zoom values "remembered" in Firefox content prefs.

What it won't fix, unfortunately, is the problem for users who have installed a zoom-overriding add-on such as Default FullZoom Level, NoSquint, or others, and perhaps forgotten all about it (see comment 8).

I've filed bug 858185 about the add-ons aspect of this.
Comment on attachment 733432 [details] [diff] [review]
part 1 - expose logicalDPI attribute on nsIScreenManager

Review of attachment 733432 [details] [diff] [review]:
-----------------------------------------------------------------

This is OK but can we avoid the code duplication here somehow? E.g. by putting this code into a method that nsIWidget can call?
I'm not sure we can gain much, if anything, by trying to share with widget... after all, this is specifically exposing a value that may be -different- from nsIWidget's GetDPI - at least in the Windows case, and I think it probably should be on Android too, if we want GetDPI there to be truly "physical" - currently it's not.

If we had an nsBaseScreenManager, we could put a default implementation returning 96.0 there, and then just override it on platforms that need something different; but IMO it doesn't seem worth it just for that. The code that's duplicated here is just a single simple statement.
I meant share with nsIWidget::GetDefaultScale ... doesn't this equal nsIWidget::GetDefaultScale()*96?
Not if the user has adjusted devPixelsPerPx.
Though I guess it's not really clear whether we ought to take that into account here. Or in various other places - currently, I believe it can break full-screen mode, for example. (Sigh.)
Oh, and there's the complication that nsIWidget::GetDefaultScale() on OS X may vary depending which screen the widget is on. Whereas here, we're looking at something that is a system-wide property on Windows, and we need access to it before any widgets have been created, AFAIK.
So how should this work on Mac?
I'd suggest that the only reasonable "system-wide logical DPI" value for the Mac is 72. Retina displays have a logical backing-store DPI of 144, but all system metrics work in terms of 72dpi pixel units (AKA "Cocoa points").

So if we exposed logicalDPI on nsIWidget, it would vary depending on the screen; but we don't currently have a need for that, AFAIK. At the nsIScreenManager level (i.e. system-wide), it's fixed at 72.

And neither of those values corresponds to the physical device DPI that nsIWidget::GetDPI returns; that depends on the physical size of the display and the resolution option chosen in the Display prefs.
To clarify (maybe) a bit further - OS X doesn't offer an equivalent to the Windows "logical DPI" adjustment. Both Mac and Win let the user adjust the effective resolution of the display, reducing its "device DPI" in order to make everything larger (but less crisp!), but only Windows has the "Smaller, Medium, Larger, Set custom text size..." options that, although phrased primarily in terms of text, are supposed to scale everything in the UI by redefining "logical DPI" so that everything sized by applications in terms of points/inches/cm/whatever will be rendered with more actual pixels.

So on Windows, logicalDPI will usually be one of the "standard" values 96, 120, 144, although users can set custom values as well; but in any case, it will -not- usually be the same as the physical device DPI that we'll need if mozmm is to be implemented accurately.
Comment on attachment 733432 [details] [diff] [review]
part 1 - expose logicalDPI attribute on nsIScreenManager

Review of attachment 733432 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsIScreenManager.idl
@@ +26,5 @@
>    readonly attribute unsigned long numberOfScreens;
>  
> +    // The logical DPI of the screen environment; 96.0 by default,
> +    // but will vary on Windows if the UI scaling option for hi-dpi is used.
> +  readonly attribute float logicalDPI;

OK, I think this needs to be more clearly specified, and maybe "logical DPI" isn't the best name for it. In particular it needs to be clear how and when this value should be used.
(In reply to Jonathan Kew (:jfkthame) from comment #54)
> I'd suggest that the only reasonable "system-wide logical DPI" value for the
> Mac is 72. Retina displays have a logical backing-store DPI of 144, but all
> system metrics work in terms of 72dpi pixel units (AKA "Cocoa points").
> 
> So if we exposed logicalDPI on nsIWidget, it would vary depending on the
> screen; but we don't currently have a need for that, AFAIK. At the
> nsIScreenManager level (i.e. system-wide), it's fixed at 72.

I disagree with this so much. A "Cocoa point" is the exact same thing as a "WPF point." Both Cocoa and WPF have extremely similar if not identical scaling models. The key difference is that most Windows apps aren't WPF and so receive the old, obsolete model of screen scaling which uses device pixels. (Windows GDI would be more accurately compared to Carbon.)

There's also a pretty big disconnect in terms of 72dpi vs 96dpi as the default 100% scaling on Mac vs. Windows. (It reminds me of how Mac used to use 1.8 gamma.)

The way I see it, "logical DPI" should represent the device pixel ratio used by the rest of the platform.
(In reply to Greg Edwards from comment #57)
> The way I see it, "logical DPI" should represent the device pixel ratio used
> by the rest of the platform.

On OS X, there isn't a single answer to that, it's a per-display value. But there is a system-wide coordinate space, and it is treated as being "logically" (or nominally, if you prefer) 72dpi, regardless of the physical size of the pixels, or of the backing scale factor being used.

> Both Cocoa and WPF have extremely similar if not identical scaling models.

I think that's only true if you're considering NSWindow's userSpaceScaleFactor, which I doubt we support properly, and which is deprecated now anyhow; with the Retina displays, Apple has chosen to go for two discrete scaling factors (1.0 or 2.0), rather than a continuously-varying scale factor.
Instead of "logicalDPI", this version exposes an attribute named systemDefaultScale. This is intended to reflect any system-wide resolution scaling factor that is in effect. Note that it does not depend on Gecko prefs (devPixelsPerPx) that may override this; it is intended to return the -system- default.

Currently, just returns 1.0 everywhere except on Windows, where it depends on the user's chosen setting in the Display control panel. If/when we fix bug 798362 or bug 712898, I expect we'll want to make the gtk2 implementation return the appropriate scaling factor there too.

(On OS X, this is unlikely to be useful, as the use of a system-wide scaling factor (userSpaceScaleFactor) is deprecated in favor of the per-display backingScaleFactor that is used for Retina-display support.)

Patch 2 can of course be trivially revised to check this value instead of logicalDPI to know when to discard saved zoom settings.
Attachment #733432 - Attachment is obsolete: true
Attachment #733432 - Flags: review?(roc)
Attachment #734614 - Flags: review?(roc)
I can agree that "system-wide scaling" doesn't exist on OSX. (I also sincerely hope it's retired in the next version of Windows. Currently hooking up an external monitor to my rMBP when it's booted Windows is a joke.)

I worry moreso about committing to a system-wide scaling model on any platform, since this model won't be practical for much longer.

(WPF does have an equivalent of backingScaleFactor, it's just not normally seen outside the Visual Studio IDE since Windows doesn't support it.)
(In reply to Greg Edwards from comment #60)
> I worry moreso about committing to a system-wide scaling model on any
> platform, since this model won't be practical for much longer.

If/when the current model is retired on Windows, or we decide to stop supporting it, we can trivially make nsIScreenManager.defaultSystemScale just return a constant 1.0, like on other platforms. Presumably if/when we adopt a significantly different model, it would be best to expose any required info from that via a distinct API, so that front-end or add-on code that needs to know about it can tell which model it's dealing with.
Comment on attachment 734614 [details] [diff] [review]
part 1 - expose systemDefaultScale attribute on nsIScreenManager

Review of attachment 734614 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't understand from the comment how this should be used, and why it's always 1.0 even on Retina Macs.
It represents "system-wide DPI scale as exposed by the platform". If anything, it should be null on OSX since DPI scale is not system-wide.

It seems the only thing it's used for is migrating zoom settings. After they're migrated, it ought to be completely useless.
OK, so people have zoomed into sites because Firefox wasn't using a default zoom, and now we're zooming in by default, those existing zoom settings make us zoom too much. And this wasn't a problem on Mac because on Retina Macs we always rendered at the right size automatically via Cocoa scaling.

What confuses me about "system-wide DPI scale" is that I think of Mac Retina scaling as a "system-wide DPI scale". What's different between Mac and Windows is not whether there is a system-wide DPI scale, but that we have always honored it on Mac and only just started honoring it on Windows.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> OK, so people have zoomed into sites because Firefox wasn't using a default
> zoom, and now we're zooming in by default, those existing zoom settings make
> us zoom too much. And this wasn't a problem on Mac because on Retina Macs we
> always rendered at the right size automatically via Cocoa scaling.

Yes. Well, strictly speaking there was an analogous issue on the Mac: Before we landed bug 674373, it was possible to force Firefox to open in "native" resolution - e.g. see Asa's comment in bug 674373#c102 - and then use full-zoom to make things readable, rather than leaving Cocoa to do its pixel-doubling thing. (The chrome remained tiny, of course.)

For someone who had done this, and so zoomed all pages to 200%, the landing of real hidpi support would've made everything suddenly way too big, and they'd have to reset their zoom levels. But in the retina-mac case, this was such an obscure edge case - not many people would have even figured out how to do it, let alone been prepared to live with the half-size chrome - that we didn't worry about it.

So the difference is that on Windows, we've always been rendering content at the "native" resolution, with the result that as pixel densities have increased, the physical size of the displayed content has been shrinking, and so users have commonly used full-zoom - either manually or via add-ons that automatically modify it (bug 858185) - to compensate for this.

> What confuses me about "system-wide DPI scale" is that I think of Mac Retina
> scaling as a "system-wide DPI scale".

But it's not system-wide, it's per-display.

> What's different between Mac and
> Windows is not whether there is a system-wide DPI scale, but that we have
> always honored it on Mac and only just started honoring it on Windows.

In practice, yes. And so we need the core platform code to let the front-end code know about this change, so that it can adjust its behavior accordingly.

In theory, I suppose it would be possible for the chrome JS code to infer the systemDefaultScale value from other properties - it would show up in the ratio between the nsIMarkupDocumentViewer.fullZoom value and window.devicePixelRatio. But that would require a window to be created already, and we want to know this ahead of time during the first launch of the new browser version, -before- creating any windows.

From the bugs about respecting x11/gtk2 dpi scaling factors, I suspect we might start honoring a similar setting there at some point, in which case the same migration issue would apply - and the same API here would allow the front-end to handle it.
(In reply to Jonathan Kew (:jfkthame) from comment #65)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #64)
> > What confuses me about "system-wide DPI scale" is that I think of Mac Retina
> > scaling as a "system-wide DPI scale".
> 
> But it's not system-wide, it's per-display.

OK, but distinguishing system-wide from per-display here doesn't really help the user of this API figure out what it's for.

> > What's different between Mac and
> > Windows is not whether there is a system-wide DPI scale, but that we have
> > always honored it on Mac and only just started honoring it on Windows.
> 
> In practice, yes. And so we need the core platform code to let the front-end
> code know about this change, so that it can adjust its behavior accordingly.

OK.

I think what we need to report here is our behavior change, not the actual systemDefaultScale.

How about this API on nsIScreenManager: int32_t GetZoomPrefsGeneration(), returning
0: Honor all zoom prefs
N>0: Honor only zoom prefs set when GetZoomPrefsGeneration() returned N

Every platform returns 0 except Windows, which returns 0 if the system-wide DPI scale is 1.0, otherwise returns 1 (since we only just started honoring it).

When we start honoring it on Linux, we should start returning 1 when its system-wide DPI scale is not 1.0.

How does that sound?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> I think what we need to report here is our behavior change, not the actual
> systemDefaultScale.

That would work, though it provides slightly less info to the front-end code (it only lets it know that the behavior has changed, but not what the new scale is). ISTM there's no reason -not- to expose the actual value. Although all that's proposed in this bug is a boolean behavior - either keep or discard the prefs - it's entirely conceivable an add-on author might decide to inverse-scale their zoom values, for example, so as to preserve the actual rendered size for users.

Directly exposing the systemDefaultScale value makes it easier to achieve this, without having to infer the "built-in" scaling we've started to apply from the devicePixelRatio or screenPixelsPerCSSPixel and fullZoom values.

> 
> How about this API on nsIScreenManager: int32_t GetZoomPrefsGeneration(),
> returning
> 0: Honor all zoom prefs
> N>0: Honor only zoom prefs set when GetZoomPrefsGeneration() returned N

This approach sounds like it effectively requires us to start "tagging" our zoom prefs in some way, so that we know whether they're "old" or "new". The consensus from the Firefox side here seemed to be that we don't want to add extra complexity to the zoom management there, just take the lightweight approach of discarding all zoom prefs when migrating to a hidpi-aware browser version.

> 
> Every platform returns 0 except Windows, which returns 0 if the system-wide
> DPI scale is 1.0, otherwise returns 1 (since we only just started honoring
> it).
> 
> When we start honoring it on Linux, we should start returning 1 when its
> system-wide DPI scale is not 1.0.
> 
> How does that sound?

It could work, but I don't see the benefit over the straightforward approach here. Why are you so hesitant to expose systemDefaultScale?
I'm not opposed to systemDefaultScale if you can explain clearly in the comments how it should be used. As it is, I don't think anyone has a chance of using it for its intended purpose unless they read and understand the contents of this bug, and maybe not even then :-). For example, it's clear that on GTK2 it is NOT the "system-wide DPI scale" ... it's actually "the system-wide DPI scale if we're honoring it, or 1.0 if we aren't".

In fact I still have some questions about this will be used. Are we going to have to land a different version of part 2 to deal with upgrading zoom settings for Linux users, when we start honoring system-wide DPI scale on Linux? What if a user changes the system-wide DPI scale, or upgrades their system in a way that changes the scale automatically? Shouldn't we be discarding zoom settings in those cases too?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> What if a user changes the system-wide DPI scale, or upgrades their
> system in a way that changes the scale automatically? Shouldn't we be
> discarding zoom settings in those cases too?

If the user perceived some content as too small or too large on a 96dpi screen and 96dpi system setting, the same would be true for some >96dpi screen and system setting, assuming we handle it correctly (which we didn't but do now), so the same content full zoom factors should be used.

Users changing the system dpi setting independently from the screen are essentially saying "make everything larger/smaller", and content full zoom factors should be applied on top of that. It's possible that e.g. users with poor eye sight migrate from zooming in on many sites to adjusting system dpi settings, but we can't really discern this case.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> I'm not opposed to systemDefaultScale if you can explain clearly in the
> comments how it should be used. As it is, I don't think anyone has a chance
> of using it for its intended purpose unless they read and understand the
> contents of this bug, and maybe not even then :-). For example, it's clear
> that on GTK2 it is NOT the "system-wide DPI scale" ... it's actually "the
> system-wide DPI scale if we're honoring it, or 1.0 if we aren't".

True, it's the value we are applying, not some value we're ignoring. (Likewise, if we had added this -before- landing bug 844604, it would've been appropriate for it to return 1.0 on Windows, regardless of the user's actual Windows settings.)

TBH, I don't expect there'll be any use cases for this outside of this bug, and the add-on changes that we hope a couple of authors will consider making (bug 858185). But I don't mind adding more extensive comments if you think that helps.

> In fact I still have some questions about this will be used. Are we going to
> have to land a different version of part 2 to deal with upgrading zoom
> settings for Linux users, when we start honoring system-wide DPI scale on
> Linux?

Yes, assuming that doesn't happen in the same UI-version cycle; the code in nsBrowserGlue doesn't deal with separate versioning for different prefs, the version is a global. So if the change happens at different times on the two platforms, those will appear as two separate version bumps there, each of which will have update code only for the affected platform.

A bit untidy, perhaps, but it's localized to this code fragment that runs once, when the user moves to the new browser version. Alternative approaches that make on-the-fly adjustments to zoom based on current dpi settings seemed more complex than was justified here.

> What if a user changes the system-wide DPI scale, or upgrades their
> system in a way that changes the scale automatically? Shouldn't we be
> discarding zoom settings in those cases too?

I don't think so. If the user changes a system-wide setting in a way that is supposed to make everything on their screen larger or smaller, or replaces their display with one of a very different size, then not only web pages in Firefox but pretty much everything in every application will be displayed at a correspondingly changed size. That's expected. If the user had been zooming pages in (or out) from the default rendered size prior to the change, it's reasonable that those pages remain zoomed in (or out) from the new default after the system change.

What we're trying to deal with here is the unexpected change that happens on a Firefox upgrade from non-hidpi-aware to hidpi-aware; in this case, the user has -not- changed anything about their system configuration, yet Firefox will suddenly start rendering pages with a different (default) size.

For hidpi-display users who didn't mess with Firefox zoom, they'll see a change; but we believe it's a good change: it will make the default scale at which we render the web match better with the default scale of other applications on their system. But for users who had been using zoom to work around our previous, poor behavior, things will suddenly appear -wrong- because their workaround, applied on top of our fix, results in oversized display. That, and only that, is the case we're trying to mitigate here.
Here's a version with more extensive comments in nsIScreenManager.idl.
Attachment #735141 - Flags: review?(roc)
Comment on attachment 735141 [details] [diff] [review]
pt 1 - expose systemDefaultScale attribute on nsIScreenManager.

Review of attachment 735141 [details] [diff] [review]:
-----------------------------------------------------------------

I still find this ugly, but OK.
Attachment #735141 - Flags: review?(roc) → review+
Attachment #734614 - Attachment is obsolete: true
Attachment #734614 - Flags: review?(roc)
Updated part 2 to check nsIScreenManager.systemDefaultScale instead of .logicalDPI, as that's what we're exposing in the final version of part 1. Functionally equivalent; carrying over r=adw.
Attachment #733433 - Attachment is obsolete: true
Attachment #733066 - Attachment is obsolete: true
Attachment #732984 - Attachment is obsolete: true
Comment on attachment 732986 [details] [diff] [review]
pt 1 - expose default scale factor from widget code to chrome JS as nsIDOMWindowUtils.defaultScale

To reduce confusion, obsoleting the various patches to implement alternate approaches that we decided not to use.
Attachment #732986 - Attachment is obsolete: true
(In reply to Jonathan Kew (:jfkthame) from comment #74)
> Comment on attachment 732986 [details] [diff] [review]
> pt 1 - expose default scale factor from widget code to chrome JS as
> nsIDOMWindowUtils.defaultScale
> 
> To reduce confusion, obsoleting the various patches to implement alternate
> approaches that we decided not to use.

I could really use this. We have apis that are accessible through chrome script that return device pixels.
(In reply to Jim Mathies [:jimm] from comment #75)
> I could really use this. We have apis that are accessible through chrome
> script that return device pixels.

I suspect you might be looking for something like window.devicePixelRatio or maybe nsIDOMWindowUtils.screenPixelsPerCSSPixel.
Target Milestone: --- → Firefox 23
...and backed out, because I missed the Gonk implementation of nsIScreenManager. :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a624a23c88a
Combined patch with the (trivial) missing Gonk impl added, for re-landing. Carrying over r=roc,adw from the individual parts.
https://hg.mozilla.org/mozilla-central/rev/a703fa409b15
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment on attachment 735421 [details] [diff] [review]
add systemDefaultScale attribute to nsIScreenManager, and use it to decide when to remove browser.content.full-zoom prefs during UI migration if running on windows/hi-dpi.

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

User impact if declined: users who have been zooming pages to work around too-small display on high-density screens will suddenly get over-large display when Firefox starts respecting the system scale factor

Testing completed (on m-c, etc.): on m-c since 04/10

Risk to taking this patch (and alternatives if risky): low risk - simply discards some prefs when it detects the changed environment, worst-case outcome would be that it resets zoom for some users who would have preferred to keep their settings

String or IDL/UUID changes made by this patch: nsIScreenManager UUID changed, because a new attribute is added
Attachment #735421 - Flags: approval-mozilla-aurora?
Attachment #735421 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Approving with UUID change (this is aurora) as it's been on central for several days and we'll get more bake time on aurora before moving to Beta. Would be good to get some QA around whether this does affect people's settings for a page, not the end of the world if it does but if we know in advance we can be sure to have a KB article ready.
Keywords: qawanted, verifyme
Keywords: qawanted
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Buid ID:20130617145905

Tested on FX 22-B6. I can not reproduce this issue.Followed steps given in Comment 0 and 12. I don't see change font size.
status-firefox22: fixed → verified
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130711122148

Tested on FX 23-B5. I can not reproduce this issue.Followed steps given in Comment 0 and 12. I don't see change font size.
status-firefox23: fixed → verified
Status: RESOLVED → VERIFIED
Thanks Samvedana.
Keywords: verifyme
Depends on: 1356023
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: