Closed Bug 284852 Opened 17 years ago Closed 13 years ago

Number of columns minus a row index?

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: mats, Assigned: mats)

Details

(Keywords: testcase)

Attachments

(2 files)

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.
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.
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 
Attached file 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 on attachment 176407 [details]
Testcase #1

Bernd, does this cover all the cases you were thinking of in comment 2?
Attached patch Patch rev. 1Splinter Review
This fixes the three cases in Testcase #1
Attachment #176408 - Flags: superreview?(bzbarsky)
Attachment #176408 - Flags: review?(bernd_mozilla)
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 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...
Attachment #176408 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #176408 - Flags: review?(bernd_mozilla) → review+
Keywords: testcase
http://hg.mozilla.org/mozilla-central/rev/641db8b29158

Filed bug 460637 to followup on the second hunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Dude slight delay since r+? ;-)
You need to log in before you can comment on or make changes to this bug.