Bug 13081 – Empty table cells not handled correctly

• Component: Tables
• Status: RESOLVED FIXED

[Antti Koivisto]

Empty table cells should be ignored in table layout calculations in same case. See the attached test case, compare with firefox.

[Antti Koivisto]

Created attachment 13648 [details]
test case

Empty table cells should be ignored in percent width calculations. Specifying border makes FF consider them non-empty (though border is still not drawn). Specifying background does not have this effect.

[Mark Rowe (bdash)]

<rdar://problem/5067926>

[Antti Koivisto]

Created attachment 13929 [details]
more complete test case

[Antti Koivisto]

Created attachment 13933 [details]
special handling for empty table cells

This behavior matches Firefox exactly (as far as the test case goes at least) and is pretty close to IE7 too. The patch causes failures in result of tests listed below. I went through the failures and they all seem to be meaningless, not visible in rendering or progressions. For example pixel results of acid2 do not change even though the render tree dump does.

New results for these would need to be checked in as well. Test fast/css/percentage-non-integer relies on old empty cell behavior and would need modifications to look nice again.

fast/block/positioning/negative-right-pos
fast/css/acid2-pixel	
fast/css/acid2
fast/css/percentage-non-integer
fast/repaint/table-cell-move
fast/table/012
fast/table/032
fast/table/empty-section-crash
http/tests/misc/acid2-pixel	
http/tests/misc/acid2	
tables/mozilla/bugs/bug1188	
tables/mozilla/bugs/bug1818-6	
tables/mozilla/bugs/bug16012
tables/mozilla/bugs/bug78162	
tables/mozilla/bugs/bug100334	
tables/mozilla/bugs/bug222336	
tables/mozilla/bugs/bug222467
tables/mozilla/core/misc
tables/mozilla_expected_failures/bugs/bug14007-1	
tables/mozilla_expected_failures/bugs/bug72393
tables/mozilla_expected_failures/other/empty_cells

[Antti Koivisto]

Created attachment 13934 [details]
even more complete test case

[Darin Adler]

Comment on attachment 13933 [details]
special handling for empty table cells

+            if(!m_layoutStruct[lastCol].emptyCellsOnly)

Needs a space after the if.

         case Static:
-            numAuto++;
-            totalAuto += m_layoutStruct[i].effMaxWidth;
-            allocAuto += w;
+            if (m_layoutStruct[i].emptyCellsOnly) 
+                numAutoEmptyCellsOnly++;            
+            else {
+                numAuto++;
+                totalAuto += m_layoutStruct[i].effMaxWidth;
+                allocAuto += w;
+            }
         default:
             break;

It's pretty cheesy how this shares the break with the default case. I suggest we remove the default altogether so we get the gcc warning about any missing case values.

r=me

[Darin Adler]

Comment on attachment 13933 [details]
special handling for empty table cells

We need a new patch that includes the additional test case and new results for all the existing tests where results change.

So I'm clearing the review flag form this patch.

[Antti Koivisto]

r21145