404698 - Acid2 misrendering in FF 3.0b1 related to default font

Status: RESOLVED INVALID

(Core :: Layout: Block and Inline, defect, P3)

Description

[Yusuf Goolamabbas]

Attachment: rendering of acid2 test page on FF3.0b1 installed alongside FF2

Hi, I installed FF 3.0b1 on a Windows XP SP2 box alongside my existing installation of FF 2.0.0.9. I started up with a separate profile and proceed to look at the Acid2 test page

Attached please find the screen shot. Basicaly the chin drops down by a few pixels.

I then removed FF2 and FF3.0b1 alongside all profiles and then reinstalled FF3.0b1. When I test the Acid2 test page, the rendering is correct

Comment 1

[Majken Connor [:Kensie]]

I've got both installed and it looks fine.  Was the profile you were using new? Extensions?  Would it look like that every time?

Comment 2

[Daniel Holbert [:dholbert]]

I also see this chin-a-few-pixels-too-low problem on Linux.

Using a clean profile on a build from today:
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112012 Minefield/3.0b2pre

And yup, it looks like that every time.

Also -- the nose seems to start a few pixels higher in the test as compared to the reference.  But it's more subtle than the chin so I'm not sure that's a problem (or it may be a different problem).

A regression range would be awesome.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The nose thing is a known issue with the test: the spec underdefines beveling of borders, and we bevel 1px differently from the reference rendering.  It's not a problem.

We should figure out what dholbert is running into, though.

Comment 4

[Majken Connor [:Kensie]]

Looks fine for me on:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007111805 Minefield/3.0b2pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre

So guessing it broke after that?

Comment 5

[Majken Connor [:Kensie]]

hmm no, cuz the user says b1, screenshot looks like XP

Comment 6

[Yusuf Goolamabbas]

bz spent some time with me on irc for this. The dropping chin seems to be a function of setting minimum font size. I had it set at 11

However, as I demonstrate in another attachment, even if the min-font-size is not set the test seems to be sensitive on some font metrics even though the user has indicated the use of a different font family for proportional fonts

I created a clean profile, set my proportional font to be San Serif and changed the default from Arial to Verdana (kept the size at 16). no mis-render detected
Then I changed the Serif font from Times New Roman to Georgia and saw the chin pushed up

Comment 7

[Yusuf Goolamabbas]

Attachment: acid2 rendering with Georgia 16 as Serif font, no min size

Comment 8

[Hixie (not reading bugmail)]

That's weird. Can you make a minimised test showing just the problem with nothing unnecessary as part of the test?

Comment 9

[Majken Connor [:Kensie]]

It looks like it's something to do with the font width.

Arial, Lucida Console, Courier New - Passed

Arial Narrow, Georgia, Cambria Math - Failed

(updating summary)

Comment 10

[Boris Zbarsky [:bzbarsky]]

The test has:

   .chin div { display: inline; font: 2px/4px serif; }

(which is why it's the serif font selection that matters?).

The markup is:

   <div class="chin"><div>&nbsp;</div></div>

I'd be curious.  Can someone who can reproduce the bug look in DOM inspector to see whether the <div class="chin" is taller than it should be, or whether something else is happening?  And if it is too tall, how big is that <div> inside it?

Comment 11

[Majken Connor [:Kensie]]

With Georgia as the font (see second screenshot), the div height is 0px, where it's 12px with Arial selected. 

With minimum font size of 11 selected, and font Times New Roman, div height is 22px

Comment 12

[Majken Connor [:Kensie]]

Attachment: min font 14

More than the chin breaks when min font size is set to 14, it seems.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> With minimum font size of 11 selected

I don't think that's worth testing.  The testcase relies on a 2px font size.  So a minimum font size of 3 or more will screw up the rendering.

With Georgia as the font, what's the height of the <div> inside the <div class="chin">?  Is it also 0px like its parent?

Comment 14

[Majken Connor [:Kensie]]

The child div height is "auto" .

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: screenshot: acid test (FF3b1 linux)

FWIW, attaching screenshots of what I'm seeing.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: screenshot: acid reference (FF3b1 linux)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> The child div height is "auto" .

This is in the computed style view in Inspector?  That's _very_ wrong.

Daniel, what are the heights of the two divs with your setup?

Comment 18

[Daniel Holbert [:dholbert]]

Using a fresh profile, the unedited Acid test, and the FF beta1 build, I see this in DOM inspector, checking computed style:

  chin div height  = 12.5px
  child div height = auto

Comment 19

[Boris Zbarsky [:bzbarsky]]

Right.  And it should be 12px, as far as I can tell.

The child div being auto is because it's got "display: inline", which I had missed.  Instead of looking at computed style, can you go to the "box model" viewer and select "Dimensions"?  What are the box width and height?

One other question.  Do any of the people seeing issues have a DPI higher than 144?

Comment 20

[Majken Connor [:Kensie]]

So the box dimensions on the chin are

box width 120 box height 0

Dimensions on the child are

box width 0 box height 3

My dpi is 96 (windows default)

Comment 21

[Boris Zbarsky [:bzbarsky]]

This is starting to sound like a gfx bug of some sort to me...

Comment 22

[dolphinling]

(On 2007-11-21-05 linux trunk nightly, clean profile) I see a 1px-too-tall chin, and I get 

chin:
(computed style) height: 12.5px
box width: 120
box height: 13

child:
(computed style) height: auto
box width: 1
box height: 4

xrdb thinks my dpi is 96, xdpyinfo thinks it's 113. (113 is correct).

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yeah, that 12.5px is the problem.  Can you figure out where that's coming from?

Comment 24

[Majken Connor [:Kensie]]

dolphinling, dholbert: to be clear, you *don't* have a min font set, right? and you're both on linux.  Does linux force a min font size?

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to comment #24)
> dolphinling, dholbert: to be clear, you *don't* have a min font set, right? and
> you're both on linux.  

Correct and correct, for me.  No minimum font set, and running Linux.

> Does linux force a min font size?

Not that I know of.

Comment 26

[dolphinling]

Attachment: Reduced testcase

In case it helps, here's a reduced testcase. It should be a 12px high blue rectangle, instead it has a computed height of 12.5px and displays as 13.

And In reply to comment #24, all my font prefs are at the default. (And the prefs UI says "Minimum font size: None".)

Comment 27

[philippe (part-time)]

Fwiw, on OS X ppc, Minefield 2007112204, both the reduced test case and the original Acid2 return the same 12.5px/13px for the chin. But _only_ with Georgia set for 'serif' in the prefs.

Other serif fonts (Times, BitStream Vera, MS Constantia, ...) all give 12px/12px.

Comment 28

[Boris Zbarsky [:bzbarsky]]

So removing that child div makes the height 12px again, I assume?

Comment 29

[philippe (part-time)]

(In reply to comment #28)
> So removing that child div makes the height 12px again, I assume?
> 
removing the child div makes div.chin collapse (height:0), because it is empty then.
Or did I misunderstand you ?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Ah, the style is line-height, not height.  OK.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I just tried Georgia on OSX/intel (Tiger), and I don't see this bug.  :(

Comment 32

[[e-mail]]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b2pre), my own build, pulled 20071120:

i'm also seeing a tall chin w/ 12.5px computed style height on a fresh profile. i'm on FC7 (x86_64); prefs show no minimum font size set.

Comment 33

[Majken Connor [:Kensie]]

messed up chin with default font set is linux only?

Comment 34

[Boris Zbarsky [:bzbarsky]]

OK, can someone who's seeing that 12.5px thing dig into this using the minimal testcase?  I'd love to see the relevant part of a frame dump from the layout debugger, for example.

It might turn out that there are several separate bugs here, of course... but that's the one we've got the most traction on so far.

Comment 35

[[e-mail]]

Attachment: frame dump

per comment 34, this is a frame dump for the reduced testcase.

Comment 36

[Boris Zbarsky [:bzbarsky]]

dwitte kindly stepped through this for me.  On the minimal testcase we end up at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsLineLayout.cpp&rev=3.285&mark=2097-2106#2092 for the block.  At that point, minY is -2px and maxY is 2px (because the inline has 2px height and 4px line height).

Now we get a 12px fontAscent and 15px fontHeight.  So we compute a leading of -3px, divide it evenly between top and bottom, and set yTop to -10.5px and yBottom to 1.5px.  That is, if it were not for the line-height being set we'd st yTop to -12px and yBottom to 3px, but our line-height is too small for our 15px font, so we reduce both top and bottom by 1.5px.

Then we end upwith minY being -10.5px and maxY being 2px.  For a total height of 12.5px.

So why exactly are we coming back with a 15px font when the testcase very explicitly styles the root with a 12px font-size?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Then again, I guess the font-size refers to the part above the baseline?

In particular, if this font had a descent of 4px (and ascent of 12px) we'd get 12px total height: 2px below the baseline, and 10px above.

On the other hand, a font with 12px ascent and 0px descent would give us 12px above the baseline and 2px below, for 14px total.

So either the line-height code in nsLineLayout is wrong, or the test is making unwarranted assumptions about fonts: specifically that either the 12px sans-serif font always has at least 4px descent or that the 2px serif font never has 2px descent.  In this case we have what looks like a 2px descent on the 2px font and a 3px descent on the 12px font.

David, Ian, which is it?  Per my reading of CSS2.1 section 10.8.1, our code looks correct (and the test buggy).

None of that explains the people who're getting a 0-height chin, of course.  That sounds like a separate issue.

Comment 38

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #37)
> Then again, I guess the font-size refers to the part above the baseline?

No, it refers to whatever the font designer wanted, although generally a height that extends both above and below the baseline.

Comment 39

[Boris Zbarsky [:bzbarsky]]

In that case, we should perhaps look into why the total height of this "12px" font is "15px".  That is, is this a font issue, or our problem?

dwitte, what's your sans-serif default font?  Stuart, Vlad, is there a way to see what our font selection code is doing, exactly?  And to view the properties of some font on Linux?

Comment 40

[Hixie (not reading bugmail)]

For the people seeing this bug -- does this test work?
  http://www.hixie.ch/tests/evil/acid/002/index-with-all-fonts-sans-serif.html

If so, then it's actually a bug in the test (I mixed fonts when I shouldn't have).
I'm not sure I can get the wasp version fixed though. Use fonts with less special metrics when testing the Acid2 test... :-)

Comment 41

[Daniel Holbert [:dholbert]]

(In reply to comment #40)
> For the people seeing this bug -- does this test work?

No, it doesn't.  Chin is still slightly lower in 'test' as compared to 'reference'.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Daniel, can you step through the nsLineLayout code in question and see what metrics you're getting?

Note that the updated test is still making assumptions about the font metrics, though.  I'm not sure why they're more justified than the original assumptions, exactly.

One other thing.  Perhaps we should use bug 400813 for the disappearing-chin issue.  Do the people seeing that also see the div in the minimal testcase here disappear?

Comment 43

[Daniel Holbert [:dholbert]]

(In reply to comment #42)
> Daniel, can you step through the nsLineLayout code in question and see what
> metrics you're getting?

Yup, I can take a look.

I might not report back with useful results until tomorrow morning, though, as I have to leave early this afternoon.

Comment 44

[Majken Connor [:Kensie]]

(In reply to comment #42)
 Do the people seeing that also see the div in the minimal testcase here
> disappear?
> 

yes! I still had my font sent to Georgia when I checked it out the first time and couldn't figure out where it was.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Great!  Now we need someone who can reproduce that _and_ has a debug build...

Comment 46

[Boris Zbarsky [:bzbarsky]]

OK.  Let's take the disappearing-chin-with-Georgia thing to bug 400813, since it's a totally different problem.

Comment 47

[philippe (part-time)]

(In reply to comment #40)
> For the people seeing this bug -- does this test work?
>   http://www.hixie.ch/tests/evil/acid/002/index-with-all-fonts-sans-serif.html
> 

On OS X 10.4.11 ppc, still a failure (that is:13px tall chin) with some fonts: 'Lucida Grande' and 'Helvetica Neue'. Latest Camino trunk build and Minefield build with new profiles.
I haven't tested all fonts available...

Incidentally, I noticed earlier this week that for both those fonts the computed height of a paragraph using those fonts is taller in Gecko 1.9 than it is in WebKit.

Comment 48

[[e-mail]]

(In reply to comment #39)
> dwitte, what's your sans-serif default font?

under firefox prefs it's "sans-serif", which on my FC7 install is:

$ fc-match sans-serif
DejaVuLGCSans.ttf: "DejaVu LGC Sans" "Book"

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to comment #42)
> Daniel, can you step through the nsLineLayout code in question and see what
> metrics you're getting?

This is what I get in the reduced testcase:
(gdb) p GetMetrics()
$39 = (const gfxFont::Metrics &) @0x8846abc: {xHeight = 7, superscriptOffset = 6, subscriptOffset = 2, strikeoutSize = 1, strikeoutOffset = 3, underlineSize = 1, underlineOffset = -1, height = 1.9189240878861421e-76, internalLeading = 0, externalLeading = 3, emHeight = 12, emAscent = 9.5999999999999996, emDescent = 2.4000000000000004, maxHeight = 15, maxAscent = 12, maxDescent = 3, maxAdvance = 21, aveCharWidth = 6.1533203125, spaceWidth = 4}

The immediate context being:
#0  nsThebesFontMetrics::GetHeight at nsThebesFontMetrics.cpp:151
#1  0xb5e9a6ed in nsLineLayout::VerticalAlignFrames at nsLineLayout.cpp:2097

If it hasn't already been said, we're getting a 15px height here because height = maxAscent + maxDescent = 12 + 3 = 15.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Oh, that's very interesting.  So the site has a emHeight and maxHeight are different, with the former being 12 (which is why we get back this font when we ask for a 12px one) and the latter being 15.  OK, then.

I think the conclusion is that this test is just making unwarranted assumptions about how fonts will work...

Of the three issues we had here, the min-font-size is invalid, looks like this one is invalid, and the disappearing chin on Windows is covered by bug 400813, right?

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like it to me.