Closed Bug 404666 Opened 14 years ago Closed 14 years ago

[FIX]"ASSERTION: How did that happen??" removing <col>

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file testcase
Loading the testcase triggers:

###!!! ASSERTION: How did that happen??: 'col->GetStyleContext() == colFrame->GetStyleContext() && col->GetContent() == colFrame->GetContent()', file /Users/jruderman/trunk/mozilla/layout/tables/nsTableColGroupFrame.cpp, line 340

(Previous bugs triggering this assertion: bug 404301, bug 403249.)
Attached patch Proposed fixSplinter Review
Really, we need to restyle the spanned cols if the primary col is restyled.  I thought about it, and decided to just make the spanned cols continuations of the primary col, and back out the changes originally made for bug 404301 (since the frame constructor knows to insert after the last continuation).  I think this is a reasonable use of non-fluid continuations (several frames, all with identical content and style context pointers, etc), but roc, please correct me if I'm wrong.
Attachment #289607 - Flags: superreview?(roc)
Attachment #289607 - Flags: review?(bernd_mozilla)
Assignee: nobody → bzbarsky
OS: Mac OS X → All
Hardware: PC → All
Summary: "ASSERTION: How did that happen??" removing <col> → [FIX]"ASSERTION: How did that happen??" removing <col>
Will this continuation mechanism interact with the next in flow mechanism for pagination?
In theory yes, but in practice we don't create next-in-flows for column frames, do we?  At least not that I could find....  In particular, they didn't use to be splittable frames.

And I'm hoping that returning NOT_SPLITTABLE will mean that no one _tries_ to create continuations for them.  Telling me whether that's correct is where roc comes in.  ;)
AFAIK, columns belong to the first in flow table. 

The plan is however, one fine day, when tables handle pagination correctly on incr. reflow to make the tables split inside column layouts. Any solution should have this goal in mind. I am not sure that this applies here.
I suspect we'll rewrite our pagination model before that happens...  In any case, it's possible to have both non-flow continuations and in-flows; inline frames do just that.

The other option would be to teach all sorts of code outside tables about these spanned columns...
Boris why do we need this instead of what you did before?
The reason for this bug is that when we restyle the <col> nsFrameManager::ReResolveStyleContext doesn't know about the anonymous colframes, so doesn't change their styles.  Compare the rendering of the reference and the test in the attached patch on current trunk to see the sort of issues this can cause.  Basically, it leaves stale style contexts on the anonymous colframes.

ReResolveStyleContext does know that all continuations need to be resolved, so setting up the anonymous colframes as continuations solves that problem.  It also makes the frame constructor insert things at the right place, which makes the patch I originally checked in for bug 404301 unnecessary.
Comment on attachment 289607 [details] [diff] [review]
Proposed fix

It looks to me that the cols can be good mapped to the continuations.
I thought that the default for a frame is to be not splittable, at least that is what nsIFrame.h says.

I hope that http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10529 
will at least create the noise when we are violating the basic assumption that somebody creates a continuing frame for a nonsplittable frame.

This will however not be catched by running the reftests as they do not pay attention to asserts (I did forget to set XPCOM_DEBUG_BREAK yesterday as I did not run the tests for a long time....)

There is however  a question if this patch is complete:

If a colgroup has a span attribute and creates cols below it, they will have
the type  eColAnonymousColGroup. Won't they exhibit the same problem? Why they should not be continuations, they share the style too.

Now comes my wishlist:

If this goes in, please update http://wiki.mozilla.org/Gecko:Continuation_Model  to mention the col frames.

If you change columns and colgroups please add also tests for visibility collapse like the attached one to the reftests.
Attachment #289607 - Flags: review?(bernd_mozilla) → review+
> Why they should not be continuations, they share the style too.

That's true, but they can't be restyled individually, right?  It might make sense to do that too.  And I think nsTableFrame::CreateAnonymousColFrames should assert that aColType is not eColAnonymousCol (which it never is in our current code).  Does that seem reasonable?

> If this goes in, please update http://wiki.mozilla.org/Gecko:Continuation_Model

I will try, yes.

And I'll check in the testcase, yes.  Did you want me to check in other tests along those lines too, and if so what should they test?
>Did you want me to check in other tests

The collapsing of cells is pretty subtle way to test if a reflow happens.
OK.  I'll see what I can do.
Comment on attachment 289607 [details] [diff] [review]
Proposed fix

GetSplittableType doesn't matter much, only block frames call it on their children. The main thing is that you're returning NS_FRAME_COMPLETE from Reflow always, that will prevent splitting.
Attachment #289607 - Flags: superreview?(roc) → superreview+
Comment on attachment 289607 [details] [diff] [review]
Proposed fix

Requesting approval.  This is a quite safe fix that makes sure we don't end up with incorrect styles on columns.
Attachment #289607 - Flags: approval1.9?
Attachment #289607 - Flags: approval1.9? → approval1.9+
Attachment #291168 - Flags: superreview?(roc)
Attachment #291168 - Flags: review?(bernd_mozilla)
Checked in, including both testcases.  Updated the wiki.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #291168 - Flags: review?(bernd_mozilla) → review+
Attachment #291168 - Flags: superreview?(roc) → superreview+
Comment on attachment 291168 [details] [diff] [review]
Followup cleanup per comment 10

Remove dead code and add some assertions.
Attachment #291168 - Flags: approval1.9?
Attachment #291168 - Flags: approval1.9? → approval1.9+
Checked in the followup.
Depends on: 422249
No longer depends on: 422249
You need to log in before you can comment on or make changes to this bug.