426616 - Acid2 chin is 1px too tall in FF3b5

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect, P2)

Description

[Sven E.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

After comparing the test with the reference you´ll see a additional line in the "test-face", that should not be there

Reproducible: Always

Steps to Reproduce:
1.Go to acid2.acidtests.org
2.Take the Test
3.Compare it with the reference
Actual Results:  
You will see there is a additional Line that should not be there

Comment 1

[Jesse Ruderman]

It looks fine for me.  Can you post a screenshot of what you're seeing?  Do you see the problem even in a clean profile (default settings, no extensions, etc)?

Comment 2

[u88484]

The nose also grows. See next screenshots, the top red line was just a reference line....the other ones show differences between the Firefox 3.0pre (2008040204) build on the left side and the right side is the reference picture.

Comment 3

[u88484]

Attachment: Screenshot

Comment 4

[u88484]

Attachment: Better screenshot

Screwed the last one up, here is a better screenshot

Comment 5

[Jesse Ruderman]

I don't see the "extra line" issue on Mac, fwiw.

The nose rendering is fine despite not matching the "reference rendering" pixel
for pixel.  See bug 343583 comment 6.

Comment 6

[u88484]

Jesse, it looks like the nose grows one pixel from exactly in the middle of the nose.  If you look exact in the middle on the edge in the reference picture, the center is 1 pixel tall but in latest Minefield builds it is two pixels tall.

Comment 7

[Rick Stockton]

I'm fine on the nose (when I remember to disable my font and minimum font size preferences). But the small square boxes on either side of the chin (not the forehead), we now display TALLER than the reference image. The way the chin div height is calculated is influenced by the <div> inside of it- we round up to 13 px.

I'm pretty sure that this is a side effect of bug # 413787, which has recently put in new code trying to streamline some layout calculations involving rounding on floating point numbers. (I pretty much duplicated this comment there too, not seeing that someone had opened a new bug for the regression.)

Comment 8

[Reed Loden [:reed]]

If bug 413787 is the cause, you're only going to see this bug on Windows.

Comment 9

[Reed Loden [:reed]]

Oh, I just noticed that this is talking about Firefox 3 Beta 5. Bug 413787 just
landed last night, so it's not in Beta 5, so it isn't the cause.

Comment 10

[Sven E.]

Attachment: Test-face with additional line

Comment 11

[Sven E.]

The red line in the above attachment shows the additional line that shouldn´t be there...

After testing the acid2 on a additional system (Intel Core 2 Duo, Intel 945 Chipset and Media Center) it doesn´t show that rendering bug, so it seems this bug is only happening on my System.

Specs:
AMD Athlon XP 1800+
Nvidia 6200

Comment 12

[u88484]

(In reply to comment #7)
> I'm fine on the nose (when I remember to disable my font and minimum font size
> preferences).

I was using a brand spanking new profile so there were no changes on my end to the fonts or anything.  I created a new profile and didn't do anything except go to the test and the reference picture.

Comment 13

[Seth Foss]

Attachment: Firefox 3.0b5 vs. Reference Rendering

Note how Firefox adds an extra row to the jaw.

Comment 14

[Damon Sicore (:damons)]

Vlad, can you take a look here?

Comment 15

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

I spent a bit looking at it earlier this afternoon, but I really have no idea.. someone who knows acid2 and what each individual row is doing would be better.  Do we have screenshots comparing acid2 pre and post the rounding patch landing?  (Also, seems like acid2 should be a reftest!)

Comment 16

[Jeff Walden [:Waldo]]

It is on Mac.  Feel free to deal with the mess that is font inconsistencies on the Linux platform, where I encountered much pain trying to get the exact right fonts with the exact right font metrics, and we can make it one there.  (Alternately, give me a @font-face so we can dump exact fonts in the tree; I know the arguments against, but my current strategy for this is to wait for acid3 to force our hand, however that happens, and work from there.)  Last I tried, Windows had the offcolor PNG problem, but that's since been fixed and there might not be any more problems there.

This is all bug 409329, and I can't venture down that rabbit hole while still being maximally productive, at least not without @font-face to guarantee exact fonts and font metrics (and I'm only speculating that'd be sufficient, I don't know for sure).

Comment 17

[Damon Sicore (:damons)]

Well, I'm going to leave this as blocking for now until we get some sorta resolution on finding out why this regressed.  Also, seems like we should have this as a reftest on all platforms, right?  Are there bugs for those?

Comment 18

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

Ok, marking qawanted -- really need a regression range, given that it's not bug 413787.

Comment 19

[Luke Iliffe (Harlequin99)]

Regression range for the lengthening of the face is:

good Gecko/2008032912 Minefield/3.0pre 20080329_1310
no good Gecko/2008032914 Minefield/3.0pre 20080329_1447

bonsai http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1206821400&maxdate=1206827219

The nose moving 1px has been around for months afaik

Comment 20

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

Thanks, Luke!  So looks like bug 421353?  (The nose isn't a problem.)  Cc'ing masayuki and roc.

Comment 21

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

If this is a regression of bug 421353, Acid2 test uses the bad underline fonts:

the default value of font.blacklist.underline_offset

FangSong,Gulim,GulimChe,MingLiU,MingLiU-ExtB,MingLiU_HKSCS,MingLiU-HKSCS-ExtB,MS Gothic,MS Mincho,MS PGothic,MS PMincho,MS UI Gothic,PMingLiU,PMingLiU-ExtB,SimHei,SimSun,SimSun-ExtB,Hei,Kai,Apple LiGothic,Apple LiSung,Osaka

But if your system is not CJKT locale, they should not be used...

Comment 22

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

(In reply to comment #21)
> If this is a regression of bug 421353, Acid2 test uses the bad underline fonts:

I don't think that's a good assumption -- the patch changes code that's not related to directly handling fonts that are listed in that pref.

Comment 23

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

We need to try a backout of that patch, to verify that it is in fact 421353.  If so, we'll need a quick fix or we'll have to back out bug 421353.

Comment 24

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

er, if the font size is very small (less than 3px?), the patch might change the layout with non-blacklisted fonts..

Comment 25

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

Yeah, Acid2 does use fonts that are very small but non-zero in size. But why would those fonts have a large underline offset? Is it because the platform forces the minimum underline offset to be 1 or something?

Perhaps we should just revert to dynamic adding of decoration area to the overflow area, for very small font sizes?

Comment 26

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

(In reply to comment #25)
> Yeah, Acid2 does use fonts that are very small but non-zero in size. But why
> would those fonts have a large underline offset? Is it because the platform
> forces the minimum underline offset to be 1 or something?

If ascent is 1px and descent is 0px, we need 4px in current trunk:
1px: overline
1px: ascent (and baseline)
1px: gap for underline
1px: underline

I think:
1. we should contain the bottom of 1px in ascent space
2. if descent is zero, underline should be on baseline
3. if descent is 1px, underline should be on descent space

the decoration lines are overwrite the text by this approach, but we don't need to worry the accessibility for the text :-p

> Perhaps we should just revert to dynamic adding of decoration area to the
> overflow area, for very small font sizes?

No, we can fix the issue by the above story.

Comment 27

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

> 1. we should contain the bottom of 1px in ascent space

1. we should contain the bottom (1px) of the overline in ascent space

Comment 28

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

(In reply to comment #27)
> > 1. we should contain the bottom of 1px in ascent space
> 
> 1. we should contain the bottom (1px) of the overline in ascent space

Ugh, this is wrong. The overline is in the ascent.

Comment 29

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

Attachment: Patch v1.0

I think that this fixes the issue. (I'm not sure this can fix this bug, because I cannot reproduce it.)

Comment 30

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

Attachment: testcase

Comment 31

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

Attachment: screenshot of attachment 314208 (left: current trunk, right: patched build)

Comment 32

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

Comment on attachment 314207 [details] [diff] [review]
Patch v1.0

mmm... sorry for the spam.
I need more works...

Comment 33

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

Comment on attachment 314207 [details] [diff] [review]
Patch v1.0

er, sorry.

this might be ok. in some cases, the border of span element is rendered outside of the p element. but 20080328 build (pre this regression) renders as same. So, this patch should not have the new bug for the issue.

Comment 34

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

Attachment: Patch v1.1

oops, the previous patch has a bug.

Comment 35

[Stuart Parmenter]

Comment on attachment 314272 [details] [diff] [review]
Patch v1.1

I don't really understand this patch... :/  I really hate adding even more special casing code like this -- it is so fragile and I don't think we have good test coverage to see what we'll break due to this.

roc: do you understand?

Comment 36

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

(In reply to comment #26)
> (In reply to comment #25)
> > Yeah, Acid2 does use fonts that are very small but non-zero in size. But why
> > would those fonts have a large underline offset? Is it because the platform
> > forces the minimum underline offset to be 1 or something?
> 
> If ascent is 1px and descent is 0px, we need 4px in current trunk:
> 1px: overline
> 1px: ascent (and baseline)
> 1px: gap for underline
> 1px: underline

I don't understand why we need to do this for non-blacklisted fonts.

Comment 37

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

So my preferred fix would be to move the ascent/descent adjustment logic down to gfxFontGroup, where we know whether the fontgroup has a bad font, and then make sure we don't adjust the ascent and descent unless the fontgroup has a bad font.

Comment 38

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

(In reply to comment #36)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Yeah, Acid2 does use fonts that are very small but non-zero in size. But why
> > > would those fonts have a large underline offset? Is it because the platform
> > > forces the minimum underline offset to be 1 or something?
> > 
> > If ascent is 1px and descent is 0px, we need 4px in current trunk:
> > 1px: overline
> > 1px: ascent (and baseline)
> > 1px: gap for underline
> > 1px: underline
> 
> I don't understand why we need to do this for non-blacklisted fonts.

First, I misunderstood that, the correct meaning are:

When font-size: 1px or 2px on Windows Build (depend on the font??)
1px: ascent and overline
1px: ascent and line-through
1px: gap for underline
1px: underline

the descent may be 0.

I'm not sure why the ascent is 2px at |font-size:1px;|, but it should depend on the font.

the gap is created by us for readability at gfxFont::SanitizeMetrics(). (i.e., |aMetrics->underlineOffset = PR_MIN(aMetrics->underlineOffset, -1.0);|)

And underline height is 1px at least.

So, we need 2px at least for underline drawing area below the baseline. Therefore, the computed maxDescent is two pixels at least even if the descent is less than 2px.

So, I didn't assume that the maxDescent can be less than 2px. Therefore, when the maxDescent is less than 2px, we need special cases:

1. when the descent space is 1px, we need to move the underline to there.
2. when the descent space is 0, we need to move the underline to the bottom of ascent space.

Therefore I'm using such code:
|aMetrics->underlineOffset = aMetrics->maxDescent >= 1.0 ? 0 : 1.0;|
0 means the top of the descent space.
1.0 means the bottom of the ascent space.

By this patch, the above case is drawing as:

1px ascent and overline
1px ascent and line-through and underline

Comment 39

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

Then I think SanitizeMetrics should just avoid moving the overline and strikeout area above the font's maxAscent. And it should avoid moving the underline area below the font's maxDescent, unless aIsBadUnderline font is set. How does that sound?

Comment 40

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

(In reply to comment #39)
> Then I think SanitizeMetrics should just avoid moving the overline and
> strikeout area above the font's maxAscent. And it should avoid moving the
> underline area below the font's maxDescent, unless aIsBadUnderline font is set.
> How does that sound?

I can not understand well, and we can ignore the aIsBadUnderline at very small fonts. Because that is used for the readability. But very small fonts cannot read by everybody. My patch doesn't expand the maxAscent/maxDescent from original size. I only move the decoration lines to font box.

Comment 41

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

-    gfxMatrix matrix = aContext->CurrentMatrix();
-    aContext->IdentityMatrix();
     cairo_glyph_t glyph;
     glyph.index = aGlyphID;
     glyph.x = 0;
     glyph.y = 0;
     cairo_text_extents_t extents;
     cairo_glyph_extents(aContext->GetCairo(), &glyph, 1, &extents);
-    aContext->SetMatrix(matrix);

This shouldn't be part of your patch, by the way.

+    enum { MAX_UNDERLINE_OFFSET = 1 };
     gfxFloat GetUnderlineOffset() {
-        if (mStyle.size != 0 && mUnderlineOffset == 0)
+        if (mUnderlineOffset > MAX_UNDERLINE_OFFSET)

This is confusing. Instead of MAX_UNDERLINE_OFFSET, you should use a special value UNDERLINE_OFFSET_NOT_SET to indicate when the offset is not set.

+    // If the maxAscent is too small for painting strikeuout line, we should use this special case.

Typo.

I actually think your patch is pretty close to what I want, but I want it to be organized differently. The important thing is to ensure that the text-decorations do not go outside the font maxAscent/maxDescent; the patch should say that in comments and the code should be organized that way.

I think what I want is that
    aMetrics->strikeoutSize = PR_MAX(1.0, aMetrics->strikeoutSize);
should only happen if the resulting strikeout fits in the ascent+descent.
Similarly,
    aMetrics->underlineSize = PR_MAX(1.0, aMetrics->underlineSize);
    aMetrics->underlineOffset = PR_MIN(aMetrics->underlineOffset, -1.0);
should only happen if the resulting underline fits in the ascent+descent.

And the last
        aMetrics->underlineOffset = PR_MIN(aMetrics->underlineOffset, -1.0);
should probably not happen at all, as in your patch.

Comment 42

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

Basically, if SanitizeMetrics might do something bad, it should just do nothing
instead, because that's safe.

Comment 43

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

Attachment: Patch v2.0

Roc. How about this patch?

The logic of this patch doesn't depend on the font-size.

Comment 44

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

And when I'm testing this, some text in the testcase (attachment 314208 [details]) is not displayed correct size. See the right side of attachment 314209 [details], the fifth line is rendered wrong size. It looks like textrun or fonts cache have a bug, do you know such bug? 

Comment 45

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

I only can reproduce a bug. That is the chin being 1px tall. My patch can fix the bug. However, the nose issue is not fixed by my patch. And also I'm not sure other issues in the screenshots are fixed completely.

Comment 46

[Stuart Parmenter]

Comment on attachment 314406 [details] [diff] [review]
Patch v2.0

I don't understand how UnderlineOffset == 2 means it isn't set?

Comment 47

[Sven E.]

i don´t think there has to be a fix for the nose as long as other browsers (e.g. Safari) even haven´t got a pixel-perfect nose. Opera is the only browser i know who has a pixel-perfect acid2-face

Comment 48

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

(In reply to comment #46)
> (From update of attachment 314406 [details] [diff] [review])
> I don't understand how UnderlineOffset == 2 means it isn't set?

Yes. In my logic, the max value of underlineOffset is 1.0. See here:

     // If underline positioned is too far from the text, descent position is preferred so that underline
     // will stay within the boundary.
     else if (aMetrics->underlineSize - aMetrics->underlineOffset > aMetrics->maxDescent) {
+        if (aMetrics->underlineSize > aMetrics->maxDescent)
+            aMetrics->underlineSize = PR_MAX(aMetrics->maxDescent, 1.0);
+        // The max underlineOffset is 1px (the min underlineSize is 1px, and min maxDescent is 0px.)
         aMetrics->underlineOffset = aMetrics->underlineSize - aMetrics->maxDescent;

Comment 49

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

(In reply to comment #47)
> i don´t think there has to be a fix for the nose as long as other browsers
> (e.g. Safari) even haven´t got a pixel-perfect nose. Opera is the only browser
> i know who has a pixel-perfect acid2-face

The nose is fine as-is.

Comment 50

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

+    enum { UNDERLINE_OFFSET_NOT_SET = 2 };

This would make more sense if it was PR_INT16_MAX or something.

+        if (aMetrics->underlineSize > aMetrics->maxDescent)
+            aMetrics->underlineSize = PR_MAX(aMetrics->maxDescent, 1.0);
+        // The max underlineOffset is 1px (the min underlineSize is 1px, and min maxDescent is 0px.)
         aMetrics->underlineOffset = aMetrics->underlineSize - aMetrics->maxDescent;

This is pretty good but it's still going to have the underline overflowing vertically if the font maxAscent+maxDescent is less than 1.0 (and greater than 0.0).

The strikeout computation has a similar problem.

Why aren't we dealing with overline here too?

Comment 51

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

(In reply to comment #50)
> This is pretty good but it's still going to have the underline overflowing
> vertically if the font maxAscent+maxDescent is less than 1.0 (and greater than
> 0.0).

I assumed that the ascent is always larger than 1px. If that is less than 1px, can I ignore the decoration lines? I.e., underlineSize = strikeoutSize = 0;
If that's not ok, I need to increase the ascent to 1px. But if the descent is not zero. I have no idea for this issue...

> Why aren't we dealing with overline here too?

Oh, underlineSize should be changed at strikeout processing too. Thanks.

Comment 52

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

Attachment: Patch v2.1

If the ascent is less than 1.0px, this patch gives up to rendering the decoration lines.

Comment 53

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

Comment on attachment 314868 [details] [diff] [review]
Patch v2.1

+    if (aMetrics->maxAscent < 1.0) {
+        // We cannot draw strikeout line and overline in the ascent...
+        aMetrics->underlineSize = 0;
+        aMetrics->underlineOffset = 0;
+        aMetrics->strikeoutSize = 0;
+        aMetrics->strikeoutOffset = 0;

You could just return here so you don't have to check maxAscent again below. Then you would remove the "else" from before the next "if".

+        if (aMetrics->underlineOffset > 0 && aMetrics->maxAscent < 1.0) {

And aMetrics->maxAscent can't be < 1.0 here so this code can be removed.

Looks good with that. Land it!

Comment 54

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

Attachment: Patch v2.2

thank you, roc.

Comment 55

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314967 [details] [diff] [review]
Patch v2.2

a1.9=beltzner

Comment 56

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

checked-in.

Comment 57

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

Note that we cannot support Acid2 test with some "bad" underline fonts (see font.blacklist.underlie_offset). If you have some ideas for this, please file a new bug and assign me to the bug.