413286 - table cell spanning a column with width of "*" is too small

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect, P1)

Description

[Elmar Ludwig]

The second table in the attached testcase has a total width of 400px and a COLGROUP definition of:

 <colgroup>
  <col width="100">
  <col width="*">
  <col width="25%">
 </colgroup>

It contains a table cell that spans the second and third columns. Before the checkin for bug 368504, this cell had a width of 75% (300px). Since then, the column width is only 25% (100px). I believe the former sizing was more correct.

In the testcase, the allocated cell widths are as follows:

Table 1: 25%, 50%, 25%
Table 2: 75%, 25%
Table 3: 50%, 50%

If my interpretation of the HTML specification is correct, I would expect the following cell widths:

Table 1: 25%, 50%, 25%
Table 2: 25%, 75%
Table 3: 25%, 75%

The widths of the cells in table 3 didn't change with bug 368504, but they also do not look correct to me. That is probably a topic for a different bug, though.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012004 Minefield/3.0b3pre ID:2008012004

Comment 1

[Elmar Ludwig]

Attachment: testcase

testcase described in comment #0

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> The second table [...]
> Before the
> checkin for bug 368504, this cell had a width of 75% (300px). Since then, the
> column width is only 25% (100px). I believe the former sizing was more correct.

I agree -- the former sizing was correct and the post-368504-rendering is incorrect.

I'll look into this.  

Flagging blocking?, as this is a newly-introduced layout regression.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like changing the cols into tds makes the problem go away.  That shouldn't be the case, since they're treated almost identically.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, but then adding cellpadding="0" to the tables makes the problem come back, which makes sense, since columns act like cells with 0 padding.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: megatest

This test just repeats the same table pattern, with variations on which cells have text content. ("xxx")

Here's the behavior of this test on various browsers.
By "wide blue cells", I mean blue cells that take up 3/4 of the table (300px), rather than 1/4 of the table (100px).

* FF2, IE7, Konqueror:
   With Colspan:     No wide blue cells
   Without Colspan:  Wide blue cells on tests 1,2,4,6

* Trunk, pre-368504
   With Colspan:     Wide blue cells on tests 1,2
   Without Colspan:  Wide blue cells on tests 1,2,4,6

* Trunk, post-368504
   With Colspan:     Wide blue cells on tests 1,2,3,4
   Without Colspan:  Wide blue cells on tests 1,2,4,6
   
* Opera
   With Colspan:     No wide blue cells
   Without Colspan:  No wide blue cells

Comment 6

[Daniel Holbert [:dholbert]]

So it looks like the following changes have occurred during trunk development (based on the results listed my previous comment):

  1. Between FF2 and Trunk pre-368504: 
       Expand blue cells on With-Colspan tests 1,2

  2. Between Trunk pre-368504 and Trunk post-368504:
       Expand blue cells on With-Colspan tests 3,4

Note also that Trunk is the only browser listed that has *any* wide blue cells on the With-Colspan tests.

So, I'd say we should do the following:
   1.  Have no wide blue cells on the With-Colspan tests (to match all other browsers)
   2.  Keep the Without-Colspan tests the way they are

...thereby making us match FF2 / Konqueror / IE7 behavior.

Comment 7

[philippe (part-time)]

fwiw, Safari 3 and the latest WebKit build on OS X match what Fx 2 does:
   With Colspan:     No wide blue cells
   Without Colspan:  Wide blue cells on tests 1,2,4,6

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: megatest 2: with padding

This is essentially the same as the first megatest, but with default cellpadding and cellspacing.  (I also made the cells shorter so that the whole test fits on the screen better.)  This changes the rendering dramatically.

FF2, Trunk, IE7, Opera, and Konqueror 3.5.8 all render this testcase with *no* wide blue cells.

Interestingly, Konqueror 4.0 (and hence latest WebKit, I'm guessing?) renders it with wide blue cells on tests 1,2,4,6.  But I think that's their problem -- Trunk is doing the right thing on this testcase.

Comment 9

[philippe (part-time)]

(In reply to comment #8)

WebKit latest build and Safari 3.04 render the same as Firefox 2. 

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> Created an attachment (id=298834) [details]
> megatest 2: with padding
> 
> This is essentially the same as the first megatest, but with default
> cellpadding and cellspacing.  (I also made the cells shorter so that the whole
> test fits on the screen better.)  This changes the rendering dramatically.
> 
> FF2, Trunk, IE7, Opera, and Konqueror 3.5.8 all render this testcase with *no*
> wide blue cells.

d'oh -- I thought I'd tested post-368504-trunk on megatest2, but I guess I didn't.

Post-368504-trunk has wide blue cells on *all* "With colspan" tables in megatest 2 (attachment 298834 [details]), unlike any other browser. (including pre-368504-trunk)  So that definitely needs fixing.


(In reply to comment #9)
> WebKit latest build and Safari 3.04 render the same as Firefox 2. 

Thanks philippe.  Konqueror 4.0 must use a weird build of WebKit, then.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch v1 (with reftests)

This patch fixes the problem (and still passes reftest suite) by making the following changes:
 - Adds the boolean "mSpanHasPrefWidth" to nsTableColFrame; this bool is set iff the colFrame is part of a colspanning cell that either...
   a) ...has nonzero pref or pct width
   b) ...spans a column that has a nonzero pref or pct width

 - Adds a second Loop2Type, to make use of this boolean in situations where we have excess width, and we have auto-width columns, but they all have zero pref width and yet some of them are part of spans that have some preferred width.  In this new case, we distribute the excess width equally among any such columns that have the new "mSpanHasPrefWidth" flag set.


This patch also includes some reftests.  The tests are based on "megatest" (attachment 298613 [details]), and they only differ from each other in the widths for col.a and col.c.  (they get either a specified width or a percent width, depending on the test).  The reference cases define exactly what the output should look like.  (matching FF2, IE7, and WebKit)  These testcases are also currently posted on the web at http://people.mozilla.com/~dholbert/tests/413286/reftests/

Pre-patching, the reftests all fail in the "with colspan" section, making the blue section too wide.  They pass post-patching.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase for %-width cases

My first serious concern with this patch is the fact that you're setting mSpanHasPrefWidth for things that have nonzero %-width.  I'm not sure what testcase led you to do that.

Doesn't that lead you to expand the dark blue cells in the second half of this testcase like you do in the first?  I couldn't find any other browsers that do so.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #12)
> My first serious concern with this patch is the fact that you're setting
> mSpanHasPrefWidth for things that have nonzero %-width.  I'm not sure what
> testcase led you to do that.

By that I mean that in places like this:
+            bool spanContainsPrefWidth = (info.prefCoord != 0 || 
+                                          info.prefPercent != 0.0f);
I think you should be checking only prefCoord and not checking prefPercent.  (This pattern occurs a bunch of times in the patch.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: more testcases

So this part doesn't really make any sense to me:

+            if (!spanContainsPrefWidth) {
+                // Condition B: If not, do we span any columns that have pref
+                // or percent width?
+                PRInt32 scol, scol_end;
+                for (scol = col, scol_end = col + colSpan;
+                     scol < scol_end; ++scol) {
+                    nsTableColFrame *scolFrame = mTableFrame->GetColFrame(scol);
+                    if (!scolFrame) {
+                        NS_ERROR("column frames out of sync with cell map");
+                        continue;
+                    }
+                    if (scolFrame->GetPrefCoord() != 0 || 
+                        scolFrame->GetPrefPercent() != 0.0f) {
+                        spanContainsPrefWidth = PR_TRUE;
+                        break;
+                    }
+                }
+            }

so I wrote a few testcases to test it.  It seems to me that what IE/Windows is doing is simply checking whether there are any spanning cells at all.  I'm not sure that really makes sense; I think we've previously deviated from IE/Windows in honoring width constraints on spanning cells, although I could be wrong (my memory of bug 379361 and bug 368504 isn't that great).

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: more testcases for zero width columns

If we wanted to do what was most intuitive for authors, we'd remove the rule that we don't expand 0-width columns (except in the case where the 0 was explicitly specified).  That's almost what Opera does (except it doesn't check for a specified 0 width), but I'm not sure Opera is enough to make us think it's Web-compatible.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I still haven't looked into:
 * whether % or specified width on the spanning cells influences this
 * whether any of this IE behavior is row-order dependent

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 299073 [details] [diff] [review]
patch v1 (with reftests)

So I'm still managing to have myself confused with testcases, and I think some more need to be written (combining some of the things I attached above, and also the things I mentioned above).

But I currently think the following:
 * I think the behavior of expanding zero-width columns is a more sensible behavior for authors, except when there's an explicit width="0".  But Opera is the only browser that does that, and I'm not sure we could get away with it.  However, we're probably better off moving as close as possible.
 * What IE seems to do is act as though the added boolean column member is "mHasNonPctSpanningCell" -- in other words, a variable that indicates that there exists a column-spanning cell spanning the column that has either a coordinate width or no width at all.

So, at the very least, I'd like to see a new patch that:
 * doesn't change the bottom three tests in attachment 302905 [details].
 * has reftests for that case
 * avoids the complexity of looping over all the cells in the column-spanning cell (this seems to produce results on the first 6 tests in attachment 302914 [details] that are the exact opposite of what old Gecko and what Safari do)

That said, I haven't looked much into what old Gecko and what Safari do, and whether that is a sensible alternative to what IE does.  (I'd consider three criteria: Web-compatibility, simplicity, and matching author expectations.)

I might still have more to say... and feel free to bug me in person.

Comment 18

[Daniel Holbert [:dholbert]]

First of all, thanks for the review and all the great testcases!

(In reply to comment #12)
> My first serious concern with this patch is the fact that you're setting
> mSpanHasPrefWidth for things that have nonzero %-width.  I'm not sure what
> testcase led you to do that.

I added this percent check...

+                    if (scolFrame->GetPrefCoord() != 0 || 
+                        scolFrame->GetPrefPercent() != 0.0f) {
+                        spanContainsPrefWidth = PR_TRUE;
+                        break;
+                    }

to make the first two lines of FF3 match FF2, IE7, Konqueror, and Opera's behavior on the first two lines of attachment 298613 [details].

Specifically, this makes us follow this path of reasoning on that testcase:
    col 3 has percent width...
       --> Triggers mSpanHasPrefWidth flag on the span's columns (cols 2 and 3)
       --> When we do final width allocation for the table, we have extra width, and we allow ourselves to allocate it to col 2 (even though it's empty) because it has mSpanHasPrefWidth flag set

I added this percent check...
+            bool spanContainsPrefWidth = (info.prefCoord != 0 || 
+                                          info.prefPercent != 0.0f);
... which you referenced in Comment 13, so as to be consistent with the other check I described above.  But maybe that's bad... I think that's the one that's causing us to change the bottom three tests in attachment 302905 [details].

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: more testcases for zero width columns (using style="width: 0px")

This is the same as attachment 302922 [details], except it uses 
  style="width: 0px"
instead of
  width="0"
on the explicitly-specified zero-width columns.  As it turns out, Trunk handles these two cases differently -- width=0 is treated as auto-width, but it's honored when we specify it as part of the style string.
(reference:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLTableCellElement.cpp#338
)

We should probably change how we handle those, to be consistent.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: patch v2: let empty auto-width columns expand

(In reply to comment #17)
>  * I think the behavior of expanding zero-width columns is a more sensible
> behavior for authors, except when there's an explicit width="0".  But Opera is
> the only browser that does that, and I'm not sure we could get away with it. 
> However, we're probably better off moving as close as possible.

I agree.  This work-in-progress patch heads in that direction, letting us expand zero-width columns, as long as they don't have a specified width.

Somewhat surprisingly, this change passes all existing reftests except for one:

http://mxr.mozilla.org/seamonkey/source/layout/reftests/table-width/balancing-2.html

Here's a screenshot of our post-patch rendering: http://preview.tinyurl.com/3cvorq (Note the red blocks in the 3rd and 4th tables -- these are gone in the reference version).  The post-patch rendering actually matches Opera, and it really makes sense if we're expanding zero-width columns.  The "broken" tables in the testcase have 4 specified-width columns and 1 auto-width empty column, and we have extra space to allocate.  So, we expand the empty red auto-width column, rather than any of the specified-width columns.

This patch also changes our behavior on the "megatests" (attachment 298613 [details] and attachment 298834 [details]) to match Opera -- all the blue cells are skinny  (i.e. all empty cells get expanded)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: testcase for %-width cases, #2

Here's an expanded version of attachment 302905 [details], with a few more cases.

Namely:
 -- Added a "Spanning cell has no specified width" case category
 -- Added higher percent-width cases (vs the existing "low percent" = 10% cases)
      For the spanning cell, "high percentage" = 50%
      For the third column, "high percentage" = 40%

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> Created an attachment (id=302905) [details]
> Doesn't that lead you to expand the dark blue cells in the second half of this
> testcase like you do in the first?  I couldn't find any other browsers that do
> so.
> 
(In reply to comment #17)
> So, at the very least, I'd like to see a new patch that:
>  * doesn't change the bottom three tests in attachment 302905 [details].
>  * has reftests for that case

So, like patch v1, patch v2 *does* expand the dark blue cells in the bottom three tests in attachment 302905 [details].  However, I think that's ok because...
 - Doing so makes us more consistent
 - In the more comprehensive attachment 303417 [details], IE7 expands the dark blue cells in all of the cases, including with percent-width colspans -- they just have a quirk where the expansion is really small on percent-width colspans.  I don't think that quirk makes sense, so I don't think we should try to emulate it.

Dbaron and I just talked about this in person, and I think we're in agreement about that.

So, before requesting review on this, I still need to
 a) add new reftests
 b) fix the width="0" issue mentioned in comment 19

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to comment #22)
>  b) fix the width="0" issue mentioned in comment 19

I actually filed this as bug 417826, as it's a somewhat separate issue from this bug here.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: (ignore; had another patch applied)

This is the same as patch v2, but with reftests now.  It just adds an additional loop2type that allows us to expand zero-width columns.

Along with the added reftests, this patch tweaks reftests/table-width/balancing-2.html (mentioned as failing in comment 20) so that we pass it.  It was failing because we were previously expanding pct-width and spec-width columns rather than expanding a zero-width column, but after this patch, we'll expand the zero-width column instead.

The added reftests are based on the testcases that dbaron posted:
 - 413286-2x.html and 413286-3.html are based on attachment 302905 [details]
 - 413286-4x.html is based on attachment 302914 [details].
 - 413286-5.html and 413286-6.html are based on attachment 303397 [details]

I've also posted all of these reftests at 
http://people.mozilla.com/~dholbert/tests/413286/reftests/

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: patch v2b: let empty auto-width columns expand (w/ reftests)

oops - the version that I just posted had my patch for bug 413180 applied as well.  here goes again, without that.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 304365 [details] [diff] [review]
patch v2b: let empty auto-width columns expand (w/ reftests)

>+     *   b. otherwise, if any columns without a specified coordinate 
>+     *   width or percent width have zero pref width, equally between
>+     *   these. [numNonSpecZeroWidthCols]

Might want to comment that this is for final width only.  (It's sort of too bad that it has to be, actually, but nobody else does it for span distribution.)

>+            } else if (pref_width == 0 &&
>+                       aWidthType == BTLS_FINAL_WIDTH &&
>+                       !colFrame->GetHasSpecifiedCoord()) {
>+                numNonSpecZeroWidthCols++;

You could actually do slightly better by squeezing this in to the if-else below as:

else if (pref_width == 0) {
  if (aWidthType == BTLS_FINAL_WIDTH) {
    ++numNonSpecZeroWidthCols;
  }
}

(between the then and the else)

Also change the postfix operator++ to prefix operator++ to fit with local style.  Prefix is a simpler operation conceptually -- more like +=, =, etc. -- and often actually simpler in code if you're dealing with custom implementations.

(And fix the numInfiniteWidthCols++ while you're there.)

>+            case FLEX_FLEX_LARGE_ZERO:
>+                NS_ASSERTION(aWidthType == BTLS_FINAL_WIDTH,
>+                             "FLEX_FLEX_LARGE_ZERO only should be hit "
>+                             "when we're setting final width.");
>+                if (pct == 0.0f &&
>+                    col_width == 0 &&
>+                    !colFrame->GetHasSpecifiedCoord()) {

You actually shouldn't need to test |col_width == 0| -- you should be able to assert it inside the if.

r+sr=dbaron with that.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: patch v2c: addressed review comments [checked in]

Thanks for the review!

Here's the patch with the review comments addressed.  I'm going to run it through reftests, and then I'll check in when the tree is quiet.

Comment 28

[Daniel Holbert [:dholbert]]

Attachment: interdiff between patches v2b / v2c

Comment 29

[Daniel Holbert [:dholbert]]

Patch v2c checked in.

RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-1-ref.html,v
done
Checking in layout/reftests/bugs/413286-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-1-ref.html,v  <--  413286-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-1a.html,v
done
Checking in layout/reftests/bugs/413286-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-1a.html,v  <--  413286-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-1b.html,v
done
Checking in layout/reftests/bugs/413286-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-1b.html,v  <--  413286-1b.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-1c.html,v
done
Checking in layout/reftests/bugs/413286-1c.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-1c.html,v  <--  413286-1c.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-2-ref.html,v
done
Checking in layout/reftests/bugs/413286-2-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-2-ref.html,v  <--  413286-2-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-2a.html,v
done
Checking in layout/reftests/bugs/413286-2a.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-2a.html,v  <--  413286-2a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-2b.html,v
done
Checking in layout/reftests/bugs/413286-2b.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-2b.html,v  <--  413286-2b.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-2c.html,v
done
Checking in layout/reftests/bugs/413286-2c.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-2c.html,v  <--  413286-2c.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-3-ref.html,v
done
Checking in layout/reftests/bugs/413286-3-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-3-ref.html,v  <--  413286-3-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-3.html,v
done
Checking in layout/reftests/bugs/413286-3.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-3.html,v  <--  413286-3.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-4-ref.html,v
done
Checking in layout/reftests/bugs/413286-4-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-4-ref.html,v  <--  413286-4-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-4a.html,v
done
Checking in layout/reftests/bugs/413286-4a.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-4a.html,v  <--  413286-4a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-4b.html,v
done
Checking in layout/reftests/bugs/413286-4b.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-4b.html,v  <--  413286-4b.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-5-ref.html,v
done
Checking in layout/reftests/bugs/413286-5-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-5-ref.html,v  <--  413286-5-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-5.html,v
done
Checking in layout/reftests/bugs/413286-5.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-5.html,v  <--  413286-5.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-6-ref.html,v
done
Checking in layout/reftests/bugs/413286-6-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-6-ref.html,v  <--  413286-6-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/413286-6.html,v
done
Checking in layout/reftests/bugs/413286-6.html;
/cvsroot/mozilla/layout/reftests/bugs/413286-6.html,v  <--  413286-6.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.363; previous revision: 1.362
done
Checking in layout/reftests/table-width/balancing-2-ref.html;
/cvsroot/mozilla/layout/reftests/table-width/balancing-2-ref.html,v  <--  balancing-2-ref.html
new revision: 1.3; previous revision: 1.2
done
Checking in layout/reftests/table-width/balancing-2.html;
/cvsroot/mozilla/layout/reftests/table-width/balancing-2.html,v  <--  balancing-2.html
new revision: 1.3; previous revision: 1.2
done
Checking in layout/tables/BasicTableLayoutStrategy.cpp;
/cvsroot/mozilla/layout/tables/BasicTableLayoutStrategy.cpp,v  <--  BasicTableLayoutStrategy.cpp
new revision: 3.266; previous revision: 3.265
done