99923 - Table.cellPadding does not work

Status: RESOLVED FIXED

Description

[Bernd]

see attached testcase, needless to say that a competitors product renders the
testcase fine, moz 2001-09-10-03 win98

Comment 1

[Bernd]

Created attachment 49511 [details]
testcase

Comment 2

[Bernd]

Created attachment 49512 [details]
testcase without px

Comment 3

[Bernd]

Created attachment 49513 [details]
the same for cellspacing

Comment 4

[Bernd]

Created attachment 49514 [details]
resize the window to see the pcikup of the cellPadding value

Comment 5

[Bernd]

looks like our (html-table) bug

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Seeing this on Linux too.  I think I know why.

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 8

[Bernd]

Boris, I am afraid that you are wrong, the code looks rather symmetric for
cellPadding and cellSpacing, cellSpacing generates a reflow, cellPadding does not.

Comment 9

[Fabian Guisset]

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.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Yeah.... I found that this morning when I went back to look at it.

Serves me right for debugging at night.  :)

Comment 11

[Sivakiran Tummala]

adding amar in cc list and changing the qa contact.

Comment 12

[Bernd]

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.

Comment 13

[Bernd]

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.

Comment 14

[Bernd]

The patch improves the picture on bug 54382, while it is not a complete fix.

Comment 15

[Bernd]

patch to come

Comment 16

[Bernd]

Created attachment 57792 [details] [diff] [review]
patch

Comment 17

[karnaze (gone)]

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

r=karnaze

Comment 18

[Marc Attinasi]

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...

Comment 19

[Bernd]

>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)

Comment 20

[Bernd]

patch checked in, testcase also, adding some status foo, if this should go on
any branch the fix for bug 108340 is also necessary.