364300 - Monospace font isn't, with ATSUI, breaking "cols" attribute of <textarea>

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Current trunk Mac build                                                  
                                                                                 
STEPS TO REPRODUCE:                                                              
1)  Load 
  data:text/html,<textarea style="font-family: monospace" cols="5" rows="3">01234
2)  Hit enter 4 times
3)  Scroll back up to the first line

ACTUAL RESULTS:  Observe that the numbers don't go up to the scrollbar.

EXPECTED RESULTS:  Numbers should go up to the scrollbar.

MORE DATA:

In nsTextControlFrame::CalcIntrinsicSize, the return values of GetAveCharWidth and GetMaxAdvance are different, even though this is a monospace font.  I tried debugging, and in gfxAtsuiFont::gfxAtsuiFont for the monospace font we have:      
                                                                                 
(gdb) p atsMetrics.maxAdvanceWidth
$35 = 0.823242188
(gdb) p atsMetrics.avgAdvanceWidth
$36 = 0
(gdb) p GetCharWidth(' ')
$38 = 7.80126953
(gdb) p GetCharWidth('a')
$39 = 7.80126953
(gdb) p GetCharWidth('-')
$40 = 7.80126953

Here "size" is 13, so the the maxAdvanceWidth doesn't match the aveCharWidth we end up setting.                                                                  
                                                                                 
The problem is that we need to be able to detect a fixed-width font in this case, and if the ave char width and max advance differ the font is clearly not fixed-width... in which case why are we using it for "monospace"?

This is a pretty serious web-compat problem, for what it's worth -- the two sizing algorithms used in the two cases by <textarea> are pretty different.  Sadly, I have no idea whether this is a cairo regression...

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The monospace font doesn't mean that the all characters has same width. E.g., CJK characters need the width of 2 ASCII characters. And some characters are zero-width.

Now, the Avg char width returns width of 'x' if ATSUI returns '0' or larger value than width of 'x' (e.g., Osaka-mono). (see bug 362665 comment 18.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

> E.g., CJK characters need the width of 2 ASCII characters. 

If that were all there were to it, we could try to deal.  But in this case the max advance is some non-integral multiple of the average char width.  I should note that the "2 ASCII characters" thing _does_ mean that there's no sane way to make "cols" work with such characters, btw.

In the end, I couldn't care less what's returned for these various font metrics; from the point of view of layout I just want a 100% reliable way to detect monospace fonts being used so that we can set the width to cols * "the font's char width", defined in some way that makes the common use cases work.  But only for monospace fonts, since for non-monopace ones IE does something different and we need to as well.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

How about that we add the |isFixedPitch| flag for the metrics? Mac can get whether the font is fixed pitch font.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That would work for my use case, yes.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Or, should we set same value to maxAdvanceWidth and avgAdvanceWidth if the font is fixed pitch?

Comment 6

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I have a patch that checks whether a font is monospace, and if so, it forces avgAdvanceWidth = maxAdvanceWidth.  However, that doesn't result in any different rendering for the given testcase -- there's still a bit of a space betwee the last digit and the start of the scrollbar.  I think that's caused by a margin around the scrollbar itself, though; the scrollbar doesn't butt up to the top/right/bottom edges of the frame either, there's about 2-3 pixels of space... and I think those 2-3 pixels are on the left side as well.

bz, are you seeing a worse spacing than this?  cols=1,2,..5 all seem to be have correctly (they go up by the same amount, which looks to be the width of one char.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: the patch

The patch, FWIW.  (It might not apply cleanly, some of the context is from another patch, but it's fairly simple.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, I'm seeing more space than that.  I can post a screenshot if you like.

I tried applying your patch, and it doesn't help.  In GetOrMakeFont (in gfxAtsuiFonts.cpp) we name == "Courier" by the time we're calling gfxAtsuiFont::gfxAtsuiFont(), aFontID == 8197, and aFonts an empty array.  aStyle seems as-expected.

Then we call InitMetrics() with a fontRef of 8197.

Then we ask the font cache whether the font with id 8197 is fixed-pitch, and it claims that it's not.  That seems bogus for Courier...

The FontEntry's mTraits is 0x100000c in this case.  That looks like NSUnboldFontMask | NSNonStandardCharacterSetFontMask | NSUnitalicFontMask if I read http://developer.apple.com/documentation/Cocoa/Reference/ApplicationKit/Classes/NSFontManager_Class/Reference/Reference.html right.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: what this looks like for me

Here's what this looks like for me.  A screenshot would be helpful, just so I know what to look for.  Can you also tell me what your font settings are?  I can't manage to reproduce this and I just tried a bunch of different fonts set as the monospace font, the default was Courier 13 (which is what this screenshot is using).

Comment 11

[Boris Zbarsky [:bzbarsky]]

That's exactly what my screenshot looks like.  That's not right.  And it's a lot more space than on the other sides of the scrollbar.

More importantly, just stepping though the code in nsTextControlFrame::CalcIntrinsicSize gives me a GetAveCharWidth of 468 and GetMaxAdvance of 643, so we go down the variable-width font sizing codepath instead of the fixed-width font sizing codepath.  That's the bug.

The text rendering itself is correct, of course.  It's just the sizing of the input control that's off.

My font prefs are whatever we default to, which is Courier 13, looks like.  As I said, your screenshot looks just like mine.  ;)

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: win32 screenshot

Oddness.. on win32, data:text/html,<textarea style="font-family: Courier New; font-size: 13pt;" cols="5" rows="3">01234</textarea>

other fonts look fine (though "fine" means that they tend to end up with a horizontal scrollbar once they have 5 characters in there when the vertical scrollbar shows up)

But, I'll debug it via looking at the metrics themselves.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Uh.. Interesting.  On linux here, "Courier New" gives me a variable-width font (which might just be because it's not installed).  "Courier" gives me a fixed-width font and works as it should.

I wonder what's up with the Windows code.  What do you see in nsTextControlFrame::CalcIntrinsicSize ?

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix

This fixes it, I think -- the problem earlier was that IsFixedPitch() was always returning false, because that trait was never appearing in the traits list... asking the font directly whether it's fixed pitch or not returns the correct value. I dunno.

Now, I'd still like to know why these fonts report a different average character width from max advance -- both those metrics are coming straight from the type system.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Interesting.  http://lists.apple.com/archives/cocoa-dev/2001/Dec/msg00836.html lists exactly the value I was seeing for mTraits.  Maybe traitsOfFont is just busted in some cases?

Comment 16

[Boris Zbarsky [:bzbarsky]]

That patch still doesn't quite work over here.  The reason is that GetAveCharWidth() does ROUND_TO_TWIPS and GetMaxAdvance() does CEIL_TO_TWIPS.  So the former ends up 468 twips and the latter ends up 469 twips, given the starting value of 7.80126953125.  7.80126953125 * 60 == 468.076...., which is where we get those numbers.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix, 2.0

Let's just get rid of using traitsOfFont entirely, based on bz's info.

Comment 18

[Boris Zbarsky [:bzbarsky]]

If I make both functions ROUND_TO_TWIPS, I get the rendering I would expect on the testcase.

Oh, and this is the only caller of GetMaxAdvance() in the tree, as far as I can tell... 

Comment 19

[Boris Zbarsky [:bzbarsky]]

So... I see the same issue on Linux.  I guess I should file a separate bug on this, eh?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I filed bug  388179 on the Linux issue.  Does Windows have a similar problem?

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This was checked in; resolving.

Comment 22

[Boris Zbarsky [:bzbarsky]]

It looks like comment 16 never got addressed.  See bug 428458, which I suspect is caused by that.