Created attachment 366493 [details] testcase ###!!! ASSERTION: Unexpected frame types: '(frame1->GetType() == nsGkAtoms::tableFrame && frame2->GetType() == nsGkAtoms::tableOuterFrame) || (frame1->GetType() == nsGkAtoms::tableOuterFrame && frame2->GetType() == nsGkAtoms::tableFrame) || frame1->GetType() == nsGkAtoms::fieldSetFrame || (frame1->GetParent() && frame1->GetParent()->GetType() == nsGkAtoms::fieldSetFrame)', file /Users/jruderman/central/layout/base/nsCSSFrameConstructor.cpp, line 8002 Also, the letter 'c' appears in the wrong place. This assertion was added in bug 480979 part 6: http://hg.mozilla.org/mozilla-central/rev/159168c61b02
Hey, good news and bad news. Bad news is it asserts. Good news is that the assert and the wrong place bug are the same issue, I would hope, and the wrong place bug is a regression from Fx2 to Fx3. We just finally caught it. That bug was intruduced between 2007-08-02-04 and 2007-08-03-04. Bonsai query: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-02+04&maxdate=2007-08-03+04&cvsroot=%2Fcvsroot I blame bug 386014 at first blush, as something that touched the relevant code.
And I bet the reason the wrapping matters is that prevSibling->GetParent() is before the break while our parentFrame is after the break. Which really means that we're using the wrong parentFrame, of course.
Yeah, this is kinda sad. On entry to ContentInserted, the prevSibling is the textframe for "B" and the parentFrame is the Letter frame around it. Then the RemoveLetterFrames call not only removes all those letter frames, but merges the textframes for A and B (which used to have different parents). However, the inline that used to be the Letter frame's parent (and which we assigned to parentFrame before removing the Letter frames) is still there. But it is not in fact the parent of the prevSibling we now compute (which is the textframe for "A B"). We used to blithely ignore this and just insert in parentFrame with the bogus prevSibling, parented the new frame to parentFrame, but then stuck it in the child list of parentFrames prev-in-flow. Luckily during the next reflow it got pushed to the next line again and hence ended up in the correct parent.
9 years ago
Created attachment 366734 [details] [diff] [review] Proposed fix Basic idea here is to just stop trying to be clever about first-letter and properly reget our prevsibling and parent.
Comment on attachment 366734 [details] [diff] [review] Proposed fix r+sr=dbaron; sorry about the delay
Created attachment 374389 [details] [diff] [review] Merged to tip
Pushed http://hg.mozilla.org/mozilla-central/rev/af71fccc01be It's probably too late in the 1.9.1 cycle to take this there... though maybe it makes sense to fix the correctness problem?
I really don't know this code well enough to have an opinion on that, nor do I really know what a fix for just the correctness problem would look like.
It would look like this patch, basically.