Closed Bug 99923 Opened 24 years ago Closed 24 years ago

Table.cellPadding does not work

Categories

(Core :: Layout: Tables, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

References

Details

(Keywords: testcase, Whiteboard: PATCH CANDIDATE_094)

Attachments

(5 files)

see attached testcase, needless to say that a competitors product renders the testcase fine, moz 2001-09-10-03 win98
Attached file testcase
Keywords: testcase
Attached file testcase without px
Assignee: jst → karnaze
Component: DOM HTML → HTMLTables
QA Contact: stummala → amar
Summary: Table.cellPadding does not work → Table.cellPadding does generate a correct reflow
looks like our (html-table) bug
QA Contact: amar → stummala
Summary: Table.cellPadding does generate a correct reflow → Table.cellPadding does not work
Seeing this on Linux too. I think I know why.
OS: Windows 98 → All
OK. Here's what's up. nsHTMLTableElement.cpp uses NS_IMPL_STRING_ATTR to implement the GetCellPadding and SetCellPadding functions. This lives in nsGenericHTMLElement.h and just updates the attribute. It does not send the document a change notification, so there is no reflow. I tried hacking that macro to include a call to AttributeChanged() but that did not seem to help. I could well be passing the wrong args to AttributeChanged; I'll play with it more.
Boris, I am afraid that you are wrong, the code looks rather symmetric for cellPadding and cellSpacing, cellSpacing generates a reflow, cellPadding does not.
Boris, the table code should be notified when one of its attribute changes, and I think that's happening properly. Like Bernd pointed out, the DOM code is the same for cellspacing and that one works fine.
Yeah.... I found that this morning when I went back to look at it. Serves me right for debugging at night. :)
adding amar in cc list and changing the qa contact.
QA Contact: stummala → amar
at http://lxr.mozilla.org/seamonkey/source/layout/html/table/src/nsTableRowFrame.cpp#843 we skip the necessary reflow for the cell the following patch fixes the problem but I have no clue about the performance impact Index: nsTableRowFrame.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/table/src/nsTableRowFrame.cpp,v retrieving revision 3.274 diff -u -w -r3.274 nsTableRowFrame.cpp --- nsTableRowFrame.cpp 2001/08/06 14:04:58 3.274 +++ nsTableRowFrame.cpp 2001/09/21 19:04:51 @@ -838,6 +838,7 @@ if ((availWidth != cellFrame->GetPriorAvailWidth()) || (cellDesiredSize.width > cellFrame->GetPriorAvailWidth()) || (eReflowReason_StyleChange == aReflowState.reason) || + (eReflowReason_Resize == aReflowState.reason) || isPaginated) { // Reflow the cell to fit the available width, height nsSize kidAvailSize(availWidth, aReflowState.availableHeight); turning on the DEBUG_TABLE_REFLOW in makefile.win in layout\html\table\src give the following : Tbl 00BF94D0 r=0 a=8940,UC c=0,0 cnt=0 Tbl 00BF96DC r=0 a=8940,UC c=4440,UC cnt=1 RowG 00BF989C r=0 a=UC,UC c=UC,UC cnt=2 Row 00BF9A1C r=0 a=UC,UC c=UC,UC cnt=3 Cell 00BF9BE4 r=0 a=UC,UC c=UC,UC cnt=4 Block 00BF9C40 r=0 a=UC,UC c=UC,UC cnt=5 Block 00BF9C40 d=300,285 me=300 Cell 00BF9BE4 d=360,345 me=360 Row 00BF9A1C d=UC,345 RowG 00BF989C d=UC,345 Initialized min=450 des=450 pref=450 Balanced min=450 des=4470 pref=450 cols=4380 RowG 00BF989C r=2 a=4440,UC c=4440,UC cnt=6 Row 00BF9A1C r=2 a=4440,UC c=4440,UC cnt=7 Cell 00BF9BE4 r=2 a=4380,UC c=4320,UC cnt=8 Block 00BF9C40 r=2 a=4320,UC c=4320,UC cnt=9 Block 00BF9C40 d=4320,285 Cell 00BF9BE4 d=4380,345 Row 00BF9A1C d=4440,345 RowG 00BF989C d=4440,345 Tbl 00BF96DC d=4470,435 Tbl 00BF94D0 d=4470,435 Tbl 00BF94D0 r=1 a=8940,UC c=0,0 cnt=10 Tbl 00BF96DC r=1 a=8940,UC c=4440,UC cnt=11 Initialized min=450 des=450 pref=450 Balanced min=450 des=4470 pref=450 cols=4380 RowG 00BF989C r=2 a=4440,UC c=4440,UC cnt=12 Row 00BF9A1C r=2 a=4440,UC c=4440,UC cnt=13 Row 00BF9A1C d=4440,345 RowG 00BF989C d=4440,345 Tbl 00BF96DC d=4470,435 Tbl 00BF94D0 d=4470,435 Note the missing cell and block reflow in the last reflow.
Chris, I think I nailed down the issue, as my workload is now high, if you would like to see this in the tree you have to take this bug. Otherwise it has to wait for another 2 weeks.
The patch improves the picture on bug 54382, while it is not a complete fix.
Blocks: 54382
patch to come
Assignee: karnaze → bernd.mielke
Attached patch patchSplinter Review
Comment on attachment 57792 [details] [diff] [review] patch r=karnaze
Attachment #57792 - Flags: review+
Comment on attachment 57792 [details] [diff] [review] patch sr=attinasi | I have to say, it is ever more confusing that a StyleChange reflow and an incremental reflow with reflowCommand type StyleChanged are different...
Attachment #57792 - Flags: superreview+
>I have to say, it is ever more confusing that a StyleChange >reflow and an incremental reflow with reflowCommand type StyleChanged are >different... the difference is, who is the target of the style change reflow, all frames higher in the hierarchy pass the incr_reflow with type style change trough, and the target accepts the reflow and notifies its childrens of the style change reflow. Clear as mud, for me at least ( it took me two weeks to come this enlighted point)
patch checked in, testcase also, adding some status foo, if this should go on any branch the fix for bug 108340 is also necessary.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Whiteboard: PATCH CANDIDATE_094
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: