499841 - "ASSERTION: FirstLetterStyle set on line with floating first letter" moving <style>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: FirstLetterStyle set on line with floating first letter: '!(outOfFlowFrame->GetType() == nsGkAtoms::letterFrame && GetFirstLetterStyleOK())', file /Users/jruderman/central/layout/generic/nsLineLayout.cpp, line 886

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

We just pass a parent frame to RemoveFirstLetterFrames, and it tries to remove the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit on that, instead of the blockframe.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 407029 [details] [diff] [review]
patch

Doh.  Nice catch!  Add a crashtest?

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to comment #2)
> Doh.  Nice catch!  Add a crashtest?

Yes of course. I'm too used to fixing bugs that can't have a test for some reason or other.

Comment 4

[Timothy Nikkel (:tnikkel)]

Try server caught a bug that was being masked by the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit not getting unset in some cases. The reftest 408493-2.html doesn't apply the first-letter style after the dynamic change. This happens because the text frame that contains the text that becomes the new first letter is in the second column continuation of the block frame, so we set the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit on the second column, and when we reflow the first letter frame gets pulled into the first column (the second column goes away), and the bit doesn't make the trip. I think, but am not sure, that the correct fix is to always set the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit on the first continuation, so that when we reflow it we pull the first letter frame from the next-in-flow as need be. But that then triggers this assertion in CreateLetterFrame

    NS_ASSERTION(aBlockFrame == GetFloatContainingBlock(aParentFrame),
                 "Containing block is confused");

(added by bug 425981). If this is the correct approach then I suggest changing that assertion to |aBlockFrame == GetFloatContainingBlock(aParentFrame)->GetFirstContinuation()|.

Comment 5

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

That sounds right.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Updated based on comment 4 so that we always set/unset the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit on the first continuation. Also added the crashtest.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Hrm.  Continuations....  Can you please document that NS_BLOCK_HAS_FIRST_LETTER_CHILD must be on the first continuation only?  What happens if the first letter is not in fact a child of the first continuation? e.g. if you have some markup like so:

<style>
  div::first-letter { color: red; /* whatever it takes to get columns here */ }
</style>
<div>
  abc <span style="float: left; width: /* 1 column wide */; height: /* 2 columns tall */">
</div>

or some such?

Comment 8

[Timothy Nikkel (:tnikkel)]

I don't think such a float can ever push first letter content into the second column. The first letter content must come before the float because otherwise we don't apply the first letter style (the spec seems ambiguous on if this is correct or not: "The ::first-letter pseudo-element represents the first letter of the first line of a block, if it is not preceded by any other content (such as images or inline tables) on its line." Does a float count as content?). And when the float comes after, it can either be beside or below, never above. So the float would be pushed to the next column, not the potential first letter content.

But I think I know what you are getting at. The text content that becomes the first letter style can be in the second column. This is what happens in the 408493-2.html test as I described in comment 4. Setting the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit tells the frame when it is reflowing that it needs to have a first letter, so it does extra pulling if it needs to to get one. For example, look at the reflowingFirstLetter variable in nsInlineFrame::ReflowFrames and nsInlineFrame::ReflowInlineFrame.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't think such a float can ever push first letter content into the second
> column.

Ah, right you are (the float gets pushed down instead).

> Setting the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit tells the frame when it is
> reflowing that it needs to have a first letter, so it does extra pulling if it
> needs to.

OK.  Can the first-letter end up in the third column?  If so, does the second column need to get the bit set too?  If so, how does it get there?

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: more columns testcase

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to comment #9)
> OK.  Can the first-letter end up in the third column?  If so, does the second
> column need to get the bit set too?  If so, how does it get there?

In static situations the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit get sets on all the columns because we copy it when making the continuations. So the "more columns testcase" I attached throws in a float at the start to stop it from being set. Killing the float sets the bit on the first column. Then we kill all but the last column. The last "x" will eventually get pulled into the first column and so it works.

But as I mentioned, the NS_BLOCK_HAS_FIRST_LETTER_CHILD bit get set on all continuations in static situations, so there doesn't seem to be any harm in setting it on continuations as long as it is set on the first. I can set the bit on all continuations until we get to the one where we actually create the letter frame if you'd like.

Side note: this testcase exposes two more bugs (thankfully not part of this bug). We don't invalidate the column rules between columns when columns get removed (after I uploaded the attachment I realized you need to add a few more x's at the end of the div to see the problem). And, although we apply first letter styling when removing a float from the start, we don't remove first letter styling when adding a float to the start.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I guess the key is that if the first column didn't end up with the first-letter neither will the second column, so not setting the bit on the second column doesn't hurt us?

> I can set the bit on all continuations until we get to the one where we
> actually create the letter frame if you'd like.

No need to do that if you can prove to me that it's not needed, but otherwise that would be the thing to do, yes.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to comment #12)
> I guess the key is that if the first column didn't end up with the first-letter
> neither will the second column, so not setting the bit on the second column
> doesn't hurt us?

Yeah. If the first column doesn't have anything that could be the first letter, then I don't think there could be anything that could be the first letter in the whole block.

> > I can set the bit on all continuations until we get to the one where we
> > actually create the letter frame if you'd like.
> 
> No need to do that if you can prove to me that it's not needed, but otherwise
> that would be the thing to do, yes.

I'll do it, since I don't think I can prove it, or want to spend more time trying to.

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to comment #13)
> > > I can set the bit on all continuations until we get to the one where we
> > > actually create the letter frame if you'd like.
> > 
> > No need to do that if you can prove to me that it's not needed, but otherwise
> > that would be the thing to do, yes.
> 
> I'll do it, since I don't think I can prove it, or want to spend more time
> trying to.

Hmm, that's not as straight forward as I thought, and it leads to another issue: if we set the bit on the first and second continuation, then pull the first letter into the first continuation, and then get rid of the first letter style altogether, the second continuation will still have the bit set, unless we keep going after finding and removing the letter frame just to unset the bits.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK.  Can we just declare that this bit must only ever be set on the first continuation, enforce that when copying bits to continuations and just set it on the first one here?  And maybe assert somewhere that it's not set on other continuations?

Comment 16

[Boris Zbarsky [:bzbarsky]]

By the way, sorry for all the questions....  This stuff is not exactly the clearest code in the world.  :(

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: patch v3

(In reply to comment #15)
> OK.  Can we just declare that this bit must only ever be set on the first
> continuation, enforce that when copying bits to continuations and just set it
> on the first one here?  And maybe assert somewhere that it's not set on other
> continuations?

Done and done.

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to comment #16)
> By the way, sorry for all the questions....  This stuff is not exactly the
> clearest code in the world.  :(

If this is because of me saying I didn't want to spend anymore time above, that was annoyance with this bug exposing other bugs and not wanting to die, which is not your fault.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Nah, it was just a general comment on this code.  ;)

Comment 20

[Timothy Nikkel (:tnikkel)]

Ah, a more careful reading of the spec:

"The ::first-letter pseudo-element represents the first letter of the first line of a block..."

The first column must contain the first line, so if the potential first letter content isn't in the first column it can't be in the first line and hence can't be the first letter. So just setting the bit on the first continuation should be fine. And this is what we currently do, if some whitespace: pre spaces push the first letter content to the second line of a block (even without columns) it doesn't get the special first letter style.

Comment 21

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/18374e0a18b9

Comment 22

[Timothy Nikkel (:tnikkel)]

And just remembered now to address comment 2 by adding a crashtest.
http://hg.mozilla.org/mozilla-central/rev/074108bd5051

Comment 23

[Timothy Nikkel (:tnikkel)]

Pushed http://hg.mozilla.org/mozilla-central/rev/1db0ecd97b22 to address a bad merge.