370353 - Dynamically setting 'visibility: collapse' to a <col> no longer works

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Gérard Talbot]

This bug comes from bug 76497 comment #c19.
The collapse examples found at
http://www.mozilla.org/newlayout/samples/test4.html
work as expected. 

Test #5 (<col> visibility with border-collapse: separate) and test #7
(<colgroup> visibility with border-collapse: separate) taken from the provided URL ( attachment 147933 [details] ) were working before (say, ~=6 weeks; after reflow branch; at the time when table caption became as wide as document, not as wide as table). Now, they do not.

Same thing with the demo page

http://www.gtalbot.org/DHTMLSection/TableRowColumnCollapse.html

Bug happens in Seamonkey 1.5a rv:1.9a3pre build 2007021310 under XP Pro SP2 here.

Reduced, minimized testcase coming up.

Comment 1

[Gérard Talbot]

Attachment: Reduced testcase

Load the testcase. Press the button.

Expected results: 2nd <col> should collapse

Actual results in Seamonkey 1.5a rv:1.9a3pre build 2007021310 under XP Pro SP2: 2nd  <col> does not collapse

Note: Firefox 2.0.0.1 rv:1.8.1.1 build 20061204 renders the expected results.

Comment 3

[Adam Guthrie]

Attachment: reftest version of testcase

If someone could please check this in, at the latest when this bug gets fixed, that would be great.

Comment 4

[Bernd]

>  Firefox 2.0.0.1 rv:1.8.1.1 build 20061204 renders the expected results.

The previously working marking mechanism is probably not compatible with the code after the reflow branch and gets not executed. The code in question is at http://lxr.mozilla.org/seamonkey/source/layout/tables/nsTableColFrame.cpp#107. One can verify that the collapsing itself works by selecting first a column and then a row to collapse at http://www.gtalbot.org/DHTMLSection/TableRowColumnCollapse.html

Comment 5

[cmtalbert]

Comment on attachment 270099 [details]
reftest version of testcase

Sounds like we should go ahead and check in the test for this into reftest.  Bernd, are you the right person to review that?

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase (reduced further)

The "border-collapse: separate" doesn't seem to be necessary to trigger the bug.

Here's a much more reduced testcase.

Comment 7

[Daniel Holbert [:dholbert]]

The problem is that the table.mBits.mHaveReflowedColGroups flag is set to 1 on the table's first load, and then it's never cleared after that.

As a result, nsTableFrame::ReflowColGroups returns early, which prevents us from hitting the code mentioned in comment 4.

So, it looks like we need to do one of the following:
 a) clear table.mBits.mHaveReflowedColGroups at some point
 b) call nsTableColFrame::Reflow at some other point.  (i.e. some place triggered by the style change, outside of the table-reflow process)

Comment 8

[Daniel Holbert [:dholbert]]

D'oh, somehow I changed OS back to WinXP... I think maybe I posted my last comment on an old version of the page.

Setting to "All" again.  Sorry for the bugspam.

Comment 9

[Bernd]

Attachment: patch with whitespace issues

Comment 10

[Bernd]

probably it would even better to  do it conditional if NeedToCollapse() is true

Comment 11

[Bernd]

forget the last proposal its wrong

Comment 12

[Daniel Holbert [:dholbert]]

RE attachment 275639 [details] [diff] [review] -- talked to dbaron, and he said this approach is probably fine.  While it means we'll reflow the col groups more than we need to, it's pretty cheap to reflow them.

One possible addition / optimization would be to check if a col group is dirty before we reflow it, as we do with (row groups) in nsTableFrame::ReflowChildren

Gonna try that out and see how that works.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: patch v2: checking colgroups' dirtiness

Clear "mHaveReflowedColGroups" on each table reflow, but also check for col-group dirtiness, so that we only reflow a col-group if its subtree is dirty.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: patch v2 (no-whitespace-changes version)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: reftest version of testcase (as patch)

Here's a patch version of ispiked's zipped testcase (which was attachment 270099 [details])

Comment 16

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

Comment on attachment 275679 [details] [diff] [review]
patch v2 (no-whitespace-changes version)

sr+a1.9=dbaron.

Make sure that if somebody checks this in for you, the check in the version with the whitespace changes :-)

Comment 17

[Colin Barrett [:cbarrett]]

Checking in layout/tables/nsTableFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.700; previous revision: 3.699
done

Comment 18

[Colin Barrett [:cbarrett]]

Checking in layout/tables/nsTableFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.701; previous revision: 3.700
done

Landing the patch with the whitespace changes like I was supposed to. :)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Checked in the reftests too.

Comment 20

[Colin Barrett [:cbarrett]]

We really need to get dholbert his cvs account ;)

Comment 21

[Gérard Talbot]

VERIFIED with Seamonkey 2.0a1pre rv:1.9a8pre build 2007082102 under XP Pro.
Thank you all!