Closed Bug 469933 Opened 11 years ago Closed 11 years ago

Default window size is much bigger than its content if the display resolution is high

Categories

(Core :: XUL, defect, P2)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dev-null, Assigned: zwol)

References

Details

(Keywords: fixed1.9.1, mobile, regression)

Attachments

(5 files, 6 obsolete files)

Attached file testcase
Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2a1pre) Gecko/20081216 Minefield/3.2a1pre

Default window size is too big if the resolution is high.
This affects all common dialogs (including alert dialog).

20081116033829 rev:8b3caddca8f5 works fine
20081117040154 rev:30cc5a85b411 fails

Steps to reproduce:
1. Set layout.css.dpi to >= 144 (e.g. 200)
2. Open the testcase with firefox -chrome URL_of_the_testcase

Actual result:
The window size is too big for the contents of the window.

Expcted result:
The window size should fit to the contents of the window.
Too tall for its contents, and clipped by display size.
Too tall for its contents.
Requesting blocking because some dialog become too big for display and make it unable to access whole dialog with high dpi.
Flags: blocking1.9.2?
Flags: blocking1.9.1?
why would this be blocking as we are not setting layout.css.dpi to anything other than 96?
(In reply to comment #4)
> why would this be blocking as we are not setting layout.css.dpi to anything
> other than 96?

If layout.css.dpi is not set, display property of Windows is used.
User may set dpi with Windows' display property.
Or the user might just happen to be running on a high-resolution display...  Given bug 459677, that's even the case for some of our builds. This should block, imo.
Keywords: regression
Should some of these dialogs have their default sizes in ems rather than pixels?  It seems like the surprise here is that the default font size becomes so small in terms of pixels once the DPI jumps up?  I think we should be scaling dialog sizes that are specified in pixels -- for the cases where things were really meant to be a certain size in pixels.  If it's not that these dialogs should be sized in em, what should happen here?  Or is there some other bug I'm not understanding?
(In reply to comment #7)
> Should some of these dialogs have their default sizes in ems rather than
> pixels?  It seems like the surprise here is that the default font size becomes
> so small in terms of pixels once the DPI jumps up?

These are not matter.
The font size in the screenshot is small because I changed DPI but I didn't change system font size.

My intent for this bug is:
1. The size of contents are determined, and the window size is determined based on the size of contents. (which is scaled correctly)
2. Also, the window size is scaled by Bug 459677.
As a result, the window size is scaled twice (by both 1. and 2.).
But should be scaled only once.
What are those windows using to set the size of the window to the contents?  window.sizeToContent()?  Or something else?
(In reply to comment #9)
> What are those windows using to set the size of the window to the contents? 
> window.sizeToContent()?  Or something else?

Nothing is needed to set the size of the window.
Please see the testcase and Steps to reproduce in Comment #0.
(In reply to comment #10)
> > What are those windows using to set the size of the window to the contents? 
> > window.sizeToContent()?  Or something else?
> 
> Nothing is needed to set the size of the window.

But I found that window.sizeToContent() also has same problem.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(In reply to comment #10)
> (In reply to comment #9)
> > What are those windows using to set the size of the window to the contents? 
> > window.sizeToContent()?  Or something else?
> 
> Nothing is needed to set the size of the window.

I found it's in DocumentViewerImpl::SizeToContent().
This might be interesting for Zack to look at.
Should be reproducible on any platform by setting layout.css.dpi.
Assignee: nobody → zweinberg
Attached patch patch (obsolete) — Splinter Review
The problem is pretty straightforward.  DocumentViewerImpl::SizeToContent computes a correct size in device pixels and passes that to nsIDocShellTreeOwner::SizeShellTo.  The implementor of that interface turns out to be nsXULWindow, which does some irrelevant-for-this-discussion calculation and calls nsXULWindow::SetSize.  So SetSize's arguments are in device pixels.  But SetSize assumes its arguments are in CSS pixels, so it converts them to device pixels - effectively applying the dpi-based scaling up again.

The fix *appears* to be simple - just make SetSize not change units - but I have not audited all other callers of that function and would not be surprised if some of them do pass CSS pixels.  (mxr finds 308 instances of the string "setsize"; obviously only a tiny fraction of those are this function; I will actually go through 'em tomorrow if I have to, but it's late here.)
Attachment #359444 - Flags: review?(bzbarsky)
I tend to think SetSize's arguments probably should be in CSS pixels.
(In reply to comment #15)
> Created an attachment (id=359444) [details]
> patch

Is this just backing out Bug 459677?

(In reply to comment #16)
> I tend to think SetSize's arguments probably should be in CSS pixels.

If SetSize's arguments should be in CSS pixels, should GetSize's return values be also in CSS pixels?
(In reply to comment #17)
> (In reply to comment #15)
> > Created an attachment (id=359444) [details] [details]
> > patch
> 
> Is this just backing out Bug 459677?

Boris told me on IRC that that's what I'd done.  Do you know where SetSize is being called from in that bug?

> (In reply to comment #16)
> > I tend to think SetSize's arguments probably should be in CSS pixels.
> 
> If SetSize's arguments should be in CSS pixels, should GetSize's return values
> be also in CSS pixels?

It ought to be consistent, whichever way it works.  It appears to me that most of nsXULWindow pays no attention to units, so it's /de facto/ in device pixels, and I think it would be easier to find the callers that're passing CSS pixels and change them than to change nsXULWindow and then find all the callers that're passing device pixels and change _them_.
Status: NEW → ASSIGNED
I tried to trace back callers of nsXULWindow::SetSize to the point where I could be certain about what unit they were passing.  (There's a lot of callers that just pass along what they were given.)  

nsWindowWatcher::SizeOpenedDocShellItem, CMozillaBrowser::OnSize, nsGlobalWindow::SetInnerWidth, and DocumentViewerImpl::SizeToContent always pass device pixels.

nsGlobalWindow::SetOuterSize passes dev pixels if it has a prescontext, otherwise it passes CSS pixels (I think it can always get a device context, it should use that to do the conversion instead).

There are also a whole bunch of calls (transitively) from classes with "embed" in their names, functions that are just passing along what their callers gave them, but I can't find anything that *uses* those.

(This is all just mxr identifier searches, and it doesn't help that mxr has no concept of filtering by class -- there are a lot of methods named "Size" :-P)

Bug 459677 was about Fennec.  Is it possible to reproduce that bug in a regular old firefox build?  If so, could someone give me a recipe please?
(In reply to comment #19)
> Bug 459677 was about Fennec.  Is it possible to reproduce that bug in a regular
> old firefox build?  If so, could someone give me a recipe please?

Same symptom occurs when just starting old Firefox with high dpi.
When it occurs, SetSize() id called from nsXULWindow::LoadSizeFromXUL(), though I'm not sure it's same code path as Fennec.
(In reply to comment #20)
> When it occurs, SetSize() id called

Sorry, SetSize() *is* called
It's odd that we even allow SetOuterSize on frames.  We shouldn't, just like we don't allow setting inner width/height on frames, right? Given that caveat, I think it's somewhat safe to assume we always have a prescontext.  Probably.

Might be worth a separate bug to disallow it on frames if we allow it right now.

In terms of other issues here... nsXULWindow::LoadPositionFromXUL seems to mix device pixels (gotten from GetPositionAndSize(); I assume that nsIBaseWindow is in device pixels, right?  In any case this is reading stuff from the nsIWidget, so that's device pixels) and CSS pixels (gotten via getAttribute on the root node).

nsXULWindow::LoadSizeFromXUL does the same in terms of mixing GetSize() and the stuff it gets from the node.  I suspect that this is the caller you want, by the way.

nsXULWindow::SavePersistentAttributes saves the device-pixel results it gets in a field expecting CSS pixels (persisted attribute value).  This was raised in bug 459677, but the patch author chose to wait on someone filing a bug on it.  No one ever did.
Bug 471729 covers the persistent attribute issue.
Blocks: 471729
Ok, this revised patch changes nsXULWindow::LoadPositionFromXUL, ::LoadSizeFromXUL, and ::SavePersistentAttributes to convert to/from dev pixels as well as reverting the change to ::SetSize.  This still fixes the test case for this bug, and does not appear to break normal window sizing a la bug 459677.  It should also resolve bug 471729 -- at least, with high dpi set, the browser window does not change size over multiple closes/opens.

There are a whole bunch of nsGlobalWindow methods that punt on conversions like this if they don't have a prescontext -- some of them don't write their outparams at all if they don't have a prescontext (while still returning NS_OK)!  On further reading it doesn't look like nsGlobalWindow has any alternative source of the conversion factor, so I decided not to mess with that.

It is my impression that, except for nsGlobalWindow::SetOuterSize which only screws up if it doesn't have a prescontext, with this patch all callers of nsXULWindow::SetSize now pass device pixels.
Attachment #359444 - Attachment is obsolete: true
Attachment #360140 - Flags: review?(bzbarsky)
Attachment #359444 - Flags: review?(bzbarsky)
(In reply to comment #24)
> There are a whole bunch of nsGlobalWindow methods that punt on conversions like
> this if they don't have a prescontext

Though I don't understand nsGlobalWindow and prescontext issue,
|window.resizeTo()| seems to be in device pixels after this patch but should not it be in CSS pixels?
Does this need a separate bug?
(In reply to comment #25)
>
> Though I don't understand nsGlobalWindow and prescontext issue,
> |window.resizeTo()| seems to be in device pixels after this patch but should
> not it be in CSS pixels?

Bwuh?  Is it?  This patch shouldn't have affected that.
(In reply to comment #26)
> > |window.resizeTo()| seems to be in device pixels after this patch but should
> > not it be in CSS pixels?
> 
> Bwuh?  Is it?  This patch shouldn't have affected that.

window.resizeTo()'s arguments are passed to nsXULWindow::SetSize() without scaling.

Without the patch, window.resezeTo()'s arguments were treated as CSS pixels because they were scaled in SetSize(). 
But with the patch, window.resizeTo()'s arguments are never scaled, therefore they are treated as device pixels.
Actual result without the patch v2:
The size of window after clicking the button is same as the size of green box even if it's in high DPI mode.

Actual result with the patch v2:
The size of window after clicking the button is not same as the size of green box when it's in high DPI mode.
And resizeBy has the same issue.  For that matter, moveTo and moveBy do as well, no?  Trying to recall whether those should be in CSS or dev pixels...
The window.move/resize methods don't seem to be specified in any level of the W3C DOM or in HTML 5.  devmo talks about their arguments being in "pixels" without further qualification.  However, devmo has an example for resizeTo using window.screen.availWidth/availHeight, and those are in CSS pixels (by code inspection).  It seems to me that having all these interfaces be CSS pixels would be least surprising for developers.  I will rev my patch to do that.
Hm, are we reaching the point where it would be less painful to have SetSize/SetPosition/GetSize/GetPosition do the conversion from CSS to dev pixels?  It would eliminate the problem with nsGlobalWindow not always being able to do the conversion, but it would also mean touching a whole bunch of lower-level platform code sitting between these methods and the OS, which expects to do its business in device pixels and maybe cannot do the conversion.

I suppose we could also add (Set|Get)(Position|Size)CSS methods to nsIBaseWindow, but that feels very invasive for this point in the cycle.
nsIBaseWindow is de-facto frozen.  We cannot change its signature, and we really shouldn't have changed its behavior in bug 459677.

We should document that it works in device pixels, and fix callers as needed.
OK.  As long as you're here, *is* there a better way for nsGlobalWindow methods to do this conversion than "if prescontext available, use that, otherwise assume 1:1"?  There are a lot of nsGlobalWindow methods that need this.
I think for a non-frame if there's no prescontext available making the methods be no-ops is as good as anything.  We shouldn't be hitting that case, imo.
(In reply to comment #32)
> nsIBaseWindow is de-facto frozen.  We cannot change its signature, and we
> really shouldn't have changed its behavior in bug 459677.

In the past, we didn't distinguish between device and CSS pixels.  When we started distinguishing, we needed to pick one side or the other.  Either way is a change in behavior.  If the callers are more likely to use it based on what they're putting inside the window, we should use CSS pixels; if they're more likely to use it based on the context in which they want the window, we should use device pixels.
This patch passes the window.resizeTo test case in attachment 360261 [details].  I am running mochitests with layout.css.dpi bumped to 192, but I won't know the results until I get back from campus late this evening.

As for the "which units should nsIBaseWindow methods take" question I'm leaning toward device pixels because (as I said above) some callers are mediating between nsIBaseWindow and the operating system, so they naturally get device pixels and I don't think they have access to the conversion factor.  For instance, CMozillaBrowser::OnSize is like this, and so are the analogous methods for other windowing systems.
Attachment #360140 - Attachment is obsolete: true
Attachment #360368 - Flags: superreview?(dbaron)
Attachment #360368 - Flags: review?(bzbarsky)
Attachment #360140 - Flags: review?(bzbarsky)
Attached patch Patch v3.1 (obsolete) — Splinter Review
Almost same as patch v3 but I've changed |*aScreenY = DevToCSSIntPixels(x);| to |*aScreenY = DevToCSSIntPixels(y);|.
Attachment #360368 - Attachment is obsolete: true
Attachment #360407 - Flags: superreview?(dbaron)
Attachment #360407 - Flags: review?(bzbarsky)
Attachment #360368 - Flags: superreview?(dbaron)
Attachment #360368 - Flags: review?(bzbarsky)
Doh! Thank you for fixing it.
Comment on attachment 360407 [details] [diff] [review]
Patch v3.1

r+sr=bzbarsky.  Please get followup bugs filed on those methods that aren't checking IsFrame(), ok?
Attachment #360407 - Flags: superreview?(dbaron)
Attachment #360407 - Flags: superreview+
Attachment #360407 - Flags: review?(bzbarsky)
Attachment #360407 - Flags: review+
After the patch v3.1, in nsXULWindow::LoadSizeFromXUL(),
there is inconsistency that spec{Width,Height} is in device pixels but screen{Width,Height} is in CSS pixels, isn't it?
Yes, you're right.  We need a couple more conversions in there.

I don't see any other places where this is wrong, but I'm not batting very accurately here, it seems.

Would also welcome suggestions for how to automate testing of this stuff.
Attachment #360407 - Attachment is obsolete: true
Attachment #360542 - Flags: superreview+
Attachment #360542 - Flags: review?
Attachment #360542 - Flags: review? → review?(dev-null)
Is dev-null the intended reviewer? :-)
(In reply to comment #42)
> Is dev-null the intended reviewer? :-)

Yah.  I'm taking Boris' r/sr+ as approval of the general plan, but Atsushi seems to be consistently finding errors, so it's a request for him to keep looking. :)

I'd be glad to hear any comments you have, too.
Keywords: mobile
Attachment #360542 - Flags: review?(dev-null) → review-
Comment on attachment 360542 [details] [diff] [review]
v3.2: more conversions in LoadSizeFromXUL

Am I the reviewer!?
I've tested v3.2 but it does not work as expected.

- In nsGlobalWindow::CheckSecurityLeftAndTop(), inconsistency still remains. It calculates dimensions in CSS pixels while the caller is in device pixels.
- In nsGlobalWindow::CheckSecurityWidthAndHeight(), it ensures the size is not smaller than 100px. I'm not sure but shouldn't it be in CSS pixels?
- Don't forget merge v3.1, i.e. |*aScreenY = DevToCSSIntPixels(x);| should be fixed!
(In reply to comment #44)
> (From update of attachment 360542 [details] [diff] [review])
> Am I the reviewer!?

Yes, that was intended.  See comment #43.

> I've tested v3.2 but it does not work as expected.
> 
> - In nsGlobalWindow::CheckSecurityLeftAndTop(), inconsistency still remains. It
> calculates dimensions in CSS pixels while the caller is in device pixels.

Will fix.

> - Don't forget merge v3.1, i.e. |*aScreenY = DevToCSSIntPixels(x);| should be
> fixed!

Oops.  Will fix.

I would appreciate it if you could look for other remaining problems so that we don't have to go round on this too many more times.

> - In nsGlobalWindow::CheckSecurityWidthAndHeight(), it ensures the size is not
> smaller than 100px. I'm not sure but shouldn't it be in CSS pixels?

I can make a case either way:

 - if the check is in CSS pixels it would ensure that a window could
   never be smaller than a particular absolute size, which is what users
   care about (I am assuming the point of this check is to make sure the
   window can't be so small that the user cannot manipulate it)
 - but on a small, high-resolution screen, "too small" might be more reasonably
   expressed as a device pixel constraint, not a CSS pixel constraint.

Anyone else have an opinion?
I looked at the CheckSecurity* functions and it makes a lot more sense for them to just do all their business in CSS pixels.  A couple of callers need to do extra conversions, but if we did it the other way, we'd be converting in CheckSecurity on every single call.  (CheckSecurityLeftAndTop always has to convert from what GetPositionAndSize gives it to CSS pixels, but the other way around it would always have to convert what nsIDOMScreen gives it, so that's a wash.)

So here's a revision that does that, and also incorporates Atsushi's typo fix.
Attachment #360542 - Attachment is obsolete: true
Attachment #360634 - Flags: superreview+
Attachment #360634 - Flags: review?(dev-null)
Comment on attachment 360634 [details] [diff] [review]
v4: merge v3.1, go back to CSS units for CheckSecurity*

Works fine for me. Great!

Just wondering one thing;
In nsXULWindow::SavePersistentAttributes(),
why is appPerDev float, not PRInt32?
GCC warns about passing float appPerDev (and implicitly casted) to DevToCSSPixels(), whose argument is PRInt32.
Attachment #360634 - Flags: review?(dev-null) → review+
DevToCSSPixels() should have a float arg.
(In reply to comment #48)
> DevToCSSPixels() should have a float arg.

> inline nscoord NSIntPixelsToAppUnits(PRInt32 aPixels, PRInt32 aAppUnitsPerPixel);

> class nsIDeviceContext : public nsISupports
> {
>   PRInt32 AppUnitsPerDevPixel() const { return mAppUnitsPerDevPixel; }
> }

> static PRInt32
> DevToCSSPixels(PRInt32 aPixels, PRInt32 aAppPerDev)
> {
>   return nsPresContext::AppUnitsToIntCSSPixels(
>     NSIntPixelsToAppUnits(aPixels, aAppPerDev));
> }

>   float appPerDev = mWindow->GetDeviceContext()->AppUnitsPerDevPixel();
>       PR_snprintf(sizeBuf, sizeof(sizeBuf), "%ld",
>                   (long)DevToCSSPixels(x, appPerDev));

- AppUnitsPerDevPixel() returns PRInt32.
- NSIntPixelsToAppUnits() has PRInt32 args.

Why should we cast it to float in the middle?
It's odd that those are integers.  I'd think a dev pixel is smaller than an app unit when printing, say...
When printing, we don't really need or have direct access to a "dev pixel" value. So we choose an appunits per dev pixel ratio that's convenient for us. A number less than 1 is never convenient so we don't do that :-).

[Ideally we would probably just choose 1 so that rounding to "device pixels" is a no-op. But right now our theme code likes to draw in device pixels and if we make a printing device pixel too small, themed widgets look wrong, especially themed borders are too thin.]
OK, sounds like that ratio should just be an int here too, then.
I'm confused; what change are y'all saying should be made to the patch?
(In reply to comment #53)
> what change are y'all saying should be made to the patch?

The variable appPerDev in nsXULWindow::SavePersistentAttributes() should be PRInt32, not float.
Whiteboard: [needs revised patch]
Here is the revised patch.

For the record I used float originally because NSAppUnitsToIntPixels() takes a float aAppUnitsPerPixel argument.  But the converse function (NSIntPixelsToAppUnits) takes a PRInt32 for the conversion factor.  *shrug*
Attachment #360634 - Attachment is obsolete: true
Attachment #361358 - Flags: superreview+
Attachment #361358 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs revised patch] → [needs landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/a71056f6ff2e

It would be nice to have some tests here. But I don't know if it's a good idea to temporarily set layout.css.dpi during mochitests.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
(In reply to comment #39)
> Please get followup bugs filed on those methods that aren't
> checking IsFrame(), ok?

The followup bug is needed but I don't understand this issue.
Anyone can file a bug for that?
Or, anyone can tell me the summary and the component?
> The followup bug is needed but I don't understand this issue.
> Anyone can file a bug for that?

I don't clearly understand what Boris was talking about, myself.
Some of the window-resizing methods seem to not check what sort of window the call is made on.  They unconditionally resize the toplevel window.  So if you make a SetOuterSize call on a subframe, you will resize the whole window instead.  That seems quite odd to me.

The bug should be to investigate all the size-changing methods of nsGlobalWindow, and if any of them should work on subframes (which I doubt) to clearly document that in the code.
(In reply to comment #59)
> Some of the window-resizing methods seem to not check what sort of window the
> call is made on.

Filed Bug 477974.
Please correct it if it's wrong.
Comment on attachment 361358 [details] [diff] [review]
v5: PRInt32 instead of float for conversion factor

>@@ -1040,30 +1050,31 @@ PRBool nsXULWindow::LoadPositionFromXUL(
>   PRInt32 temp;
> 
>   GetPositionAndSize(&currX, &currY, &currWidth, &currHeight);
> 
>   // Obtain the position information from the <xul:window> element.
>   PRInt32 specX = currX;
>   PRInt32 specY = currY;
>   nsAutoString posString;
>+  PRInt32 appPerDev = mWindow->GetDeviceContext()->AppUnitsPerDevPixel();
Based on crash-stats.m.c this added a new topcrasher.
For example
http://crash-stats.mozilla.com/report/index/316433f1-da06-4365-aeac-839c02090304
Depends on: 482687
Depends on: 495219
You need to log in before you can comment on or make changes to this bug.