Testcase 1 shows the character 'j' being duplicated mysteriously. I have no idea whether the rest of the rendering of this testcase is "correct".

This testcase shows the assertion that first alerted me to the presence of this bug: ###!!! ASSERTION: After-spacing inside a ligature!: 'i - 1 <= aStart || aSpacing[i - 1 - aStart].mAfter == 0', file /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp, line 1542

We seem to get a pretty confused frame model here (e.g. the "jk" textframe ends up between the "A" and "B" textframes?). The content model (including anon content) in DOM inspector looks correct, though...

Boris, sounds like you're probably the best person to look into this. Either something in layout gets really confused, or we double notify on the content side, I guess. Not blocking on this though, unless it turns out to be worse than we think...

The basic issue here was that the frame model looked somewhat like this: block line inline text line inline text placeholder with the inlines anonymous, and "line" being first-line frames. The two inlines and the two texts are in-flows caused by the line-breaking the word-spacing triggers. We found the placeholder as the next sibling, had no prev-sibling, so did an insert at the beginning of the second line frame. That put our new text frame sorta in between the two in-flows, which then confused things all around. The same thing happens if I replace the first-line frame with a <span>, but then at some later point the frame model gets fixed up. Looks like it happens when we ReflowInlineFrame the "A B" textframe, which does a StealFrame on the second part, then we also StealFrame the second part of the inline, then when we don't fit and push the continuations they end up in the right place, before the "jk". So looks like the reason first-line is needed is that the flow works differently there, somehow...

roc, do you think this is serious enough to take on branches?

why can't the prevsibling or nextsibling have continuations?

Because we're going to set the GetNextSibling() of the prevsibling to ourselves or set our GetNextSibling to the nextsibling. We don't want to have two frames with the same GetNextSibling(), nor do we want a situation where GetNextSibling() != nsnull and != GetNextContinuation(), do we? That said, those asserts may not be quite right... FindFrameForContentSibling guarantees that the frame returned is either the primary frame (for nextsibling) or the tail continuation of the last special sibling (for prevsibling). So perhaps I should replace the GetNextContinuation asserts with ones about the tail continuation?

I think those assertions are OK. But this looks fragile to me. We really need ChildIterator to find this anonymous content, don't we?

Fragile, agreed. ChildIterator doesn't seem to have a way of starting at a given node and then going one back or forward from it... If it could, I agree we should use it and rip out all this business of finding anonymous or non-anonymous content before or after. If you prefer I can try to make ChildIterator handle this use case and see how it goes. Just have to be careful to not regress insertion performance in the obvious case of no XBL.

I think that's worth a try.

OK. I have that working, in a totally branch-not-ok way, basically by fixing bug 307394. So I'll move things there, and we can think about fixing this on branches using the hacky patch or something.

Fixed by checkin for bug 307394. Checked in test. Do we want to do the original patch on branch, or just not worry about it?

Comment on attachment 357368 [details] [diff] [review] Make sure to get the right prevSibling Sounds like a "don't worry about it", I guess.