Created attachment 8745 [details]
Simple testcase

I extracted the testcase from bugzilla, to add it here.

Still here with my konqueror 3.4.1 (Debian unstable).

Cannot see any markable differences between konqueror 3.5.2/4 and firefox 1.5/opera 9. Could you verify again with 3.5.x please?

Created attachment 17605 [details]
Display with konqueror 3.5.4

This a screenshot of the render in my konqui 3.5.4 (deb packaging). The "blank"
to the right of the cells should not be displayed.

The JS remove the thead, then create a new one with only one tr, which has only
one th.

And yes, the bug exists with my firefox too (1.5.0.6, deb packaging), i opened
a bug on bugzilla.mozilla.org... still unresolved for the moment.

the bug is probably _now_ fixed in mozilla, see comment # 6. confirming the bug for konqueror, as of kde 3.5.4 the bug is up and running.
https://bugzilla.mozilla.org/show_bug.cgi?id=242045

I confirm the bug is still up with my konqueror 3.5.5 (unstable deb packaging).

Here is my analysis, if someone can tell me how to fix that, i'll code it.

This appears in the debug output _after_ having removed the tHead (and all its rows and cells):
---
AutoTableLayout::layout()
    tableWidth=45,  nEffCols=4
    effcol 0 is of type 0 value 0, minWidth=40, maxWidth=40
        effective: type 0 value 0, minWidth=40, maxWidth=40
    effcol 1 is of type 0 value 0, minWidth=1, maxWidth=1
        effective: type 0 value 0, minWidth=1, maxWidth=1
    effcol 2 is of type 0 value 0, minWidth=1, maxWidth=1
        effective: type 0 value 0, minWidth=1, maxWidth=1
    effcol 3 is of type 0 value 0, minWidth=1, maxWidth=1
        effective: type 0 value 0, minWidth=1, maxWidth=1
---

That means that the size RenderTable::columns member is still '4' after the removal (whereas it should be '1' after the new insertion). As far as i can tell, the size of 'columns' always increases (when needed) and never decreases in the code.

Should we recompute 'columns' when we call 'removeChildNode' by looking through all tr's? Could be very inefficient...

So the conclusion is... i need a table layout guru ;)

PS: i didn't check the FixedLayout behavior.

Le Dimanche 13 Mai 2007 21:01, Vincent Ricard a écrit :
> Hi,
>
> I posted a new comment about #93791 :
> http://bugs.kde.org/show_bug.cgi?id=93791#c7
>

Hi,
your analysis looks completely correct indeed.

> It seems 'RenderTable::columns' can be out of date after a table
> section removal (as with the testcase of the bug); and i wondered if
> someone can help me to fix that (just give me the process, i'll code
> it). Should we recompute 'columns' in the removeChildNode call 
> (at 
> which level? section? row? cell?) by looking through all tr's of all
> sections? by using the grid of each section?

yes, the column count should be recalculated after any node removal, but not right away as that would be costly and complicated. You would better use the delayed recalcSections/recalcCells mechanism. 

recalcSections is triggered whenever setNeedSectionRecalc or setNeedCellRecalc is set, so it's the best place I think. 

> by looking through all tr's of all
> sections? by using the grid of each section?

at the end of recalcSections, even though the grid will be sized based on the previous column count (which doesn't matter), it should be correctly populated, so simply counting all places where cellAt(x, y) is non-zero for each row and gathering the max result for all sections should be fine.

>
> Any comments are welcomed (even to say my analysis is wrong :) )
>
> Cheers,
> --
> Vincent Ricard

SVN commit 722231 by ggarand:

RenderTable::columns array can sometimes be out of sync after a table section removal.

fix transposed from a patch by Alexey Proskuryakov <ap@nypop.com> 


 M  +29 -1     render_table.cpp  
 M  +2 -0      render_table.h  


--- trunk/KDE/kdelibs/khtml/rendering/render_table.cpp #722230:722231
@@ -738,6 +738,20 @@
        }
        child = child->nextSibling();
     }
+
+    int maxCols = 0;
+    for (RenderObject *child = firstChild(); child; child = child->nextSibling()) {
+        if (child->isTableSection()) {
+            RenderTableSection *section = static_cast<RenderTableSection *>(child);
+            int sectionCols = section->numColumns();
+            if (sectionCols > maxCols)
+                maxCols = sectionCols;
+        }
+    }
+    
+    columns.resize(maxCols);
+    columnPos.resize(maxCols+1);
+    
     needSectionRecalc = false;
     setNeedsLayout(true);
 }
@@ -1774,6 +1788,20 @@
     }
 }
 
+int RenderTableSection::numColumns() const
+{
+    int result = 0;
+    
+    for (int r = 0; r < numRows(); ++r) {
+        for (int c = result; c < table()->numEffCols(); ++c) {
+            if (cellAt(r, c))
+                result = c;
+        }
+    }
+    
+    return result + 1;
+}
+ 
 void RenderTableSection::recalcCells()
 {
     cCol = 0;
@@ -2902,7 +2930,7 @@
 
 void RenderTableCell::paintBackgroundsBehindCell(PaintInfo& pI, int _tx, int _ty, RenderObject* bgObj)
 {
-    if (!bgObj)
+    if (!bgObj || style()->visibility() != VISIBLE)
         return;
 
     RenderTable* tableElt = table();
--- trunk/KDE/kdelibs/khtml/rendering/render_table.h #722230:722231
@@ -271,6 +271,8 @@
     QVector<RowStruct> grid;
     QVector<int> rowPos;
 
+    int numColumns() const;
+
     signed short cRow;
     ushort cCol;
     bool needCellRecalc;