Closed Bug 5126 Opened 25 years ago Closed 25 years ago

text or area frames in tables not handling incremntal reflow correctly

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: buster, Assigned: karnaze)

Details

(Whiteboard: workaround for 4577 now checked in. ignore the attachment.)

Attachments

(1 file)

This is the source file that you need to reproduce the bug. It includes the workaround for bug 4577.
(deleted), text/plain
Details
Closely related to 5110, may even be a duplicate. You won't be able to test this until the tree opens and I can check in nsTableCellFrame.cpp. 1. In the viewer, load the test case and switch on editor mode. 2. Select '1' 3. Ctrl-B to make bold. 4. Dump content and frames, to convince yourself all is well. 5. Ctrl-Z to undo 6. Notice the '1' is missing entirely from the table. This is not just a display problem, because the table resizes as if the '1' really isn't there. 7. Dump content. Content looks correct. 8. Dump frames. Notice the Area frame in the TD has no line frame This is the result after the workaround for 4577 has been checked in. As far as I can tell, that workaround is doing the right thing when retargeting the incremental reflow. See nsTableCellFrame.cpp, look for the comment: // BEGIN bug 4577 --------------------------------------------------------- Test case: <html><body> <table border> <tr><td>1<td>2 </table> </body></html> Here is the debug output for the above steps: // Initial layout webshell=013B4BF0 html 0141B91C refcount=3< head 0141B98C refcount=2< > body 01455AAC refcount=3< Text 0145E53C refcount=3<\n> table border= 0145E67C refcount=7< tbody 0145E9DC refcount=3< tr 0145EA4C refcount=3< td 0145EF20 refcount=4< Text 0145EF8C refcount=3<1> > td 0145F130 refcount=4< Text 0145F19C refcount=3<2\n> > > > > Text 0146748C refcount=3<\n> > > webshell=013B4BF0 Viewport(-1)@01455B60 [view=0144E8D0] {0,0,9180,4470} [state=00000006]< Scroll(-1)@0145D300 [view=0145D3B0] {0,0,9180,4470} [state=00000004]< Root(-1)@0145DE90 [view=0145DF40] {0,0,9180,4470}< Area(html)(-1)@0145E310 {0,0,9180,4470} [state=00000014] [flags=c]< line 0145EC10: count=1 state=clean,block[0xa] {120,120,9060,435} ca={120,120,8940,435} < Block(body)(1)@0145E810 {120,120,8940,435} [state=00000004]< line 01461120: count=1 state=clean,inline[0x8] {0,0,0,0} ca={0,0,0,0} < Text(0)@0145F500[0,0,T][1] {0,0,0,0} [state=00000024]< "\n" > > line 01461180: count=1 state=clean,block[0xa] {0,0,540,435} ca={0,0,540,435} < TableOuter(table)(1)@0145F740 {0,0,540,435} [state=00000004]< Table(table)(1)@0145F7C0 {0,0,540,435} [state=00000004]< TableRowGroup(tbody)(0)@0145FCA0 {15,15,510,405} [state=00000004]< TableRow(tr)(0)@0145FF70 {0,0,480,405} [state=00000004]< TableCell(td)(0)@01460120 {30,30,180,345} [state=00000004]< Area(td)(0)@01460220 {30,30,120,285} [state=00000004]< line 01460770: count=1 state=clean,inline[0x8] {0,0,120,285} ca={0,0,120,285} < Text(0)@01460650[0,0,T][0] {0,0,120,285} [state=00000024]< "1" > > text-runs < 01461D00: count=1 <Text(0)@01460650 > > > > TableCell(td)(1)@01460A50 {240,30,240,345} [state=00000004]< Area(td)(1)@01460B50 {30,30,180,285} [state=00000004]< line 01460F50: count=1 state=clean,inline[0x8] {0,0,180,285} ca={0,0,180,285} < Text(0)@01460E30[0,1,T][0] {0,0,180,285} [state=00000024]< "2\n" > > text-runs < 01461DD0: count=1 <Text(0)@01460E30 > > > > > > > ColGroup-list< TableColGroup(table)(1)@014608F0 {0,0,0,0} [state=00000004]< TableCol(table)(1)@01460CF0 {0,0,0,0} [state=00000004]<> TableCol(table)(1)@014602C0 {0,0,0,0} [state=00000004]<> > > > > line 014676C0: count=1 state=clean,inline[0x8] {0,435,0,0} ca={0,435,0,0} < Text(2)@01467640[0,0,T][1] {0,435,0,0} [state=00000024]< "\n" > > text-runs < 01467980: count=1 <Text(0)@0145F500 > 01467A10: count=1 <Text(2)@01467640 > > > > > > > > // Set text in first cell to Bold Sel. Collapse to 0145EF8C __moz_text 0 Sel. Extend to 0145EF8C __moz_text 1 ---------- nsTextEditor::SetTextProperty B ---------- webshell=013B4BF0 html 0141B91C refcount=3< head 0141B98C refcount=2< > body 01455AAC refcount=3< Text 0145E53C refcount=3<\n> table border= 0145E67C refcount=7< tbody 0145E9DC refcount=3< tr 0145EA4C refcount=3< td 0145EF20 refcount=8< b 0146AE9C refcount=6< Text 0145EF8C refcount=11<1> > > td 0145F130 refcount=4< Text 0145F19C refcount=3<2\n> > > > > Text 0146748C refcount=3<\n> > > webshell=013B4BF0 Viewport(-1)@01455B60 [view=0144E8D0] {0,0,9180,4470} [state=00000016]< Scroll(-1)@0145D300 [view=0145D3B0] {0,0,9180,4470} [state=00000004]< Root(-1)@0145DE90 [view=0145DF40] {0,0,9180,4470}< Area(html)(-1)@0145E310 {0,0,9180,4470} [state=00000014] [flags=c]< line 0145EC10: count=1 state=clean,block[0xa] {120,120,9060,435} ca={120,120,8940,435} < Block(body)(1)@0145E810 {120,120,8940,435} [state=00000004]< line 01461120: count=1 state=clean,inline[0x8] {0,0,0,0} ca={0,0,0,0} < Text(0)@0145F500[0,0,T][1] {0,0,0,0} [state=00000024]< "\n" > > line 01461180: count=1 state=clean,block[0xa] {0,0,540,435} ca={0,0,540,435} < TableOuter(table)(1)@0145F740 {0,0,540,435} [state=00000004]< Table(table)(1)@0145F7C0 {0,0,540,435} [state=00000004]< TableRowGroup(tbody)(0)@0145FCA0 {15,15,510,405} [state=00000004]< TableRow(tr)(0)@0145FF70 {0,0,480,405} [state=00000004]< TableCell(td)(0)@01460120 {30,30,180,345} [state=00000004]< Area(td)(0)@01460220 {30,30,120,285} [state=00000004]< line 01460770: count=1 state=clean,inline[0x8] {0,0,120,285} ca={0,0,120,285} < Inline(b)(0)@0146A220 {0,0,120,285} [state=00000004]< Text(0)@0146BCA0[0,0,T][0] {0,0,120,285} [state=00000024]< "1" > > > text-runs < 0146B690: count=1 <Text(0)@0146BCA0 > > > > TableCell(td)(1)@01460A50 {240,30,240,345} [state=00000004]< Area(td)(1)@01460B50 {30,30,180,285} [state=00000004]< line 01460F50: count=1 state=clean,inline[0x8] {0,0,180,285} ca={0,0,180,285} < Text(0)@01460E30[0,1,T][0] {0,0,180,285} [state=00000034]< "2\n" > > text-runs < 01461DD0: count=1 <Text(0)@01460E30 > > > > > > > ColGroup-list< TableColGroup(table)(1)@014608F0 {0,0,0,0} [state=00000004]< TableCol(table)(1)@01460CF0 {0,0,0,0} [state=00000004]<> TableCol(table)(1)@014602C0 {0,0,0,0} [state=00000004]<> > > > > line 014676C0: count=1 state=clean,inline[0x8] {0,435,0,0} ca={0,435,0,0} < Text(2)@01467640[0,0,T][1] {0,435,0,0} [state=00000024]< "\n" > > text-runs < 0146B370: count=1 <Text(0)@0145F500 > 0146B600: count=1 <Text(2)@01467640 > > > > > > > > Editor::Undo ---------- webshell=013B4BF0 html 0141B91C refcount=3< head 0141B98C refcount=2< > body 01455AAC refcount=3< Text 0145E53C refcount=3<\n> table border= 0145E67C refcount=7< tbody 0145E9DC refcount=3< tr 0145EA4C refcount=3< td 0145EF20 refcount=12< Text 0145EF8C refcount=7<1> > td 0145F130 refcount=4< Text 0145F19C refcount=3<2\n> > > > > Text 0146748C refcount=3<\n> > > webshell=013B4BF0 Viewport(-1)@01455B60 [view=0144E8D0] {0,0,9180,4470} [state=00000016]< Scroll(-1)@0145D300 [view=0145D3B0] {0,0,9180,4470} [state=00000004]< Root(-1)@0145DE90 [view=0145DF40] {0,0,9180,4470}< Area(html)(-1)@0145E310 {0,0,9180,4470} [state=00000014] [flags=c]< line 0145EC10: count=1 state=clean,block[0xa] {120,120,9060,435} ca={120,120,8940,435} < Block(body)(1)@0145E810 {120,120,8940,435} [state=00000004]< line 01461120: count=1 state=clean,inline[0x8] {0,0,0,0} ca={0,0,0,0} < Text(0)@0145F500[0,0,T][1] {0,0,0,0} [state=00000024]< "\n" > > line 01461180: count=1 state=clean,block[0xa] {0,0,435,435} ca={0,0,435,435} < TableOuter(table)(1)@0145F740 {0,0,435,435} [state=00000004]< Table(table)(1)@0145F7C0 {0,0,435,435} [state=00000004]< TableRowGroup(tbody)(0)@0145FCA0 {15,15,405,405} [state=00000004]< TableRow(tr)(0)@0145FF70 {0,0,375,405} [state=00000004]< TableCell(td)(0)@01460120 {30,30,75,345} [state=00000014]< Area(td)(0)@01460220 {15,157,45,30} [state=00000004]< > > TableCell(td)(1)@01460A50 {135,30,240,345} [state=00000004]< Area(td)(1)@01460B50 {30,30,180,285} [state=00000004]< line 01460F50: count=1 state=clean,inline[0x8] {0,0,180,285} ca={0,0,180,285} < Text(0)@01460E30[0,1,T][0] {0,0,180,285} [state=00000034]< "2\n" > > text-runs < 01461DD0: count=1 <Text(0)@01460E30 > > > > > > > ColGroup-list< TableColGroup(table)(1)@014608F0 {0,0,0,0} [state=00000004]< TableCol(table)(1)@01460CF0 {0,0,0,0} [state=00000004]<> TableCol(table)(1)@014602C0 {0,0,0,0} [state=00000004]<> > > > > line 014676C0: count=1 state=clean,inline[0x8] {0,435,0,0} ca={0,435,0,0} < Text(2)@01467640[0,0,T][1] {0,435,0,0} [state=00000024]< "\n" > > text-runs < 013CBC10: count=1 <Text(0)@0145F500 > 013CBCA0: count=1 <Text(2)@01467640 > > > > > > > >
Whiteboard: I've attached the source file with the workaround for bug 4577 because the tree is going to be closed for a long, long time this week.
Whiteboard: I've attached the source file with the workaround for bug 4577 because the tree is going to be closed for a long, long time this week. → workaround for 4577 now checked in. ignore the attachment.
Severity: major → normal
Status: NEW → ASSIGNED
Target Milestone: M5
Severity: normal → major
Just wanted to be clear: the code to reproduce this is checked in now. The attachment is no longer needed.
Assignee: kipp → buster
Status: ASSIGNED → NEW
After discussion with steve, we've determined this bug is case of api bit-rot. THe table cell frame code is using reflow to aim incremental operations at itself down to the area frame it contains. The new and improved (TM) method is to have the table cell frame code override AppendFrames, InsertFrames and RemoveFrame and have those methods delegate to the contained area frame. E.g.: return mAreaFrame->AppendFrames(the same args as passed in)
I don't really agree with that. I think that since it's the frame constuction code that created the table cell's area frame, then the frame construction code should call the area frame's AppendFrames() function I completely agree that tables need to start using the new and improved reflow command handling
Assignee: buster → kipp
I added the code Kipp recommended, it only took about 5 minutes. Except for one minor glitch, it works: the problem originally described goes away, and the table + content all paints correctly. Except that I hit an assertion on line 158 of nsAbsoluteContainingBlock.cpp: NS_ASSERTION(nsnull != nextFrame, "next frame in reflow command is null"); If I continue past this assertion, the table lays out as expected. As far as I can tell, I'm doing the right things in table cell frame. Is this assertion bogus? I can't check this in today because the tree is closed, and I'm going on vacation until Monday. So, in case you want to merge my changes into your tree: -------------- nsTableCellFrame.h ---------------------- /* ----------- nsIFrame overrides ---------- */ /** @see nsIFrame::AppendFrames */ NS_IMETHOD AppendFrames(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aFrameList); /** @see nsIFrame::InsertFrames */ NS_IMETHOD InsertFrames(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aPrevFrame, nsIFrame* aFrameList); /** @see nsIFrame::RemoveFrame */ NS_IMETHOD RemoveFrame(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aOldFrame); ------- nsTableCellFrame.cpp -------------------- NS_IMETHODIMP nsTableCellFrame::AppendFrames(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aFrameList) { nsIFrame *child; nsresult result = FirstChild(nsnull, &child); if (NS_SUCCEEDED(result) && child) { return child->AppendFrames(aPresContext, aPresShell, aListName, aFrameList); } return nsHTMLContainerFrame::AppendFrames(aPresContext, aPresShell, aListName, aFrameList); } /** @see nsIFrame::InsertFrames */ NS_IMETHODIMP nsTableCellFrame::InsertFrames(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aPrevFrame, nsIFrame* aFrameList) { nsIFrame *child; nsresult result = FirstChild(nsnull, &child); if (NS_SUCCEEDED(result) && child) { return child->InsertFrames(aPresContext, aPresShell, aListName, aPrevFrame, aFrameList); } return nsHTMLContainerFrame::InsertFrames(aPresContext, aPresShell, aListName, aPrevFrame, aFrameList); } /** @see nsIFrame::RemoveFrame */ NS_IMETHODIMP nsTableCellFrame::RemoveFrame(nsIPresContext& aPresContext, nsIPresShell& aPresShell, nsIAtom* aListName, nsIFrame* aOldFrame) { nsIFrame *child; nsresult result = FirstChild(nsnull, &child); if (NS_SUCCEEDED(result) && child) { return child->RemoveFrame(aPresContext, aPresShell, aListName, aOldFrame); } return nsHTMLContainerFrame::RemoveFrame(aPresContext, aPresShell, aListName, aOldFrame); }
Assignee: kipp → buster
Steve, I'm assigning the bug back to you, because yes you must be doing something wrong. That assert is there so we catch the case where incremental reflow processing is incorrect. It indicates that the reflow command chain is wrong; the absolute positioning code isn't the target of the reflow command, and there are no more frames in the reflow chain
adding chris to the cc list. He might just go ahead and do the right thing in frame construction code, making the recent conversation between Kipp and I irrelevant.
Target Milestone: M5 → M6
set to M6, assigned to karnaze. Chris, you'll just do the Right Thing in the frame construction code to dispatch the incremental reflow command correctly, right?
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee: buster → karnaze
Status: ASSIGNED → NEW
whoops, didn't actually do the reassignment. Now it's Chris'
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Fixed with latest checkin.
Status: RESOLVED → VERIFIED
Fixed in 5/17 Build.
URL: see test case
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: