Missing line from the Acid2 face

VERIFIED FIXED

Status

()

Core
Layout: Block and Inline
P2
normal
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Lior Halphon (LIJI), Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 285858 [details]
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

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

Comment 3

10 years ago
Upgrading to the same agent doesn't solve the problem.
(Reporter)

Comment 4

10 years ago
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.
(Reporter)

Comment 5

10 years ago
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?
Appears to be caused by a minimum font-size setting.
(Reporter)

Comment 7

10 years ago
I have not set any minimum font size.

Comment 8

10 years ago
I am also missing this line in acid2.

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

Comment 9

10 years ago
WFM on current nightly
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre
What fonts do people have selected?  See also the comments in bug 365809.

Comment 11

10 years ago
I am using Georgia font.
Er... I meant bug 404698.  See bug 404698 comment 6, and following.
Status: UNCONFIRMED → NEW
Component: General → Layout: Block and Inline
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → layout.block-and-inline
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!
Flags: blocking1.9?

Comment 14

10 years ago
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
> 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...  
Flags: blocking1.9? → blocking1.9+
Priority: -- → P5
Version: unspecified → Trunk

Updated

10 years ago
Blocks: 289480
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?
Created attachment 296269 [details] [diff] [review]
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?
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.
Created attachment 296286 [details]
real testcase

I'm so sad.

There may have been something masking this bug before, but the code at fault is definitely ancient.
Attachment #296269 - Attachment is obsolete: true
Yeah, I would say that should have a 100px blank line.
Alright, I'll take this just for the sake of acid2 bragging
Assignee: nobody → roc
Created attachment 297631 [details] [diff] [review]
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.
Depends on: 412859
Created attachment 297673 [details] [diff] [review]
safety checks in Deflate()

These should go in too.
Attachment #297673 - Flags: superreview?(mats.palmgren)
Attachment #297673 - Flags: review?(mats.palmgren)
Attachment #297631 - Flags: superreview?(dbaron)
Attachment #297631 - Flags: review?(dbaron)
bug 412859 is diagnosed and very definitely is not caused by my patch here, so I guess we should just go ahead here.
Whiteboard: [needs review]
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.
Attachment #297673 - Flags: superreview?(mats.palmgren)
Attachment #297673 - Flags: superreview+
Attachment #297673 - Flags: review?(mats.palmgren)
Attachment #297673 - Flags: review+
Checked in the Deflate checks.
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)
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 ?
(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 on attachment 297631 [details] [diff] [review]
fix

OK, r+sr=dbaron with those changes and the assertion added.
Attachment #297631 - Flags: superreview?(dbaron)
Attachment #297631 - Flags: superreview+
Attachment #297631 - Flags: review?(dbaron)
Attachment #297631 - Flags: review+
Checked in with reftest.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
backed out, that assertion I added fired.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: P5 → P2
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.
Created attachment 300755 [details] [diff] [review]
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.
Attachment #300755 - Flags: superreview?(dbaron)
Attachment #300755 - Flags: review?(dbaron)
I also took out the assertion that was invalid.
Whiteboard: [needs review]
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?
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 on attachment 300755 [details] [diff] [review]
updated fix

r+sr=dbaron
Attachment #300755 - Flags: superreview?(dbaron)
Attachment #300755 - Flags: superreview+
Attachment #300755 - Flags: review?(dbaron)
Attachment #300755 - Flags: review+
Relanded
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs review]
Looks like the test lost its bullet or something.
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.
Created attachment 303183 [details] [diff] [review]
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!
Attachment #300755 - Attachment is obsolete: true
Whiteboard: [needs landing]
I relanded about 10 hours ago with that fix and tests passed. Just now I reenabled the reftest.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]

Updated

10 years ago
Depends on: 417839

Updated

10 years ago
Depends on: 419545

Updated

10 years ago
Depends on: 420351
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050810 Minefield/3.0pre

Verified.
Status: RESOLVED → VERIFIED
Depends on: 455855
Depends on: 473410
You need to log in before you can comment on or make changes to this bug.