Bug 13081 - Empty table cells not handled correctly
Summary: Empty table cells not handled correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://www.msnbc.msn.com/id/7383887/
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-03-15 08:04 PDT by Antti Koivisto
Modified: 2007-04-27 04:53 PDT (History)
1 user (show)

See Also:


Attachments
test case (1.16 KB, text/html)
2007-03-15 08:10 PDT, Antti Koivisto
no flags Details
more complete test case (4.05 KB, text/html)
2007-04-02 17:51 PDT, Antti Koivisto
no flags Details
special handling for empty table cells (26.13 KB, patch)
2007-04-03 07:28 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
even more complete test case (4.75 KB, text/html)
2007-04-03 07:29 PDT, Antti Koivisto
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2007-03-15 08:04:38 PDT
Empty table cells should be ignored in table layout calculations in same case. See the attached test case, compare with firefox.
Comment 1 Antti Koivisto 2007-03-15 08:10:19 PDT
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.
Comment 2 Mark Rowe (bdash) 2007-03-15 22:21:11 PDT
<rdar://problem/5067926>
Comment 3 Antti Koivisto 2007-04-02 17:51:09 PDT
Created attachment 13929 [details]
more complete test case
Comment 4 Antti Koivisto 2007-04-03 07:28:33 PDT
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
Comment 5 Antti Koivisto 2007-04-03 07:29:55 PDT
Created attachment 13934 [details]
even more complete test case
Comment 6 Darin Adler 2007-04-03 09:16:09 PDT
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
Comment 7 Darin Adler 2007-04-03 10:44:38 PDT
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.
Comment 8 Antti Koivisto 2007-04-27 04:53:06 PDT
r21145