Closed Bug 457398 Opened 11 years ago Closed 11 years ago

ASSERTION: First-continuation first-letter should have first-letter style enabled in nsLineLayout

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: bernd_mozilla, Assigned: roc)

References

()

Details

(4 keywords)

Attachments

(3 files, 2 obsolete files)

the testcase  (reftest) from  bug 372553 triggers the following assert

###!!! ASSERTION: First-in-flow first-letter should have first-letter style enabled in nsLineLayout!: 'll->GetFirstLetterStyleOK() || GetPrevInFlow()', file d:/moz_src/src/layout/generic/nsFirstLetterFrame.cpp, line 240
Bug 448610 changed the assertion condition and message, but this testcase still hits it.

###!!! ASSERTION: First-continuation first-letter should have first-letter style enabled in nsLineLayout!: 'll->GetFirstLetterStyleOK() || GetPrevContinuation()', file /Users/jruderman/central/layout/generic/nsFirstLetterFrame.cpp, line 240
Keywords: assertion, testcase
OS: Windows XP → All
Hardware: PC → All
Summary: ASSERTION: First-in-flow first-letter should have first-letter style enabled in nsLineLayout → ASSERTION: First-continuation first-letter should have first-letter style enabled in nsLineLayout
Is it just me, or is the rendering of that testcase incorrect? (I see Bbbbbbbbbbbbbb all blue in both the testcase and the reference)
I see the incorrect blueness too.
Relevant part of the frametree:

            line 0x1610c34: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8100] {60,60,6552,840} <
              Inline(span)(1)@0x1610858 next=0x160d270 prev-continuation=0x160cf20 {60,60,6552,840} [state=00000004] [content=0x7258b70] [sc=0x160cde4]
<
                Letter(span)(1)@0x16107a8 next=0x160d238 {0,0,6552,840} [content=0x7258b70] [sc=0x160d090] pst=:first-letter
<
                  Text(1)@0x160d484[0,14,T]  {0,0,6552,840} [state=80200010] [content=0x7259b80] sc=0x160d118 pst=:-moz-non-element<
                    "Bbbbbbbbbbbbbb"
                  >
                >
                Inline(span)(2)@0x160d238 {6552,0,0,840} [content=0x7259bc0] [sc=0x160d008]

<>
              >
              Text(2)@0x160d270[0,2,T]  next=0x160d168 {6612,60,0,840} [state=80400000] [content=0x7259be0] sc=0x160cd94 pst=:-moz-non-element<
                "\n\n"
              >
            >


So yes, it's wrong.  So is the frame tree.  I would assume due to this assertion, since I bet that prevents us creating the overflow stuff for the rest of the text....
Flags: blocking1.9.1?
And the reason we screw _that_ up is that in nsBlockFrame::DoReflowInlineFrames we have aLineLayout.GetLineNumber() == 1, at least if the frame tree has anything to do with the situation:

          Block(body)(1)@0x160ce34 {480,480,4020,960} [state=a0100008] [overflow=0,0,6612,960] sc=0x160cb74(i=3,b=0)<
            line 0x160d300: count=2 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8021] {60,60,0,0} <
              Text(0)@0x160cfa4[0,2,T]  next=0x160cf20 {60,60,0,0} [state=08600000] [content=0x7258b20] sc=0x160cd94 pst=:-moz-non-element<
                "\n\n"
              >
              Inline(span)(1)@0x160cf20 next=0x1610858 next-continuation=0x1610858 {60,-540,0,840} [state=00000010] [content=0x7258b70] [sc=0x160cde4]
<
                Inline(span)(0)@0x160d058 {0,0,0,840} [content=0x7258b90] [sc=0x160d008]

<>
              >
            >
            line 0x1610c34: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8100] {60,60,6552,840} <
              Inline(span)(1)@0x1610858 next=0x160d270 prev-continuation=0x160cf20 {60,60,6552,840} [state=00000004] [content=0x7258b70] [sc=0x160cde4]
<
                Letter(span)(1)@0x16107a8 next=0x160d238 {0,0,6552,840} [content=0x7258b70] [sc=0x160d090] pst=:first-letter
<
                  Text(1)@0x160d484[0,14,T]  {0,0,6552,840} [state=80200010] [content=0x7259b80] sc=0x160d118 pst=:-moz-non-element<
                    "Bbbbbbbbbbbbbb"
                  >
                >
                Inline(span)(2)@0x160d238 {6552,0,0,840} [content=0x7259bc0] [sc=0x160d008]

<>
              >
              Text(2)@0x160d270[0,2,T]  next=0x160d168 {6612,60,0,840} [state=80400000] [content=0x7259be0] sc=0x160cd94 pst=:-moz-non-element<
                "\n\n"
              >
            >
            line 0x1610890: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x4300] {60,900,0,0} <
              Text(4)@0x160d168[0,2,T]  {60,900,0,0} [state=08600000] [content=0x724f3b0] sc=0x160cd94 pst=:-moz-non-element<
                "\n\n"
              >
            >
          >

Why are we ending up with the first-letter on the second line?

In any case, as of July 1 last year this worked correctly in terms of rendering.
Keywords: regression
This time, I'll check in a reftest that uses <span> to highlight the first letter, dammit.  ;)
Presumably we're breaking on the newlines before that first <span>.  Seems to me that breakable whitespace at the immediate start of a block should not be a break opportunity.  Certainly breaking on it makes no sense.  Similar for end of a block.
Minimal-ish testcase for the correctness problem:

data:text/html,<style>body::first-letter { color: blue }</style><body style="width: 0"> <span></span>Test</body>
Assignee: nobody → roc
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
Attached file harder testcase
We don't have to break before "Test" in Boris' testcase, and that's easy to fix. But this testcase is harder; we really do (with our current organization) have to break before "Test" to make sure it gets pushed down below the float.
Attached patch fix (obsolete) — Splinter Review
I think the best thing to do here is simply to not advance the line number when nothing non-empty was placed on the line.
Attachment #348913 - Flags: superreview?(dbaron)
Attachment #348913 - Flags: review?(dbaron)
Attached patch fix v2 (obsolete) — Splinter Review
Oops, I forgot to put the test in the last patch.
Attachment #348913 - Attachment is obsolete: true
Attachment #348916 - Flags: superreview?(dbaron)
Attachment #348916 - Flags: review?(dbaron)
Attachment #348913 - Flags: superreview?(dbaron)
Attachment #348913 - Flags: review?(dbaron)
Whiteboard: [needs review]
Comment on attachment 348916 [details] [diff] [review]
fix v2

This also looks like it should affect text-indent.  After reading section 9.5 of CSS 2.1, the change in terms of text-indent is clearly correct -- in other words, a long first word of a paragraph that can't fit next to a float should be indented when it is pushed below the float.  Could you add a test for that as well?

r+sr=dbaron with that
Attachment #348916 - Flags: superreview?(dbaron)
Attachment #348916 - Flags: superreview+
Attachment #348916 - Flags: review?(dbaron)
Attachment #348916 - Flags: review+
Attached patch fix v1.3Splinter Review
With the extra test.
Attachment #348916 - Attachment is obsolete: true
Attachment #349025 - Flags: superreview+
Attachment #349025 - Flags: review+
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/70a0658284a6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Whiteboard: [needs landing]
Flags: in-testsuite+
pushed onto 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/70a0658284a6


also, verified FIXED on debug builds (no assertion):

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.