363524 - {inc}Inserting/removing cells with rowspan needs to reflow more than one row

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Current trunk build.

STEPS TO REPRODUCE: Notice that <http://lxr.mozilla.org/seamonkey/source/layout/reftests/table-dom/insertCellsExpand2.html>, <http://lxr.mozilla.org/seamonkey/source/layout/reftests/table-dom/deleteCellsRebuild1.html>, <http://lxr.mozilla.org/seamonkey/source/layout/reftests/table-dom/insertCellsRebuild1.html> all fail in a current trunk build.  (You have to load them from local file to test, not from LXR, because LXR munges the JS file.)

I'm pretty sure the problem is a reflow branch regression, since the cellmaps are correct.  Indeed, simply marking more frames dirty during the cellmap manipulations fixes the issues; I'll attach a hacky patch that tries to do that.

The basic problem is that any time the position of an originating entry in the cellmap changes we need to reflow the row that cell is in to make sure that the cell is correctly repositioned.  My patch does this by marking the cell dirty, but really we should have some way of keeping track of dirty rows, similarly to the way we keep track of dirty lines in blocks.

Without something like this, inserting a rowspan 2 cell at the beginning of the first row, say, will only reflow the first row, and not reposition the cell in the second row that should now be in column two, not in column one...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Hacky patch

Comment 2

[Bernd]

Hmm I would say places that previously had the tableFrame->SetNeedStrategyInit(PR_TRUE); for instance at http://lxr.mozilla.org/mozilla1.8/source/layout/tables/nsTableRowFrame.cpp#258 need now to call
nsTableFrame::MarkIntrinsicWidthsDirty()

Comment 3

[Bernd]

The suspicious places are at http://lxr.mozilla.org/mozilla1.8/search?string=SetNeedStrategyInit%28PR_TRUE%29

Comment 4

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

(In reply to comment #2)
> Hmm I would say places that previously had the
> tableFrame->SetNeedStrategyInit(PR_TRUE); for instance at
> http://lxr.mozilla.org/mozilla1.8/source/layout/tables/nsTableRowFrame.cpp#258
> need now to call
> nsTableFrame::MarkIntrinsicWidthsDirty()

They should already, via FrameNeedsReflow.

Comment 5

[Bernd]

Let me rephrase then we need to call FrameNeedsReflow on the table frame so that it calls MarkIntrinsicWidthsDirty at http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3458. I guess a nsIPresShell::eTreeChange argument would be correct as inserting/removing cells will change the position of the cells with respect to the columns.

Comment 6

[Bernd]

arggh I should have read the code. the previous comment is bogus, We need to find a way that reassembles what SetNeedStrategyInit(PR_TRUE) has done, causing a reflow of all rows so that a changed size/position of the cells will be honoured.

Comment 7

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

The obvious way is marking the table frame dirty, unless we care enough about optimizing for this case that we want to add another dirty bit to the table frame that says that all rows should be reflowed, but that the contents of the cells don't need reflow.

Comment 8

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

... which we might, if we don't have another way of distinguishing an append from the content sink from an append to a row that's got another row (with more cells) already after it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't think we want to reflow _all_ rows.  We only want to reflow the "dirty" ones; this should correspond well to the damage area that the cellmap returns, right?

That said, shouldn't we reflow all rows anyway when a column rebalance happens (and in particular when the number of columns changes)?  We don't even seem to be doing that right now... at least based on our behavior on the testcases.

Comment 10

[Bernd]

"That said, shouldn't we reflow all rows anyway...."

Thats what I meant with comment 6. And yes we definitely should

Usually the content sink gives us entire rows, so the situation at nsTableRowFrame Append/Insert/Delete is special.

"this should correspond well to the damage area that the cellmap returns,
right?"

probably yes, but be aware that the damage area was before only used for border collapse - which is not recognized for its bug freeness

Comment 11

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

(In reply to comment #9)
> That said, shouldn't we reflow all rows anyway when a column rebalance happens
> (and in particular when the number of columns changes)?  We don't even seem to
> be doing that right now... at least based on our behavior on the testcases.

The code that's supposed to do that is the call to DidResizeColumns from the layout strategy, then 
nsTableFrame::InitChildReflowState setting mHResize for the row group, and nsTableRowGroupFrame::ReflowChildren explicitly propagating any mHResize to the row.

Comment 12

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

Attachment: patch

This fixes the failed reftests.  It reflows all the rows/cells in these cases, but shouldn't cause us to descend into the contents of the cells, so it shouldn't be too expensive.

(I don't want to assume that the content sink won't notify in the middle of rows -- I actually think we should consider changing that.)

Comment 13

[Bernd]

Comment on attachment 248422 [details] [diff] [review]
patch

My comment about the content sink should be: My impression is that the content sink tries to deliver entire rows, happily it succeeds in this very often, but not always.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 248422 [details] [diff] [review]
patch

sr=bzbarsky.

Comment 15

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

Checked in to trunk.

Comment 16

[Jeff Walden [:Waldo]]

Already tested by the reftests that this bug fixed.