Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
10 months ago

People

(Reporter: roc, Assigned: roc)

Tracking

({dev-doc-complete})

Trunk
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 7 obsolete attachments)

11.99 KB, patch
vlad
: review+
Details | Diff | Splinter Review
6.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.71 KB, patch
vlad
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
6.25 KB, patch
jimm
: review+
Josh Aas
: review+
Details | Diff | Splinter Review
11.88 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.16 KB, patch
surkov
: review+
Details | Diff | Splinter Review
15.53 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.52 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.71 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.43 KB, patch
Details | Diff | Splinter Review
47.95 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
15.46 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
180.59 KB, image/png
Details
We want to disable automatic scaling for now. When automatic scaling is reenabled, it should depend on platform settings such as "UI Resolution" on Mac.

We should get the true DPI for the screen and report it on all platforms.

I also think we should get the DPI through an API on nsIWidget, so that it can be per-widget ... i.e., creating a window on a new screen should get the right DPI.
(In reply to comment #0)
> I also think we should get the DPI through an API on nsIWidget, so that it can
> be per-widget ... i.e., creating a window on a new screen should get the right
> DPI.

I don't know that this is a good idea -- what do you do when the window moves to another screen?  Or when it straddles two screens?  Or are you just referring to linux without xinerama or something, where an X Screen is isolated from all others?
(In reply to comment #1)
> I don't know that this is a good idea -- what do you do when the window moves
> to another screen?  Or when it straddles two screens?

Short-term answer: you lose, in that case. But we still get things right when you open a new window on a particular screen, or even reload a document while the window is on the right screen. We don't need to make the best the enemy of the good.

Long-term answer: it wouldn't actually be that hard to detect when the window has moved from one screen to another (or when the screen that the window is "mostly on" has changed) and fire some kind of event to perform a dynamic DPI change. We already support dynamic DPI changes when the layout.css.dpi pref changes. This could also handle the case when the screen DPI changes due to a mode change or other screen reconfiguration.
Created attachment 420228 [details] [diff] [review]
Part 1: Create new DPI APIs in nsIWidget

This patch makes layout.css.devPixelsPerPx default to 1.0, which means nsIWidget::GetDefaultScale() will be ignored even if it gets implemented. I plan to file followup bugs to implement GetDefaultScale on each platform; having its usage prefed off by default will make it easier to implement GetDefaultScale incrementally.
Attachment #420228 - Flags: superreview?(dbaron)
Attachment #420228 - Flags: review?(vladimir)
Created attachment 420229 [details] [diff] [review]
Part 2: Fetch DPI from the system

Adds code to implement nsIWidget::GetDPI on Mac, Windows and X.
Attachment #420229 - Flags: review?(jmathies)
Attachment #420229 - Flags: review?(joshmoz)
Attachment #420229 - Flags: review?(karlt)
Created attachment 420231 [details] [diff] [review]
Part 3: remove gfxPlatform::GetDPI

Remove gfxPlatform::GetDPI() now that it's no longer needed. For some Pango/GTK/Qt font stuff we still need some notion of DPI, but I believe this is really looking for "preferred font size" data. Anyway, I'm leaving behaviour unchanged there; Qt just uses 96 (since gfxQtPlatform doesn't implement InitDisplayCaps) and for GTK, gfxPlatformGtk::GetDPI does what gfxPlatformGtk::InitDisplayCaps used to do.
Attachment #420231 - Flags: review?(vladimir)
Created attachment 420233 [details] [diff] [review]
Part 4: Make pt a non-physical unit

Also does some refactoring. I guess there is still some question about whether we should do this. I think we should.
Attachment #420233 - Flags: review?(dbaron)
Created attachment 420234 [details] [diff] [review]
Part 5: Make picas a non-physical unit

There is also some question about whether we should do this.
Attachment #420234 - Flags: review?(dbaron)
I think the above set of patches can land together to leave us in a good, shippable state, one that we can build on cleanly.

Supporting stuff like Windows' "font DPI" settings will require more work. I will file followup bugs to implement GetDefaultScale() on each platform, and also to add a media query feature to detect the device-pixels-per-CSS-pixel ratio. The latter is needed so that themes can offer artwork customized to particular resolutions, which we'll want before we enable scaling by default.
Comment on attachment 420228 [details] [diff] [review]
Part 1: Create new DPI APIs in nsIWidget

>+     * Return the physical DPI of the screen containing the window ...
>+     * the number of device pixels per inch.
>+     */
>+    virtual PRInt32 GetDPI() = 0;

Shouldn't this return double (or float)?
Comment on attachment 420229 [details] [diff] [review]
Part 2: Fetch DPI from the system

>+    Display *dpy = GDK_DISPLAY();
>+    int defaultScreen = 0;

= DefaultScreen(dpy);

>+    return NS_lround(DisplayHeight(dpy, defaultScreen)/heightInches);

Rounding may be significant with projectors, and a double return value gives a better hint as to what the function is calculating.
Attachment #420229 - Flags: review?(karlt) → review+
I'll fix.
Created attachment 420243 [details] [diff] [review]
Part 6: switch DPI dynamically when a window moves across screens on Mac

This is basically just to annoy Vlad. MacOS makes it really easy to be notified when a window moves across screens, so take advantage of that and switch the DPI dynamically. The system chooses the screen that contains the most window area. The notification also fires when you change the screen's mode, which is nice.
Attachment #420243 - Flags: review?(dbaron)
Created attachment 420245 [details] [diff] [review]
Part 1 v2

Made GetDPI return float.
Attachment #420228 - Attachment is obsolete: true
Attachment #420245 - Flags: superreview?(dbaron)
Attachment #420245 - Flags: review?(vladimir)
Attachment #420228 - Flags: superreview?(dbaron)
Attachment #420228 - Flags: review?(vladimir)
Created attachment 420246 [details] [diff] [review]
Part 2 v2

Make GetDPI return float and address Karl's comments
Attachment #420229 - Attachment is obsolete: true
Attachment #420246 - Flags: review?(jmathies)
Attachment #420229 - Flags: review?(joshmoz)
Attachment #420229 - Flags: review?(jmathies)
Attachment #420246 - Flags: review?(joshmoz)
Created attachment 420249 [details] [diff] [review]
Part 7: Tests

These tests check that pc/pt don't vary with DPI, and that mm/cm/in do vary (correctly) with DPI.
Note to self: We have a patch in bug 524919 that creates the true DPI for Qt. We could update attachment 420246 [details] [diff] [review] or land a followup:

https://bugzilla.mozilla.org/attachment.cgi?id=414047&action=edit
(In reply to comment #12)
> Created an attachment (id=420243) [details]
> Part 6: switch DPI dynamically when a window moves across screens on Mac
> 
> This is basically just to annoy Vlad. MacOS makes it really easy to be notified
> when a window moves across screens, so take advantage of that and switch the
> DPI dynamically. The system chooses the screen that contains the most window
> area. The notification also fires when you change the screen's mode, which is
> nice.

Grumble. I still think that's going to annoy users, especially if it's the default -- do any other mac apps behave this way?
Comment on attachment 420231 [details] [diff] [review]
Part 3: remove gfxPlatform::GetDPI

Looks fine, though I guess the Gtk bit is just a temporary workaround until we figure out the right way to do this with the crazy per-screen dpi stuff?

I wonder if it would be better to move the GetDPI call into gfxPangoFonts to make sure nothing else starts using it accidentally.  It's fine either way, though.
Attachment #420231 - Flags: review?(vladimir) → review+
In cases where it straddles, I don't think we should change the DPI unless the entire window has moved from one screen DPI to the other, or at least the vast majority (like, more than 85% of the window area). If I toss a window aside for a second in order to see something, I don't want my font sizes changing on me.
Attachment #420245 - Flags: review?(vladimir) → review+

Updated

8 years ago
Attachment #420246 - Flags: review?(joshmoz) → review+

Comment 20

8 years ago
Comment on attachment 420246 [details] [diff] [review]
Part 2 v2

>+  return (heightPx/scaleFactor)/(heightMM/25.4f);

You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3 times. Maybe do the same for 96.0f.

I prefer space around operators, "x / y" instead of "x/y", that's how we've tried to do it everywhere else in Cocoa widgets. I think it is easier to read.
(In reply to comment #18)
> (From update of attachment 420231 [details] [diff] [review])
> Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> figure out the right way to do this with the crazy per-screen dpi stuff?

I dunno, there may be no way to do this right on X.

But picking an arbitrary screen and using its DPI for all windows is at least no worse from what we were doing before with a global DPI in gfxPlatform.
(In reply to comment #17)
> Grumble. I still think that's going to annoy users, especially if it's the
> default -- do any other mac apps behave this way?

I don't know of any other Mac apps that use physical units.

(In reply to comment #19)
> In cases where it straddles, I don't think we should change the DPI unless the
> entire window has moved from one screen DPI to the other, or at least the vast
> majority (like, more than 85% of the window area). If I toss a window aside for
> a second in order to see something, I don't want my font sizes changing on me.

This only affects CSS dimensions specified in 'mm', 'cm' and 'in'. They are hardly ever used, so users aren't going to notice in general. Nor will the differences be very noticeable if your screens have similar DPI. If the DPIs are very different you'll probably be constantly annoyed that graphics spanning the screen boundary don't line up. The cases where this might be annoying are tiny tiny edge cases IMHO.

(In reply to comment #20)
> (From update of attachment 420246 [details] [diff] [review])
> >+  return (heightPx/scaleFactor)/(heightMM/25.4f);
> 
> You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3
> times. Maybe do the same for 96.0f.
> 
> I prefer space around operators, "x / y" instead of "x/y", that's how we've
> tried to do it everywhere else in Cocoa widgets. I think it is easier to read.

OK.
(In reply to comment #21)
> (In reply to comment #18)
> > Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> > figure out the right way to do this with the crazy per-screen dpi stuff?
> 
> I dunno, there may be no way to do this right on X.
> 
> But picking an arbitrary screen and using its DPI for all windows is at least
> no worse from what we were doing before with a global DPI in gfxPlatform.

gfxPlatformGtk::GetDPI is a logical font dpi (just for converting Pango-points/pixels).  There is only one value for all monitors on the Screen.
(Mozilla only runs on one Screen.)

nsIWidget::GetDPI is the physical dpi.  It looks like that information would be available for each monitor through the RANDR extension (newer versions at least), but the existing info from the entire Screen is much simpler to use and is a good start.
(In reply to comment #20)
> (From update of attachment 420246 [details] [diff] [review])
> >+  return (heightPx/scaleFactor)/(heightMM/25.4f);
> 
> You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3
> times. Maybe do the same for 96.0f.

I'll add MM_PER_INCH_FLOAT to nsCoord.h. I won't do anything for 96.0f, since it's an arbitrary default and there's no real reason it needs to be consistent across widget implementations.

> I prefer space around operators, "x / y" instead of "x/y", that's how we've
> tried to do it everywhere else in Cocoa widgets. I think it is easier to read.

Will do.
Created attachment 420410 [details] [diff] [review]
Part 8: Define MM_PER_INCH_FLOAT and use it

Amusingly, this fixes a bug in SVG.
Attachment #420410 - Flags: review?(dbaron)
Created attachment 420411 [details] [diff] [review]
Part 9: Fix accessibility

Accessibility tests failed because accessibility's font-size text attribute returns values in pt, and accessibility was using the screen DPI to compute the pt, which no longer matches CSS pt when pt is non-physical. This patch fixes accessibility to report pt defined as 4/3 of a CSS pixel. I think this makes more sense for accessibility anyway; it doesn't make much sense to me for the font size reported by accessibility to depend on the DPI of the user's screen, especially if the user can't see the screen...
Attachment #420411 - Flags: review?(surkov.alexander)
Created attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

Some print reftests failed because they make assumptions about pts and inches. It's not easy to remove these assumptions because page dimensions and margins are defined in terms of inches and twips. So this patch just forces the DPI to 96 in DocumentViewerImpl::SetPageMode.
Attachment #420412 - Flags: review?(dbaron)
Created attachment 420413 [details] [diff] [review]
Part 11: fix other tests that ssume a relationship between inches and pt

Some other tests (mostly written by fantasai :-) ) assume there are always 72 points per inch. This changes those tests to use pt everywhere instead of inches, except for test_unit_length where we just temporarily force the DPI to 96.

Updated

8 years ago
Attachment #420411 - Flags: review?(surkov.alexander) → review+
We probably need to do something to ensure that pt and pc have their correct physical interpretation when printing. I guess we should ensure that px is exactly 1/96 of an inch.
Attachment #420412 - Flags: review?(dbaron)
Comment on attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

This may not be necessary depending on how the DPI discussion evolves.
There's an evolving discussion on www-style about what we should do here.

Partly thanks to Jonathan, I'm coming around to the view that we should redefine all existing units to be "non-physical" in non-print contexts, and create a new unit (say, "truemm") to be a physical unit in all contexts. This would let authors use in, cm, etc to get the correct dimensions when printed but still be usable on a variety of screens.

Updated

8 years ago
Attachment #420246 - Flags: review?(jmathies) → review+
The discussion on www-style seems to have mostly converged on redefining all existing units to be non-physical for non-print media.

It's less clear whether there will be a true physical unit or what it would be called. Since we have a clear use-case for chrome, I suggest we create our own new unit "mozmm".
Comment on attachment 420410 [details] [diff] [review]
Part 8: Define MM_PER_INCH_FLOAT and use it

I suppose the loss in precision in the places operating on doubles isn't a real problem?

(Are there any tests about those places in SVG code that were using 24.4 instead of 25.4 for 'pc' units?)

r=dbaron
Attachment #420410 - Flags: review?(dbaron) → review+
David, does the plan in comment #32 sounds OK to you? If so, I'll implement it. That would obsolete parts 4 and 5, and make part 11 unnecessary.
Comment on attachment 420245 [details] [diff] [review]
Part 1 v2

sr=dbaron
Attachment #420245 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 420243 [details] [diff] [review]
Part 6: switch DPI dynamically when a window moves across screens on Mac

r=dbaron
Attachment #420243 - Flags: review?(dbaron) → review+
Comment on attachment 420234 [details] [diff] [review]
Part 5: Make picas a non-physical unit

r=dbaron
Attachment #420234 - Flags: review?(dbaron) → review+
Attachment #420233 - Flags: review?(dbaron) → review+
Comment on attachment 420233 [details] [diff] [review]
Part 4: Make pt a non-physical unit

>+nscoord nsCSSValue::GetPixelLength() const
>+  case eCSSUnit_Point:
>+    return nsPresContext::CSSPixelsToAppUnits(mValue.mFloat*4/3);

Maybe "* (4.0/3.0)" instead of "*4/3"?

>   PRBool    IsRelativeLengthUnit() const  
>-    { return eCSSUnit_EM <= mUnit && mUnit <= eCSSUnit_Pixel; }
>+    { return eCSSUnit_EM <= mUnit && mUnit <= eCSSUnit_RootEM; }
>+  PRBool    IsPixelLengthUnit() const
>+    { return eCSSUnit_Point <= mUnit && mUnit <= eCSSUnit_Pixel; }

Add a comment that what the spec calls relative length units is, for us, split between relative length units and pixel length units.  I like the new terminology better, but we should have a comment saying that it's different from CSS 2.1.

>-    zoom = aFontData.mSize.IsFixedLengthUnit() ||
>-           aFontData.mSize.GetUnit() == eCSSUnit_Pixel;
>+    zoom = aFontData.mSize.IsLengthUnit() &&
>+      !aFontData.mSize.IsRelativeLengthUnit();

Please keep the indentation style matching the surrounding code.

r=dbaron with that
The CSS WG seems very against breaking the relationships between pt/pc/in/cm/mm.  The current proposal in the WG seems to be to require that 3pt == 4px (i.e., 96px == 72pt == 1in), and the UA can decide whether to peg the whole set of units to physical units or display pixels.

Updated

7 years ago
Duplicate of this bug: 573359

Updated

7 years ago
Duplicate of this bug: 555665
Comment on attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

Not needed now that we're going to make inches and points a fixed multiple of CSS pixels on screen
Attachment #420412 - Attachment is obsolete: true
Comment on attachment 420413 [details] [diff] [review]
Part 11: fix other tests that ssume a relationship between inches and pt

Not needed now that we're going to make inches and points a fixed multiple of CSS pixels on screen
Attachment #420413 - Attachment is obsolete: true
Created attachment 454482 [details] [diff] [review]
Part 4 v2

Addressed review comments. You might want to reread the comments in nsCSSValue.h.

As in the previous version of the patch, this patch just makes points be tied to CSS pixels instead of physical, and refactors code a bit to make that easier. I will attach a new patch to make all the other current "physical" units also be tied to CSS pixels.
Attachment #420233 - Attachment is obsolete: true
Attachment #454482 - Flags: review?(dbaron)
Created attachment 454483 [details] [diff] [review]
Part 4.5: Create mozmm physical unit

Mobile people need this.
Attachment #454483 - Flags: review?(dbaron)
Created attachment 454484 [details] [diff] [review]
Part 5 v2: make pc, in, cm and mm all fixed multiples of CSS pixels

As previously discussed, this makes all the CSS standard "physical" units be fixed multiples of CSS pixels, leaving only mozmm as an actual physical unit.
Attachment #454484 - Flags: review?(dbaron)
Whiteboard: [needs review]
I've also updated the other patches to trunk and updated part 7, the tests. I'll reattach that for informational purposes.
Created attachment 454485 [details] [diff] [review]
Part 7 v2: Updated tests
Attachment #420234 - Attachment is obsolete: true
Attachment #420249 - Attachment is obsolete: true
Created attachment 454826 [details] [diff] [review]
Part 10: Fix nsPresContext and nsIDeviceContext unit-conversion methods to be explicit about using CSS logical units instead of physical units

Running tests I found one rather large issue I'd overlooked: nsPresContext has methods for converting between appunits and twips/inches/points, and they were still using physical units, which is mostly not what we want.

This patch renames those methods to CSSPointsToAppUnits, etc, and makes them static since they no longer depend on the device. This actually simplifies a some SVG code quite a bit.
Attachment #454826 - Flags: review?(dbaron)
Note that part 10 converts all the use of twips/inches for print margins etc to use CSS twips/inches instead of physical units. I think that's fine since for actual printing, CSS units match their physical counterparts.
Comment on attachment 454482 [details] [diff] [review]
Part 4 v2

I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
round anymore.  I'm inclined to say you should go through all the users
that aren't passing the result to nsPresContext::TwipsToAppUnits and
either (a) add the rounding or (b) convert to floats, or that you add a
new function for floats (overloading has serious danger of compilation
errors, though) for those callers that do use TwipsToAppUnits.  If you 
want to do the go-through-all-the-users, I'd prefer to see that as an 
additional patch (on top?).

Other than that this looks fine.
Attachment #454483 - Flags: review?(dbaron) → review+
Comment on attachment 454484 [details] [diff] [review]
Part 5 v2: make pc, in, cm and mm all fixed multiples of CSS pixels

>+  eCSSUnit_Pica         = 904,    // (float) 12 points == 16 CSS pixls

s/pixls/pixels/

r=dbaron
Attachment #454484 - Flags: review?(dbaron) → review+
Comment on attachment 454826 [details] [diff] [review]
Part 10: Fix nsPresContext and nsIDeviceContext unit-conversion methods to be explicit about using CSS logical units instead of physical units

I'm having a little trouble understanding the changes in 
nsThebesDeviceContext::CalcPrintingSize.  Are CSS and physical units 
interchangable in that case, since it's for printing?

I'm guessing it's also the case in the DocumentViewerImpl::InitInternal
changes, but there using CSS makes sense since it'll be faster.

r=dbaron
Attachment #454826 - Flags: review?(dbaron) → review+
(In reply to comment #51)
> I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
> the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
> round anymore.  I'm inclined to say you should go through all the users
> that aren't passing the result to nsPresContext::TwipsToAppUnits and
> either (a) add the rounding or (b) convert to floats,

I'll audit the callers to the _TO_TWIPS macros. For NS_TWIPS_TO and NSTwipsToUnits, I've only changed the parameter from an nscoord to a float, so I don't think anything needs to be checked there. Right?

(In reply to comment #53)
> I'm having a little trouble understanding the changes in 
> nsThebesDeviceContext::CalcPrintingSize.  Are CSS and physical units 
> interchangable in that case, since it's for printing?

Yes. I'll add a comment.

> I'm guessing it's also the case in the DocumentViewerImpl::InitInternal
> changes, but there using CSS makes sense since it'll be faster.

Right.
(In reply to comment #54)
> (In reply to comment #51)
> > I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
> > the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
> > round anymore.  I'm inclined to say you should go through all the users
> > that aren't passing the result to nsPresContext::TwipsToAppUnits and
> > either (a) add the rounding or (b) convert to floats,
> 
> I'll audit the callers to the _TO_TWIPS macros. For NS_TWIPS_TO and
> NSTwipsToUnits, I've only changed the parameter from an nscoord to a float, so
> I don't think anything needs to be checked there. Right?

Er, right.  I originally had a comment mentioning only that, and then I changed it for some reason.

So I think you should be looking at all the users of *_TO_TWIPS that pass it to something other than NS_TWIPS_TO_* or nsPresContext::TwipsToAppUnits.

Be careful to use MXR text search rather than identifier search if you're using MXR; otherwise you'll miss the .mm files.
Created attachment 464696 [details] [diff] [review]
Part 11: Audit _TO_TWIPS macro usage

I added NS_POINTS_TO_INT_TWIPS and NS_INCHES_TO_INT_TWIPS and used them in various places that were calling _TO_TWIPS and expecting an integer ... pretty much all related to printing. The other sites were putting the result in a float.
Attachment #464696 - Flags: review?(dbaron)
Attachment #464696 - Attachment is patch: true
Attachment #464696 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 464696 [details] [diff] [review]
Part 11: Audit _TO_TWIPS macro usage

Maybe you want NSToCoordRound rather than NSToIntRound?  They are different.  I'm not sure it matters.

r=dbaron
Attachment #464696 - Flags: review?(dbaron) → review+
Comment on attachment 454482 [details] [diff] [review]
Part 4 v2

r=dbaron given part 11
Attachment #454482 - Flags: review?(dbaron) → review+
I think I want NSToIntRound. NSToCoordRound is optimized but only works on values in the nscoord range.

dbaron, if you agree we should land this for FF4, can you give blanket approval to the patches here? A comment in the bug would suffice.
Whiteboard: [needs review] → [needs approval]
Version: 1.9.2 Branch → Trunk
Yeah, I think we should.  a2.0=dbaron
Whiteboard: [needs approval] → [needs landing]

Comment 61

7 years ago
(In reply to comment #18)
> Comment on attachment 420231 [details] [diff] [review]
> Part 3: remove gfxPlatform::GetDPI
> 
> Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> figure out the right way to do this with the crazy per-screen dpi stuff?

From GDK 2.14 you do have functions for multi-monitor setups:

http://library.gnome.org/devel/gdk/stable/GdkScreen.html

gdk_screen_get_monitor_at_window()
and
gdk_screen_get_monitor_width_mm()

I can't find a signal for when the window's screen changes like on OSX. Maybe you need to listen for when the window moves and check if the monitor number changes.
This seems to have been responsible for test failures, so it was backed out.

MacOSX Debug geolocation test failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281715052.1281717855.27052.gz

MacOSX and MacOSX64 Debug test_popup failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281715052.1281717285.24668.gz

It may also have been responsible for a Txul regression, not sure yet.

I can't reproduce those failures locally, but I can on tryserver. Not sure why I didn't see those failures when I ran these patches through tryserver before. Maybe because Tinderbox was flakey at the time and those results were reported late.

When I get a chance I will try bisecting the patch queue on tryserver. That's not expensive since I can disable all builds except for MacOSX Debug.
Whiteboard: [needs landing]
Keywords: dev-doc-needed
Bisection on try-server shows that the test failures appear to have been caused by part 6, dynamic DPI changing on Mac. That's fine, we can leave that patch out.

Updated

7 years ago
Blocks: 588464

Updated

7 years ago
Depends on: 588627
Depends on: 588664
Depends on: 588977
Relanded a couple of days ago:
http://hg.mozilla.org/mozilla-central/rev/e95f616c9ee9
http://hg.mozilla.org/mozilla-central/rev/a3b0261ab61f
http://hg.mozilla.org/mozilla-central/rev/3569f9c1e13f
http://hg.mozilla.org/mozilla-central/rev/6229ad7196d4
http://hg.mozilla.org/mozilla-central/rev/3e399932a0e4
http://hg.mozilla.org/mozilla-central/rev/3cfc1b1098e6
http://hg.mozilla.org/mozilla-central/rev/49429c0426d1
http://hg.mozilla.org/mozilla-central/rev/46d18f2dabe9
http://hg.mozilla.org/mozilla-central/rev/a9982342b435
http://hg.mozilla.org/mozilla-central/rev/585a75516573
http://hg.mozilla.org/mozilla-central/rev/6991b186ea72

There was a Tp4 regression Mac, which was fixed in bug 588664.
Status: NEW → ASSIGNED
Flags: in-testsuite+

Updated

7 years ago
Depends on: 589002
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Note for docs:

http://weblogs.mozillazine.org/roc/archives/2010/08/css_units_chang.html

Updated

7 years ago
Depends on: 595039
Documented here:

https://developer.mozilla.org/en/CSS-2_Quick_Reference/Units

And linked to from Firefox 4 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Comment 67

7 years ago
(In reply to comment #66)
Good to have that URL, but I'm disappointed to see there "In addition, Gecko has improved ability to correctly scale absolute lengths to the actual resolution of the rendering medium, whether that's the screen or paper." without either an explanation or a link to an explanation.

On every Linux I install, like many others do, I force DPI to be accurate, so I need to know how to make pt mean pt & cm mean cm instead of the perpetuation of the unfortunate legacy explained on http://blogs.msdn.com/fontblog/archive/2005/11/08/490490.aspx that lead to this "bug" and "fix". Until users universally can easily adjust the size of their own px, px values of more than 9 in author CSS should be rejected by all web browsers as invalid, and 96 as a standard should be relegated to history books.

Meanwhile, thanks for mozmm, which keeps my http://fm.no-ip.com/Auth/dpi-screen-window.html tool viable.

Comment 68

6 years ago
I suppose mentioning this here will reach the people who care about DPI issues. Firefox4 renders everything at 96 DPI, no matter what I do, layout.css.dpi is completely ignored.

Tracking the issue down I found that gfx/src/thebes/nsThebesDeviceContext.cpp uses the PR_MAX macro in an unsafe manner, i.e. with function calls as parameters, which leads to very weird effects on my system. The macro substitution causes each function to be called twice and apparently they somehow interfere with each other and set stuff to 0 instead of a sensible value.

The result was that dpi always resulted in 96, no matter what was set in layout.css.dpi.

After fixing the issue I found that thebes has all the correct data on screen resolution and units, but layout.css.dpi still had no effect.

So I went in search for a central change to change this and ended up changing GetPixelLength() in layout/style/nsCSSValue.cpp. The function is hard coded to 96 DPI. I changed the signature to take a presentation context as an argument (the function is only called in 2 place in the entire source, so this wasn't a big deal) and made it use the information provided by thebes.

The result is that everything scales as expected I created a little dpi-meter at http://www.home.hs-karlsruhe.de/~fado0001/testdpi.html and not only does my trusty ruler tell me that the units are rendered perfectly, changing layout.css.dpi adjusts there rendering on the fly. I don't even have to reload the page.

My fix can be found in bug 603880, comment 5. I hope you will review it and take it into consideration.
Thanks for the info. I commented about the PR_MAX issues in bug 603880.

It is intentional that DPI values other than 96 (either automatically detected, or manually forced via layout.css.dpi) have little effect in Firefox 4. See http://weblogs.mozillazine.org/roc/archives/2010/01/css_absolute_le.html and http://weblogs.mozillazine.org/roc/archives/2010/08/css_units_chang.html. Since our new behavior matches the other major browsers and is necessary for many Web pages to be usable on devices like phones, we really have no choice. If you disagree with the decision, that's fine, but please express your disagreement elsewhere, perhaps on my blog, or on your own blog :-).

You can use mozmm if you really want a physical unit. Note that a mozmm-sized element that looks fine on a desktop screen is unlikely to be suitably sized for a phone, say, so the use-cases for mozmm are quite limited.

Updated

6 years ago
Duplicate of this bug: 507201

Comment 71

6 years ago
I cannot understand that comment, Robert.

> It is intentional that DPI values other than 96 have little effect in Firefox 4.

No it is not. From your second link: "Currently GetDefaultScale always returns 1.0 on all platforms, although on Mac we should set it to the system 'default UI scale' and on Windows we should set it based on the 'system font DPI'."

> our new behavior matches the other major browsers

No it does not. Increase your DPI and Windows and test Chrome, Firefox and IE. Chrome and IE scale things up correctly, while Firefox does not.

> and is necessary for many Web pages to be usable on devices like phones

I can understand the need for phones to have sizing hacks. But there is no reason to force the same hacks onto the desktop!

> You can use mozmm if you really want a physical unit.

Who said anything about wanting a physical unit? High DPI is used all the time by people with poor eyesight who want things *to appear larger*.

Comment 72

6 years ago
Created attachment 558144 [details]
Firefox 6 at 120 DPI. Note how the browser chrome is correct but the CSS dimensions are wrong
> > our new behavior matches the other major browsers
> 
> No it does not. Increase your DPI and Windows and test Chrome, Firefox and
> IE. Chrome and IE scale things up correctly, while Firefox does not.

Bug 603880 is about automatically scaling when you increase the Windows "font DPI". That is a valid bug. This bug was about changing behavior in response to screen DPI changes. Other browsers do not do that, and we match that now.

> Who said anything about wanting a physical unit? High DPI is used all the
> time by people with poor eyesight who want things *to appear larger*.

Right, you want bug 603880.
You need to log in before you can comment on or make changes to this bug.