Table.cellPadding does not work

RESOLVED FIXED

Status

()

RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

Tracking

({testcase})

Trunk
x86
All
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PATCH CANDIDATE_094)

Attachments

(5 attachments)

(Assignee)

Description

17 years ago
see attached testcase, needless to say that a competitors product renders the
testcase fine, moz 2001-09-10-03 win98
(Assignee)

Comment 1

17 years ago
Created attachment 49511 [details]
testcase
(Assignee)

Updated

17 years ago
Keywords: testcase
(Assignee)

Comment 2

17 years ago
Created attachment 49512 [details]
testcase without px
(Assignee)

Comment 3

17 years ago
Created attachment 49513 [details]
the same for cellspacing
(Assignee)

Comment 4

17 years ago
Created attachment 49514 [details]
resize the window to see the pcikup of the cellPadding value
(Assignee)

Updated

17 years ago
Assignee: jst → karnaze
Component: DOM HTML → HTMLTables
QA Contact: stummala → amar
Summary: Table.cellPadding does not work → Table.cellPadding does generate a correct reflow
(Assignee)

Comment 5

17 years ago
looks like our (html-table) bug

Updated

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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

17 years ago
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.  :)

Comment 11

17 years ago
adding amar in cc list and changing the qa contact.
QA Contact: stummala → amar
(Assignee)

Comment 12

17 years ago
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.
(Assignee)

Comment 13

17 years ago
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.
(Assignee)

Comment 14

17 years ago
The patch improves the picture on bug 54382, while it is not a complete fix.
Blocks: 54382
(Assignee)

Comment 15

17 years ago
patch to come
Assignee: karnaze → bernd.mielke
(Assignee)

Comment 16

17 years ago
Created attachment 57792 [details] [diff] [review]
patch

Comment 17

17 years ago
Comment on attachment 57792 [details] [diff] [review]
patch

r=karnaze
Attachment #57792 - Flags: review+

Comment 18

17 years ago
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+
(Assignee)

Comment 19

17 years ago
>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)
(Assignee)

Comment 20

17 years ago
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
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: PATCH CANDIDATE_094
You need to log in before you can comment on or make changes to this bug.