Closed Bug 387227 Opened 17 years ago Closed 15 years ago

"ASSERTION: special frame error" with block-in-inline

Categories

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

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

testcase
474 bytes, application/xhtml+xml
Details
Fix, sorta
23.00 KB, patch
Details | Diff | Splinter Review
Attached file testcase —
Loading the testcase triggers: ###!!! ASSERTION: special frame error: 'prevParent == nextParent', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 8698
Man. This is a mess. In brief, the markup looks like this: <body> <span> <p></p> <td></td> <span id="s"> <div></div> </span></span> </body> and then the page inserts two spans before the span with id="s". Now the first problem is that ContentInserted looks up the previous sibling and uses that sibling's parent as the insertion parent. In this case the previous sibling is the <td>, so the insertion parent is the tableRowFrame. Really, it would be better if the previous-sibling finding code didn't screw that up... In any case, we insert the first span inside an anonymous table cell inside the tableRowFrame. We then go to insert the second span. Now the previous sibling is a child of the anonymous blockFrame inside the anonymous table cell. This is a block, so we look at the next sibling to figure out whether to insert inside that block or in the (hypothetical) following inline. But the next sibling is a child of a totally different block (in particular, it's a child of the {ib} anonymous block for the outermost span. The assertion is just about the fact that these two blocks are not the same. We really need to fix all three of: our FindPreviousSibling thing (to return things that closer resemble reality), table anonymous frames (to handle FindPreviousSibling working better), and NeedSpecialFrameReframe (to be simpler and actually look at the things that matter, instead of all the stuff it does now).
Wasn't nsCSSFrameConstructor::IsValidSibling plain wrong with its display resolution?
IsValidSibling() doesn't do anything interesting with display:table-cell frames. Which is correct (and in fact, its behavior with table-row-group and company is wrong). If the table row were NOT a pseudoframe, then inserting a child under it should in fact use the table cell as a previous sibling... I think FindPreviousSibling just needs to be a lot smarter about stepping out of pseudoframes.
I filed bug 390425 on some changes that will make the assert go away. This will still render wrong, however...
Note that the markup in comment 1 in fact shows other problems than this bug. See http://lists.w3.org/Archives/Public/www-style/2007Aug/0034.html for the sort of issues it hits. You have to remove all that whitespace to trigger this bug.
Attached patch Fix, sorta — — Splinter Review
This still needs a lot more testing, but does the basic approach make sense? Or should we not really bother with this pending a more complete revamping of the sibling-getting stuff? Note that with this patch we pass the -1 tests enclosed, while the -2 tests show the whitespace issue I posted to www-style about.
Attachment #275076 - Flags: review?(dbaron)
Could you briefly explain what that basic approach is?
Yeah. Basically, the premise of this patch is that we care about the parent we'd have ended up with if our previous sibling were not a mis-nested table-display thingie. So we look up the prev sibling's primary frame, get its placeholder if it's out of flow, then step up the frame tree until we get to a frame whose parent is not a table pseudoframe (which might be not stepping up at all, if the parent is not a table pseudoframe). The upshot is that this markup: <body> <div style="display: table-cell"> </body> gives the nsBlockFrame for the <body> as the parent instead of giving the nsTableRowFrame that is the parent frame of the primary frame of the <div>. The win is that this testcase is fixed. ;) The loss is that if we insert a table cell styled div next to another such div, they won't end up in the same table row. Right now sometimes they will. I will admit that I'm somewhat tempted to not worry about this bug pending a more comprehensive rewrite of table pseudoframes or Find*Sibling or both...
I don't have strong feelings about the question based on what I understand so far; I haven't looked at this code for a while, though. It wouldn't break static cases of table pseudo-frame grouping, right? Just dynamic ones (most of which are already broken, I think, unless we fixed them recently)?
Yeah, this will only break dynamic cases (but including http-packet-boundary type dynamic stuff). And yes, all this stuff is kinda broken no matter what we do, which is why I'm not sure we should worry about fixing this.
Attachment #275076 - Flags: review?(dbaron)
I no longer see the assertion when I load the testcase. But I'm not marking the bug as WFM, since there are a bunch of comments from bernd and bz and dbaron.
> I no longer see the assertion when I load the testcase. Right. See comment 5.
So just as a general update: 1) Bug 386642 made most of the previous-sibling finding changes I wanted to make in this bug, in terms of merging the implementations. It did NOT change the previous sibling found to step out of table pseudos. 2) The correctness bug here got fixed in bug 148810. 3) The whitespace issues got fixed in bug 484448.
Status: NEW → RESOLVED
Closed: 15 years ago
Depends on: 148810
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: