As of when? Is it failing on tinderbox? Is this a regression?

Not a regression. It's a new test from bug 410405. It was failing on tinderbox but has been disabled.

Oh, for crying out loud. People broke that _again_? :( This used to work. There are bugs I filed and vlad fixed a while back to make it work... When did it regress again?

And I assume we did verify that the differing pixels are in the sizing of the first input? See bug 364300 for the mac manifestation of this that got fixed, for what it's worth. Too bad no one created a test for it. :( Or is the real issue here the 1px padding that's there for the direction indicator on the caret?

It's only a little too wide so I'm guessing aveCharWidth == maxAdvance, but it's more than 1 pixel.

Windows is similar to Mac

This is the type of 1 pixel / anti-alias issue that I would expect.

That's really odd. Can you breakpoint in the auto-width computation in text control frame and see what things look like? The Mac/Windows screenshots look to me like aveCharWidth != maxAdvance and we're using the variable-width algorithm...

I've added a space to the reftest as it was probably too strict, but there still seems to be too much width on Mac and Windows.

forms/input-text-size-1.html that is.

(In reply to comment #8) > Can you breakpoint in the auto-width computation in text > control frame and see what things look like? Not easily. I don't have debug Windows or Mac builds. > The Mac/Windows screenshots look to me like aveCharWidth != maxAdvance > and we're using the variable-width algorithm... Maybe you are right. Maybe that is an extra character - 4px.

At some text zoom sizes on Mac the testcase renders as expected suggesting that perhaps aveCharWidth ≆ maxAdvance at other times.

Well, one obvious possible problem is that nsThebesFontMetrics::GetMaxAdvance uses CEIL_TO_TWIPS while nsThebesFontMetrics::GetAveCharWidth uses ROUND_TO_TWIPS. So if they're equal in floating point (as stored) but round differently due to the CEIL vs ROUND thing, this will break. That's bug 364300 comment 16. I guess that never got resolved... Nominating, since this is a regression from 1.8 that we really should fix. Just making GetAveCharWidth use CEIL_TO_TWIPS seems like the right thing to me, to be honest. Or giving textframe a way to ask the question it really wants to be asking, of course....

As suggested by Boris. Thanks! Doing the same in GetSpaceWidth for consistency.

Remove my irrelevant comment.

Comment on attachment 315051 [details] [diff] [review] use ceil consistently [checked-in] Looks ok to me but really Stuart should review this

Comment on attachment 315051 [details] [diff] [review] use ceil consistently [checked-in] very low risk, fixes a fairly major layout regression.

Comment on attachment 315051 [details] [diff] [review] use ceil consistently [checked-in] a1.9+=damons

Comment on attachment 315051 [details] [diff] [review] use ceil consistently [checked-in] Checked in and tightened up test. http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1208302414&maxdate=1208302443&who=karlt%2B%25karlt.net http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1208302200&maxdate=1208302269&who=karlt%2B%25karlt.net but apparently this is no longer the only issue.

Still failing the same way on Mac.

and Windows

(reftest forms/input-text-size-1.html currently disabled on Mac and Windows)

I'll take the Mac/Windows side.

Hmm. So the CEIL change didn't fix things?

On Mac, with the input-text-size-1 testcase, charWidth is 397 appunits and charMaxAdvance is 544 appunits --- way off.

Yeah, that sounds like something is just failing... and that the something is a regression, since vlad did fix just that sort of thing, iirc. If it's at all possible, let's get tests in for this.

The font is Courier. ATSUI reports atsMetrics.maxAdvanceWidth=0.823242188, which we multiply by the size (11) to get 9.0556640625. The width of 'x' is reported as 6.60107422. We get the width of 'x' by examining trapezoids (ATSUGetGlyphBounds) so I'm not all that confident it's accurate. The glyph does look about that wide, but you need space between it and the next character, and I don't think that's being measured. I'll try measuring two 'x's and measuring the distance from the left edge of one to the left edge of the next. But anyway, this all seems incredibly fragile especially given that ATSUI uses float font metrics; we'll have to be really lucky to get the computed width to equal the max-width. Is this really the right thing to check?

What I _want_ to be checking is whether this is a monospace font or not. There's no gfx API exposed for this, and vlad and stuart were opposed to adding one in bug 364300 and company... Note that the way bug 364300 got resolved was: + if (gfxQuartzFontCache::SharedFontCache()->IsFixedPitch(aFontID)) { + mMetrics.maxAdvance = mMetrics.aveCharWidth; which works whether you're using floats or not. That code is still there, and that's the information layout should be getting, right? Is layout seeing different numbers here?

Heh, turns out Courier is not a monospace font on the Mac but that's what the default monospace font is set to. Change it to 'Andale Mono' and the testcase passes. The Cocoa API [NSFontManager availableMembersOfFontFamily] used to initialize the font list gives use font traits for each font face and IsFixedPitch uses these: PRBool MacOSFontEntry::IsFixedPitch() { return mTraits & NSFixedPitchFontMask; } There are only three normal system fonts on my 10.4 system that are monospace fonts: Andale Mono Courier New Osaka Mono Sounds like we should switch Courier to Courier New instead on the Mac. On 10.5, Courier and Monaco are also listed as monospace fonts.

Does there need to be such a distinction between monospace and variable width fonts. Could the effect of inexact metrics (or monospace fonts that don't report as monospace) be reduced by using min(cols * avgWidth + maxAdvance - 4 * onePixel, cols * maxAdvance + onePixel) instead of choosing from cols * avgWidth + maxAdvance - 4 * onePixel or cols * avgWidth + onePixel (keeping the other tweaks that I left out to simplify)? Even then we'd also need a reasonable measure of average character advance width. I thought that was what aveCharWidth was for... (In reply to comment #28) > We get the width of 'x' by examining trapezoids > (ATSUGetGlyphBounds) so I'm not all that confident it's accurate. The glyph > does look about that wide, but you need space between it and the next > character, and I don't think that's being measured. ... but this makes it sound like aveCharWidth is a tight glyph bound. That doesn't sound so useful to me. The windows code uses tmAveCharWidth which may be "the average width of characters in the font (generally defined as the width of the letter x). This value does not include the overhang required for bold or italic characters." http://msdn2.microsoft.com/en-us/library/ms534202.aspx or "the arithmetic average of the escapement (width) of all non-zero width glyphs in the font." http://www.microsoft.com/typography/otspec/os2.htm#acw Not sure what "escapement" is but I thought it was advance width.

Doing the min() thing might work. The thing to test would be how it compares to IE... But then again, we seem to be less worried about matching IE's form control sizing; native theming breaks a lot of that, esp. on Mac.

Seems to me that the best fix for 1.9 would be to make the default monospaced font on Mac Courier New. We could try Karl's idea near the beginning of a major release cycle. I'll do that, and look into what's going on with Linux.

I mean, Windows.

On Windows, the font is Courier New, the maxAdvance is 9 but the average char width is 8.

Fixes Mac by changing the default monospace font to Courier New. Fixes Windows by adding code similar to what we added for Mac, setting the maxAdvance to aveCharWidth for monospace fonts.

Comment on attachment 316138 [details] [diff] [review] fix Smokey, could you review the mac portion of this?

Is this the part where I get to grumble about bug 364300 comment 20 never getting a response? :( I should have just assumed Windows was buggy and filed... roc, thanks for picking this up!

Yes it is! No problem.

Comment on attachment 316138 [details] [diff] [review] fix Based on comment 30, we need to fix Cyrillic, too. On 10.5, the new version of Courier New has Cyrillic glyphs; I don't know about the old version on 10.4 (which is where this bug appears to exist), though. If Courier New doesn't have those glyphs on 10.4, there's nothing we can do on that OS version, so we should probably leave Cyrillic alone in that case, as Monaco is not broken on 10.5. (Alternatively in the no-glyphs-for-Cyrillic-on-10.4 case, we could, and perhaps should, make a change to Monaco for font.name.monospace and Monaco,Monaco CY for font.name-list.monospace, since Monaco CY was merged into Monaco on 10.5--font-name-list should provide a "fast-path" fallback on 10.4, right?) Note that we'll also be wrong in just about every other langGroup, but we have no choice given that we're lucky to have a single font in most of those cases. This really sucks that 10.4 is buggy here, because Courier New has very different metrics and is a much lighter/harder-to-read face (screenshots to come for folks on other platforms). I suspect this will also raise the ire of Mac cognoscenti, both for being ugly and for Moz "randomly" differing from Safari. That said, it does seem to be the reasonable fix within the current constraints. Can we get a follow-up on switching back to Courier when we move to 10.5-and-above or if we come up with another solution? r=ardissone on the Mac bits, though, with a decision about Cyrillic.

John Daggett is going to take over my patch here.

Based on Smokey's screenshots, setting Courier New as the default monospace font doesn't seem like the right way to go. Instead, on 10.4 explicitly tag Courier and Monaco to be fixed-pitch fonts. This appears to eliminate the problem. Patch also includes roc's previous Windows code change.

John, did you want to ask for approval here?

Comment on attachment 316186 [details] [diff] [review] patch, v.0.2, explicitly set Courier to be fixed-pitch font on mac wait, this is already a blocker...

Comment on attachment 316186 [details] [diff] [review] patch, v.0.2, explicitly set Courier to be fixed-pitch font on mac a=me, but please fix nit: >+ // clean up various minor 10.4 font problems for specific fonts >+ if ( gfxPlatformMac::GetPlatform()->OSXVersion() < MAC_OS_X_VERSION_10_5_HEX ) { No spaces after the (/before the ) in the if clause

Didn't John land this yesterday? http://tinyurl.com/4qrqev

Yes, that's it.