191794 - [FIXr]Dynamic border-collapse changes are totally broken

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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...

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: hack hack

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

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 4

[Bernd]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 7

[Bernd]

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.

Comment 8

[karnaze (gone)]

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.

Comment 9

[karnaze (gone)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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 11

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 13

[Asa Dotzler [:asa]]

Comment on attachment 113478 [details] [diff] [review]
hack hack

a=asa (on behalf of drivers) for checkin to 1.3 final.

Comment 14

[Boris Zbarsky [:bzbarsky]]

fixed for 1.3final.

Comment 15

[Madhur Bhatia]

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