388700 - "ASSERTION: Failed to find cell frame for cell data: 'Error'" with contenteditable and border-collapse table

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

When the testcase is loaded, I get this assertion for every caret blink:

###!!! ASSERTION: Failed to find cell frame for cell data: 'Error', file /Users/jruderman/trunk/mozilla/layout/tables/nsTableRowGroupFrame.cpp, line 1617

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsTableRowGroupFrame::GetLine just looks weird to me.  First of all, it's possible that there are in fact no cells in the row in question or anywhere above it (as in this testcase).  So the assertion is just bogus.

But even so, is returning a cell originating in another row while returning the number of cells originating in this row as the number of frames in line correct?  I'm not sure what this API is expecting, exactly...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and that nsILineIterator docs don't really say much about what they assume about what they get out here...

Comment 3

[Eli Friedman]

nsILineIterator is supposed to return the first cell intersecting the given row, and the number of frames in the line should be the number of frame retrievable using GetNextSiblingOnLine.  (I won't vouch for the correctness of this design, but that's how it was written.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

At the risk of stating the obvious, nsILineIterator is a mess and needs a good cleanup --- deCOMtamination if nothing else.

Comment 5

[Boris Zbarsky [:bzbarsky]]

nsTableRowGroupFrame::GetNextSiblingOnLine seems to be busted if there are colspans around...

Bernd, you want to give making this stuff sane a shot?

Comment 6

[Bernd]

Hmm I did review a very similiar thing for accessibility, where the function is inside the cellmap where it belongs IMHO.

Comment 7

[Bernd]

I do not understand why this interface is at the rwogroup frame level and not the table frame level. I am not sure that this will work with multiple rowgroups.

Comment 8

[Bernd]

Attachment: design review of the iterator interface

There is not a single function in this interface that is now flawed.
The patch is a comment only change to describe what is going on. It also describes what I believe this interface should do. If I get an OK on this implementing it correctly is a nobrainer.

Comment 9

[Bernd]

Attachment: patch

this is what the function should do IMHO

Comment 10

[Bernd]

The patch does not fix the assert, the assert is bogus if there are no cells in a row, then there is no need for whining.

Comment 11

[Bernd]

Attachment: the right one

Comment 12

[Mike Schroepfer]

Comment on attachment 301834 [details] [diff] [review]
design review of the iterator interface

Clearing approval since the later patch seems to be the one - if this is a mistake please re-ask

Comment 13

[Bernd]

Comment on attachment 301834 [details] [diff] [review]
design review of the iterator interface

this is comment only change that describes the interface.It should be documented anyway. I would like to check this in in case that the real fix for the mistakes will be postponed past 3.0

Comment 14

[Bernd]

Attachment: patch to fix the remaining issues that I see

Comment 15

[Bernd]

Attachment: next rev

Comment 16

[Bernd]

Comment on attachment 302336 [details] [diff] [review]
next rev

I did not invent FindFrameAt its from the lineiterator adapted.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How hard would it be to actually move the iterator off the rowgroup frame and to the table where it seems to belong?

Comment 18

[Bernd]

I think it belongs into the rowgroup as it plays the role of the containing block for the lines (rows). I am afraid of special casing in the callers.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hum. But it returns the number of lines in the table and the line numbers are for the whole table. Should it return the number of lines in the rowgroup and line numbers relative to the rowgroup instead?

Comment 20

[Bernd]

Attachment: patch relative to the rowgroup

Comment 21

[Bernd]

Comment on attachment 304161 [details] [diff] [review]
patch relative to the rowgroup

Robert I was to paranoid with the rowgroup offset. The computation is pretty cheap as we will only hit a rowgroup that has at least a row as a child.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 304161 [details] [diff] [review]
patch relative to the rowgroup

//nsILineIterator methods for nsTableFrame

Fix this comment?

I think you need to change comments here:
http://mxr.mozilla.org/seamonkey/source/layout/tables/nsTableRowGroupFrame.h#227

Comment 23

[Bernd]

Attachment: patch with comments adapted

Comment 24

[Bernd]

fix checked in