Closed Bug 363524 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: dbaron)

References

Details

(Keywords: regression)

Attachments

(2 files)

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...
Attached patch Hacky patchSplinter Review
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()
(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.
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.
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.
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.
... 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.
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.
Flags: blocking1.9?
"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
(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.
Attached patch patchSplinter Review
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.)
Attachment #248422 - Flags: superreview?(bzbarsky)
Attachment #248422 - Flags: review?(bernd_mozilla)
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.
Attachment #248422 - Flags: review?(bernd_mozilla) → review+
Comment on attachment 248422 [details] [diff] [review]
patch

sr=bzbarsky.
Attachment #248422 - Flags: superreview?(bzbarsky) → superreview+
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
Already tested by the reftests that this bug fixed.
Flags: in-testsuite-
Assignee: nobody → dbaron
You need to log in before you can comment on or make changes to this bug.