Acid2 chin is 1px too tall in FF3b5

RESOLVED FIXED

Status

Core Graveyard
GFX
P2
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Sven E., Assigned: masayuki)

Tracking

(Blocks: 1 bug, {qawanted, regression})

Trunk
x86
Windows XP
qawanted, regression
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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)?
Blocks: 289480

Comment 2

9 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression

Comment 3

9 years ago
Created attachment 313243 [details]
Screenshot

Comment 4

9 years ago
Created attachment 313245 [details]
Better screenshot

Screwed the last one up, here is a better screenshot
Attachment #313243 - Attachment is obsolete: true

Comment 5

9 years ago
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

9 years ago
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.

Updated

9 years ago
Component: General → Layout: Block and Inline
Product: Firefox → Core
QA Contact: general → layout.block-and-inline
Version: unspecified → Trunk

Comment 7

9 years ago
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.)
Flags: blocking1.9?
If bug 413787 is the cause, you're only going to see this bug on Windows.

Updated

9 years ago
Blocks: 413787
Component: Layout: Block and Inline → GFX
QA Contact: layout.block-and-inline → general
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.
No longer blocks: 413787
(Reporter)

Comment 10

9 years ago
Created attachment 313299 [details]
Test-face with additional line
(Reporter)

Comment 11

9 years ago
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

9 years ago
(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

9 years ago
Created attachment 313401 [details]
Firefox 3.0b5 vs. Reference Rendering

Note how Firefox adds an extra row to the jaw.
Vlad, can you take a look here?
Assignee: nobody → vladimir
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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!)
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).
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?
Ok, marking qawanted -- really need a regression range, given that it's not bug 413787.
Keywords: qawanted
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
Thanks, Luke!  So looks like bug 421353?  (The nose isn't a problem.)  Cc'ing masayuki and roc.
Blocks: 421353
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...
(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.

Updated

9 years ago
Assignee: vladimir → masayuki
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.
er, if the font size is very small (less than 3px?), the patch might change the layout with non-blacklisted fonts..
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?
(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.
> 1. we should contain the bottom of 1px in ascent space

1. we should contain the bottom (1px) of the overline in ascent space
(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.
Created attachment 314207 [details] [diff] [review]
Patch v1.0

I think that this fixes the issue. (I'm not sure this can fix this bug, because I cannot reproduce it.)
Attachment #314207 - Flags: review?(pavlov)
Created attachment 314208 [details]
testcase
Created attachment 314209 [details]
screenshot of attachment 314208 [details] (left: current trunk, right: patched build)
Comment on attachment 314207 [details] [diff] [review]
Patch v1.0

mmm... sorry for the spam.
I need more works...
Attachment #314207 - Flags: review?(pavlov)
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.
Attachment #314207 - Flags: review?(pavlov)
Created attachment 314272 [details] [diff] [review]
Patch v1.1

oops, the previous patch has a bug.
Attachment #314207 - Attachment is obsolete: true
Attachment #314272 - Flags: review?(pavlov)
Attachment #314207 - Flags: review?(pavlov)

Comment 35

9 years ago
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?
Attachment #314272 - Flags: superreview?(roc)
(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.
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.
(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
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?
(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.
-    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.
Basically, if SanitizeMetrics might do something bad, it should just do nothing
instead, because that's safe.
Created attachment 314406 [details] [diff] [review]
Patch v2.0

Roc. How about this patch?

The logic of this patch doesn't depend on the font-size.
Attachment #314272 - Attachment is obsolete: true
Attachment #314406 - Flags: superreview?(roc)
Attachment #314272 - Flags: superreview?(roc)
Attachment #314272 - Flags: review?(pavlov)
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? 
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.

Updated

9 years ago
Summary: Acid2 incorrect Rendering on FF3b5 → Acid2 chin is 1px too tall in FF3b5

Comment 46

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

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

Comment 47

9 years ago
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
(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;
(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.
+    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?
(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.
Created attachment 314868 [details] [diff] [review]
Patch v2.1

If the ascent is less than 1.0px, this patch gives up to rendering the decoration lines.
Attachment #314406 - Attachment is obsolete: true
Attachment #314868 - Flags: superreview?(roc)
Attachment #314406 - Flags: superreview?(roc)
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!
Attachment #314868 - Flags: superreview?(roc) → superreview+
Created attachment 314967 [details] [diff] [review]
Patch v2.2

thank you, roc.
Attachment #314868 - Attachment is obsolete: true
Attachment #314967 - Flags: review?(pavlov)

Updated

9 years ago
Attachment #314967 - Flags: review?(pavlov) → review+
Attachment #314967 - Flags: approval1.9?
Comment on attachment 314967 [details] [diff] [review]
Patch v2.2

a1.9=beltzner
Attachment #314967 - Flags: approval1.9? → approval1.9+
checked-in.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.