400813 - Missing line from the Acid2 face

Status: VERIFIED FIXED

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

Description

[Lior Halphon (LIJI)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8) Gecko/2007091216 GranParadiso/3.0a8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8) Gecko/2007091216 GranParadiso/3.0a8

One line is missing from the Acid2 Face when using Gran Paradiso.
You're almost there, don't give up! :)

Reproducible: Always

Steps to Reproduce:
1. Visit http://www.webstandards.org/files/acid2/test.html
Actual Results:  
One line (The one before last) is missing from the face.

Expected Results:  
Render the missing line.

Might be my computer's or Gran Paradiso's settings.

Comment 1

[Lior Halphon (LIJI)]

Attachment: Comparison of the Gran Paradiso rendering with the rendering reference

The left image is how GP renders the Acid Test while the right image is the rendering reference with the missing line highlighted.

Comment 2

[Jo Hermans]

worsk for me in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102205 Minefield/3.0a9pre

Comment 3

[Lior Halphon (LIJI)]

Upgrading to the same agent doesn't solve the problem.

Comment 4

[Lior Halphon (LIJI)]

Switching to another user (without a profile folder) seems to render it correctly. I'll try to find out what is the bugging setting and report it.

Comment 5

[Lior Halphon (LIJI)]

Although switching to another user fixes the problem, deleting the profile folder does not fix the problem. Are there any user specific settings that are not stored in the Profile folder?

Comment 6

[Ria Klaassen (not reading all bugmail)]

Appears to be caused by a minimum font-size setting.

Comment 7

[Lior Halphon (LIJI)]

I have not set any minimum font size.

Comment 8

[David McWilliams]

I am also missing this line in acid2.

I recently upgraded from firefox 2 to firefox 3 beta 1.

Comment 9

[Marco Bonardo [:mak]]

WFM on current nightly
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre

Comment 10

[Boris Zbarsky [:bzbarsky]]

What fonts do people have selected?  See also the comments in bug 365809.

Comment 11

[David McWilliams]

I am using Georgia font.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er... I meant bug 404698.  See bug 404698 comment 6, and following.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Confirming this bug to cover the disappearing-chin issue, which seems to be Windows-only.  I got a frame dump of the minimal testcase in bug 404698 from Alex Vincent, and it looks like Lucy's bug 404698 comment 9 was spot-on.  The relevant part of the frame dump is:

  Block(div)(1)@05B0648C next=05B0634C {0,0,5760,0} [state=00100000]
   sc=05B06268(i=1,b=0)<
     line 05B06640: count=1 state=inline,clean,prevmarginclean,
      not impacted,not wrapped,before:nobr,after:nobr[0x4100] {0,0,0,0} <
        Inline(div)(0)@05B065B8 {0,-120,0,180} [content=05ADFCE8] [sc=05B0650C]<
          Text(0)@05B063DC[0,1,T]  {0,0,0,120} [state=10200200] SELECTED
           [content=0622C148] sc=05B065F0 pst=:-moz-non-element<

The key point is that the text has width 0 (presumably because it's a narrow font, at a 2px font size).  So the child inline ends up with a width of 0 (and a position of -2px and height of 3px).  Then the block ends up with a height of 0, presumably because when we get to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsLineLayout.cpp&rev=3.285&mark=2093#2092 we have |psd->mX == psd->mLeftEdge|.  So the code hits the "else" case and sets minY = maxY = 0.

I'm not sure that's entirely correct per CSS2.1, actually...

That said, does it really make sense to return 0px for the width of a char in this case? I'm actually a little surprised we're not doing "subpixel" font layout here, certainly in CSS pixels. 2 CSS pixels might be a whole lot of device pixels on a high-res device!

Comment 14

[David McWilliams]

In reply to Hixie: https://bugzilla.mozilla.org/show_bug.cgi?id=404698#c40

Yes, this test works using Georgia / Size 16.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Yes, this test works using Georgia / Size 16.

That's because Georgia is your serif font and the test in question no longer uses the serif font...  

Comment 16

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

Windows GDI will only give us font metrics in pixels. That's why we get width 0.

Instead of comparing mX to mLeftEdge, we should probably be checking whether nonempty content has been placed.

However, should this bug really block 1.9? It's certainly not a regression. Is it worth fixing just for acid2 bragging rights for a certain set of fonts on Windows?

Comment 17

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

Attachment: testcase

This testcase is a bit more focused. I think we should have a 100px blank line at the top, but we don't. Boris, do you agree?

Comment 18

[Boris Zbarsky [:bzbarsky]]

roc, you attached a patch for that assertion bug instead of your testcase....

For the rest, if this is not a cairo regression it probably shouldn't block.

Comment 19

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

Attachment: real testcase

I'm so sad.

There may have been something masking this bug before, but the code at fault is definitely ancient.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yeah, I would say that should have a 100px blank line.

Comment 21

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

Alright, I'll take this just for the sake of acid2 bragging

Comment 22

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

Attachment: fix

This does it. I think it's reasonably straightforward. There's still bogus code lying around checking for zero widths and heights etc, but I don't want to fix more than necessary here.

I had to fix a couple of the reftests. The 398144-1 test had an empty inline-block that we were assuming ended up in a zero-height line, now we give that line normal height. The new behaviour matches Safari 3 and I think it's correct. The non-breakable-1 test is strange, it seems to fail as-is with this patch, but I think that's related to bug 412859 (which is a trunk bug). Anyway I'll get a fix for that before I submit this for review.

Comment 23

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

Attachment: safety checks in Deflate()

These should go in too.

Comment 24

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

bug 412859 is diagnosed and very definitely is not caused by my patch here, so I guess we should just go ahead here.

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 297673 [details] [diff] [review]
safety checks in Deflate()

Looks good, but I don't like having a mix of MAX and PR_MAX in this file.
Please use MAX, or change the existing MIN/MAX to PR_* and remove this block:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/gfx/src/nsRect.cpp&rev=3.22&root=/cvsroot&mark=42-53#38

r+sr=mats with that fixed.

Comment 26

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

Checked in the Deflate checks.

Comment 27

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

Comment on attachment 297631 [details] [diff] [review]
fix

In nsLineLayout::PlaceFrame:
>   if (!emptyFrame) {
>     mTotalPlacedFrames++;
>+    psd->mHasNonemptyContent = PR_TRUE;
>   }

In what cases would you need this where the code in nsLineLayout::ReflowFrame wouldn't be sufficient?

>+  // XXX roc why not just check !emptyFrame again here?

I'd skip this, since we really need to be able to place floats much more often (not less). (bug 50630)

Comment 28

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

And could you leave a comment in the list of bits in nsTextFrameThebes.cpp with the one you defined in nsTextFrame.h listed there, commented out, with a pointer to nsTextFrame.h ?

Comment 29

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

(In reply to comment #27)
> (From update of attachment 297631 [details] [diff] [review])
> In nsLineLayout::PlaceFrame:
> >   if (!emptyFrame) {
> >     mTotalPlacedFrames++;
> >+    psd->mHasNonemptyContent = PR_TRUE;
> >   }
> 
> In what cases would you need this where the code in nsLineLayout::ReflowFrame
> wouldn't be sufficient?

You're right, it's not needed. I'll take it out. I suppose I could assert psd->HasNonemptyContent here.

> >+  // XXX roc why not just check !emptyFrame again here?
> 
> I'd skip this, since we really need to be able to place floats much more often
> (not less). (bug 50630)

OK, I'll take that comment out.

(In reply to comment #28)
> And could you leave a comment in the list of bits in nsTextFrameThebes.cpp
> with the one you defined in nsTextFrame.h listed there, commented out, with a
> pointer to nsTextFrame.h ?

Sure.

Comment 30

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

Comment on attachment 297631 [details] [diff] [review]
fix

OK, r+sr=dbaron with those changes and the assertion added.

Comment 31

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

Checked in with reftest.

Comment 32

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

backed out, that assertion I added fired.

Comment 33

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

The assertion fires when we have an empty inline frame. It can have nonzero height, so nsLineLayout::PlaceFrame will set emptyFrame to false. But we don't have psd->mHasNonemptyContent set to true because indeed, it shouldn't be. I think we just have to take that assertion out.

Comment 34

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

Attachment: updated fix

This fixes the reftest failures. There were two main issues:
-- we treated nsFirstLetterFrames as empty. Big mistake, we should actually treat them like inlines for determining emptiness.
-- we treated MathML container frames as empty because they return null from GetType(). We should treat null types like unknown types, as non-empty.

Comment 35

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

I also took out the assertion that was invalid.

Comment 36

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

The only changes I see between attachment 297631 [details] [diff] [review] and attachment 300755 [details] [diff] [review] seem to be related to my review comments, not to the issues described in comment 34.  Should the patch contain more files?

Comment 37

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

No, the logic in nsLineLayout::ReflowFrame has changed. In particular the code that sets psd->mHasNonemptyContent is different, it uses a new local variable hasNoncollapsedContent.

Comment 38

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

Comment on attachment 300755 [details] [diff] [review]
updated fix

r+sr=dbaron

Comment 39

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

Relanded

Comment 40

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

I had to back this out again due to a reftest failure...

REFTEST UNEXPECTED FAIL: file:///C:/slave/trunk_2k3/mozilla/layout/reftests/bugs/413840-pushed-line-bullet.html

Comment 41

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

Looks like the test lost its bullet or something.

Comment 42

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

Are the two conditions for zero height lines here not equivalent anymore?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.931&mark=3967-3971#3948

That code is intended to ensure that we place the bullet on either the first or second line.

Comment 43

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

Attachment: updated again

OK, I found the bug. When we have a frame with nonempty content in nsLineLayout::ReflowFrame, we shouldn't mark its containing span as having nonempty content until we're sure that the frame actually fits on the line, i.e. after CanPlaceFrame has returned true!

Comment 44

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

I relanded about 10 hours ago with that fix and tests passed. Just now I reenabled the reftest.

Comment 45

[(no longer active)]

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

Verified.