Last Comment Bug 493428 - Mail/News Account Settings far too narrow on OS/2
: Mail/News Account Settings far too narrow on OS/2
: regression, verified1.9.1.1
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 OS/2
: -- major (vote)
: ---
Assigned To: Peter Weilbacher
Depends on: 490561
Blocks: 317659
  Show dependency treegraph
Reported: 2009-05-16 23:09 PDT by Peter Weilbacher
Modified: 2009-07-14 09:24 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

screenshot (17.56 KB, image/png)
2009-05-16 23:09 PDT, Peter Weilbacher
no flags Details

Description Peter Weilbacher 2009-05-16 23:09:23 PDT
Created attachment 377908 [details]

As noted in the newsgroup by Peter Brown, the Mail/News Account Settings panel in SeaMonkey is far too narrow on OS/2. This regressed some time between 20090422135858 and 20090514112020. 

In comm-central, the most likely change that may have caused this is bug 317659. Either the new size is generally wrong for OS/2 (maybe because the default system font just has a weird zero width), then we need to special-case the values in AccountManager.dtd. Or we do something wrong when determining the "ch" unit (that would point to a problem in gfxOS2Fonts::GetMetrics).
Comment 1 Peter Weilbacher 2009-05-16 23:13:09 PDT
Need to look at GetMetrics() anyway for bug 490561.
Comment 2 Robert Kaiser 2009-05-17 12:56:57 PDT
I'm pretty sure this is a problem in determining to ch unit, and 1ch should be some approximation of the average width of a character, which makes it ideal for dialog width specifications and that's why we're switching to that in many places.
Fixing it for account manager specifically would just defer the problem to the next dialog that is being switched to use the "correct" unit for widths, which is ch.
Comment 3 Peter Weilbacher 2009-05-17 15:11:52 PDT
OK, the metrics for the 'ch' unit were implemented in bug 363706; 'ch' uses the zeroOrAveCharWidth member of the metrics structure. AFAICS it is correctly derived on OS/2. For the standard Workplace Sans font (of size 15px) I get e.g.


If I artificially increase zeroOrAveCharWidth to 1.5x its natural size (i.e. 7.312500 in the example above), I do get a reasonably sized panel.
Forcing zeroOrAveCharWidth to be at least aveCharWidth is not enough. I don't see any reason to have to do either.
Comment 4 Peter Weilbacher 2009-06-12 04:26:48 PDT
I compared this to a SM build on Linux where the fonts involved are Vera and VeraBd. There, LockedFTFace::GetMetrics computes
The horizontal values are quite a bit larger than on OS/2. I'll try to install the same fonts on OS/2 to see what I get there.

Perhaps there is a mistake in the horizontal scaling from font units to pixels. (But then, why don't I see any problems with that in other text display?!).
Comment 5 Peter Weilbacher 2009-06-12 13:32:35 PDT
On OS/2 I didn't manage to get the exact same pixel size when setting Vera as standard dialog font, but I get either this
or this

While this shows some minor differences, the dialog is wide enough. So the problem could really be with the Workplace Sans font, although I don't know what that could be. Perhaps I should try vice-versa to set up Workplace Sans on Linux to check that.
Comment 6 Peter Weilbacher 2009-06-15 08:26:39 PDT
OK, I set up Workplace Sans 9 (pt? GNOME doesn't give units) as default Application font in the GNOME Appearance settings app under Linux. That resulted in 

So indeed the OS/2 values (comment 3) for this font come out a bit weird, and a lot narrower than on Linux.
Comment 7 Peter Weilbacher 2009-07-09 14:34:43 PDT
OK, so the difference is that OS/2 uses the glyph's width (of '0') to determine zeroOrAveCharWidth. I think this is correct, because the CSS3 working draft says
   ch: The width of the "0" (ZERO, U+0030) glyph found in the font
       for the font size used to render.

On Linux, the x_advance is used which is larger because it takes into account offsets between characters in a text layout, same for Windows with GetTextExtentPoint32W() and the implementation in gfxAtsuiFonts.

I could simply change the OS/2 implementation to use metrics.horiAdvance instead of metrics.width, but given the CSS spec I don't think that's correct.

Zack, any comments on that?
Comment 8 Zack Weinberg (:zwol) 2009-07-09 15:20:50 PDT
The intended use of 'ch' is for sizing boxes containing fixed width text.  For instance, a box styled with { width:80ch; font-face:monospace } should be able to hold 80 columns of monospace characters.  Thus I'm inclined to say that one should use the advance width of '0' rather than the glyph width (so Linux/Win/Mac are correct and OS/2 is wrong).

David, what do you think?
Comment 9 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-07-09 15:33:41 PDT
Yeah, it should be the advance width.
Comment 10 Peter Weilbacher 2009-07-10 00:14:06 PDT
OK, then. I'll fix OS/2. Do you have influence on the CSS3 spec so that this can be clarified in their text?
Comment 11 Peter Weilbacher 2009-07-10 00:16:42 PDT
(In reply to comment #8)
> The intended use of 'ch' is for sizing boxes containing fixed width text.

Hmm, one could use this to argue that using 'ch' to size dialogs (which on most platforms contain non-monospace text) is misusing that unit. ;-)
Comment 12 Peter Weilbacher 2009-07-10 01:00:02 PDT
OK, I pushed the fix:

Other similar changes may have to be done for the width calculation of other metrics, but I'll postpone that to bug 490561.
Comment 13 Robert Kaiser 2009-07-10 05:40:40 PDT
(In reply to comment #11)
> Hmm, one could use this to argue that using 'ch' to size dialogs (which on most
> platforms contain non-monospace text) is misusing that unit. ;-)

That would bring the question up of what the correct use for that unit is - one would guess that the intended use is actually to make boxes wide enough to hold a certain amount of characters in that font.
Comment 14 Peter Weilbacher 2009-07-10 05:55:21 PDT
But you can't guarantee that if the font is not monospaced. If you fill a box with "width: 50ch" with 50 'm' characters (which in many fonts is wider than '0') then it is pretty sure that the text overflows that box. If you fill the same box with 50 'l' chars, then it's very likely that the box is much wider than that text.
Comment 15 Robert Kaiser 2009-07-10 06:09:25 PDT
Right, but you're talking about extreme cases, and I'm talking about the average casee. Nobody says that any text with 50 characters needs to fit into a 50ch box, but if a text fits into a 50ch box with some margin in one font, the chance is pretty high that it will fit in a 50ch box in other fonts. And I think that's the actual intention of that unit.
Comment 16 Peter Weilbacher 2009-07-10 06:56:37 PDT
If that is indeed the intent (searching around I found nothing to support your point), then you are right that it is useful for dialog sizing. But then it would not be very useful on the web. Because there one wants to separate style from content and not resize boxes when the text changes.

I guess this discussion (and an even longer one in bug 317659) just shows that 'ch' is underspecified and the spec (css3-values) should be clarified. Both in terms of implementation and intended use of the unit.
Comment 17 Robert Kaiser 2009-07-10 07:09:36 PDT
I don't have influence on any spec, I'm just an observer and project manager.
Not sure about Zack, but surely David actually is one of those people who can bring up clarification of the spec.

(In reply to comment #16)
> If that is indeed the intent (searching around I found nothing to support your
> point), then you are right that it is useful for dialog sizing. But then it
> would not be very useful on the web. Because there one wants to separate style
> from content and not resize boxes when the text changes.

I disagree that it's not useful, as I've done a lot of website designing where I wanted exactly a unit that does this and went with em even though it's a height and no width unit (confusingly, knowing where its name comes from) and therefore doesn't change with more narrow or wider fonts. I wonder what you see as the usefulness or intent of the unit at all if not sizing boxes to fit a number of average-size characters (there is no CSS for "width of the string 'Page Navigation' plus some extra margin). But this discussion drifts away from what the bug is/was about.
Comment 18 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-07-10 09:21:50 PDT
I posted a comment about the spec:
Comment 19 Tony Chung [:tchung] 2009-07-14 08:15:24 PDT
Can someone that has OS/2 here verify this fix on trunk and a 1.9.1 nightly? 
Please change the status and keyword to verified also.   Thanks.
Comment 20 Peter Weilbacher 2009-07-14 09:24:12 PDT
Maybe we should always just go to verified directly for OS/2 bugs to keep QA from finding them. (In this case, Steve now verified in the newsgroup that this is fixed with Workplace Sans 0.4.)

David, thanks for raising the point on the W3 list. :-)

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