284852 - Number of columns minus a row index?

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Mats Palmgren (inactive)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=3.91&root=/cvsroot&mark=1531,1591#1531

damageHeight = (aRowSpanIsZero) ? aMap.GetColCount() - aRowIndex : aRowSpan;

The above looks odd to me, "aMap.GetRowCount() - aRowIndex" seems more logical.

Comment 1

[Bernd]

IIRC
aMap.GetRowCount will return all rows in the cellmap. Rowspans however stop at
rowgroup boundaries. So it might be necessary to first retrieve the nsCellMap
(this is for a row group) for the current row and then call GetRowCount on it
rather than calling it directly on a nsTableCellMap.

Comment 2

[Bernd]

To see the impact of this one needs 
1. a strict doctype in quirks rowspan=0 is parsed as rowspan=1
2. a table with multiple rowgroups / or to see the original bug a table where
the number of rows after a cell with rowspan=0 exceeds the number of cols.
3. and a border collapse table
4. trigger this code 

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Here are three different examples that leads to an assertion.

A bit of debug output on the first example at the top:
Clicking 5 times on the button...

Document file:///home/mats/work/bug284852/284852.html loaded successfully
ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2
  aRowSpanIsZero aMap.GetColCount()=1 endColIndex=0
  SetDamageArea x=0 y=3 width=1 height=-2
ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2
  aRowSpanIsZero aMap.GetColCount()=2 endColIndex=0
  SetDamageArea x=0 y=3 width=1 height=-1
ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2
  aRowSpanIsZero aMap.GetColCount()=3 endColIndex=0
  SetDamageArea x=0 y=3 width=1 height=0
ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2
  aRowSpanIsZero aMap.GetColCount()=4 endColIndex=0
  SetDamageArea x=0 y=3 width=1 height=1
ExpandWithCells aRowSpanIsZero=1 aColIndex=0 aRowSpan=2
  aRowSpanIsZero aMap.GetColCount()=5 endColIndex=0
  SetDamageArea x=0 y=3 width=1 height=2
###!!! ASSERTION: invalid BC damage area: 'PR_FALSE', file nsTableFrame.cpp,
line 4569
Break: at file nsTableFrame.cpp, line 4569

The damage area setup is wrong... the root of the problem IMO is the 'aRowSpan'

value (2) which is the result of:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsCellMap.cpp&rev=3.91&root=/cvsroot&mark=1752,1759,1760#1750


The problem also occurs for aRowSpanIsZero is false though -
see the second example.

The third is also a zero rowspan case, although it triggers the assertion
at a different place, nsTableFrame line 982:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.610&root=/cvsroot&mark=967,982#966

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 176407 [details]
Testcase #1

Bernd, does this cover all the cases you were thinking of in comment 2?

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes the three cases in Testcase #1

Comment 6

[Bernd]

Comment on attachment 176408 [details] [diff] [review]
Patch rev. 1

while I am perfectly happy with the first part of the patch

PR_MAX(1.... scares me. I would like to be certain that there is no elsewhere
as shown in the comment above.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176408 [details] [diff] [review]
Patch rev. 1

sr=bzbarsky on the first hunk.	I'm also a little leery of the second one...

Comment 8

[Bernd]

Comment on attachment 176408 [details] [diff] [review]
Patch rev. 1

r+ on the first hunk, this can be checked in. Please file a separate bug for
the second hunk as it changes substancially the zero rowspan treatment.

Comment 9

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/641db8b29158

Filed bug 460637 to followup on the second hunk.

-> FIXED

Comment 10

[Bernd]

Dude slight delay since r+? ;-)