Loading the testcase triggers: frame: TableCol(col)(0) (0x2666d84) style: 0x2784110 {} ###!!! ASSERTION: Wrong parent style context: 'Error', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 818 Wrong parent style context: style: 0x2783758 {} should be using: style: 0x2783dec {} About half of the time, it also triggers: ###!!! ASSERTION: Style contexts are not in the same style context tree: 'top1 == top2', file /Users/jruderman/trunk/mozilla/layout/base/nsFrameManager.cpp, line 781

When we removed the col, we left the anonymous cols hanging around.... This patch makes sure to remove them. I'm not sure about replacing them with anonymous "cell" cols, but I think there are cases when we do need that, and detecting whether we do from here is hard. :( I _think_ having extra "cell" cols is ok, right?

Comment on attachment 284205 [details] [diff] [review] Fix removing a col without calling nsTableFrame::RemoveCol is wrong. Having the cols not in sync with the cellmap was tolerated for too long time.

Uh. This code IS calling RemoveCol for everyu single column it removes. It's just that RemoveCol replaces the col being removed with a eColAnonymousCell col. So if I have a <col span="3"> but only one cell, I'll end up with three eColAnonymousCell cols, not just one. That's the only odd thing about the behavior this patch gives. If that behavior is not acceptable, then we have to somehow figure out the "right" number of eColAnonymousCell cols to create. Any ideas how? Perhaps RemoveCol should actually be RemoveCols so we can remove the whole span at once? But are eColAnonymousCell colframes ever removed explicitly?

Comment on attachment 284205 [details] [diff] [review] Fix Resetting review request per comment 3. Bernd, if it's really very important that the set of colframes after removal of a <col> exactly match what we would have had if the <col> were not there at all, I can do that. But it seems like we deal with extra cols anyway, because of spans...

Comment on attachment 284205 [details] [diff] [review] Fix Oh I did miss that this is a recursion call

Comment on attachment 284205 [details] [diff] [review] Fix Requesting 1.9 approval. I think this is reasonably low-risk... it's definitely less risky than leaving the code as-is.

Fixed.

Should get in a test for this...

Testcase checked in as a crashtest. (bz, did you want something more?)

I added reftests for this using both background and visibility:collapse. Bernd, I put them under bugs/, but if you want me to put things like this under table-dom, let me know?