border-collapse: collapse creates special cellframes. So border-collapse changes should trigger a reframe. 932 nsChangeHint nsStyleTableBorder::CalcDifference(const nsStyleTableBorder& aOther) const 933 { 934 if ((mBorderCollapse == aOther.mBorderCollapse) && 935 (mCaptionSide == aOther.mCaptionSide) && 936 (mBorderSpacingX == aOther.mBorderSpacingX) && 937 (mBorderSpacingY == aOther.mBorderSpacingY)) { 938 if (mEmptyCells == aOther.mEmptyCells) 939 return NS_STYLE_HINT_NONE; 940 return NS_STYLE_HINT_VISUAL; 941 } 942 else 943 return NS_STYLE_HINT_REFLOW; 944 } Oops. I bet fixing this will fix a large majority of the outstanding bugs in dynamic border-collapse changes...

Targeting 1.3final, but the only way this could get into 1.3 is if we do a hell of a lot of testing.....

The real fix, of course, is to make it need a _reflow_, not a reframe. In the meantime, this blocks bug 171830

Comment on attachment 113478 [details] [diff] [review] hack hack How about a comment saying this is a hack and isn't the way it should work?

Comment on attachment 113478 [details] [diff] [review] hack hack As I explained you on IRC I think a reframe is the right thing to do, there is no need that separated border cells have to pay the performance price of having a border collapse sibling. I think the dynamic switch between those models is so rare that any optimization should go towards making the nondynamic usual case faster. So I would not like to see any comments that indicate this as a hack without showing the performance penalty (speed and memory) that a change to reflow would cause.

David? Thoughts? Bernd, note that dynamic switches may get more common as A) Alternate stylesheets are used more B) We stop blocking the parser for stylesheet loads I've not looked at the code in much detail yet, so I'm not sure what the performance cost of a nsBCTableCellFrame is. What is it exactly? (As for this patch, I'm tempted to put in a long comment summarizing the issues so that people don't blindly change that code... but I would like to know what the issues are first.)

Fine with me if this is permanent. Comment 4 seems reasonable.

As you might see from http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableCellFrame.h#454 bc table cells need another 4 bytes and as a minimum the cellmap increases also by 4 bytes per table cell. I believe that Chris's design decision was based on the following assumptions if the large majority of table based layout out in the real world (I would guess > 90% are still table based) use primarly the separate border model (At least from what I have seen I would believe the number should be around 99%) and all those pages should not regress in memory use (if you look at the checkin time it was the time when footprint optimization was en vogue) and in execution speed as a consequence of the increased memory use. I can't specify how large are those speed penalties, probably the only way to specify it is to rewrite it and to compare then. Those considerations may be not very realistic but I really recommend you to talk with Chris while he is still available at his nscp address to try to understand what has driven his decisions. btw over at the original bc bug there is a list with testcases, one of them includes a switch between bc and separate mode and I now understand why Chris did never got that thing running.

The cost of avoiding the reframe will be 4 bytes per cell frame, 4 bytes per cell map entry, changing code to call nsTableFrame::SetBorderCollapse() appropriately, and some testing.

But going to collapsed (from separate) the first time would require a reframe.

Makes sense (unless we're willing to always pay the cost of those extra 4+4 bytes, which I think we do want to avoid for now if possible). Thank you for the info, Chris, Bernd.

Comment on attachment 113478 [details] [diff] [review] hack hack This makes things like :hover correctly toggle border-collapse if the page styles hovered pages differently from unhovered... It's a very safe change that should have no effect on pages that don't do dynamic changes to border-collapse. Could this please be approved for 1.3?

Comment on attachment 113478 [details] [diff] [review] hack hack a=asa (on behalf of drivers) for checkin to 1.3 final.

fixed for 1.3final.

verified with testcase attachment 113716 [details] on winXP and linux platforms with trunk build dated 03-12-2003