428521 - Nested table very narrow when a column has high percentage width

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Chris Boyle]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5

The test case which I will attach renders very narrowly on Firefox 3 beta 5, and is the full width of the page on Firefox 2 and other browsers. The test case is a nested table where one cell in the inner table is styled to have a width of 98%.

Reproducible: Always

Steps to Reproduce:
1. View the test case.
Actual Results:  
The table is very narrow.

Expected Results:  
The table should be the full width of the page.

If you edit the width percentage in the test case, say to 50%, everything is as expected. The table's width is determined by the large block of text. For me, the bug occurs when the width is 98% or greater.

The test case is a simplified version of a layout used in a web-based configuration interface for Zeus Extensible Traffic Manager, where it causes some settings pages to be unusually narrow and tall, but because of the nature of the product, there isn't a public instance to link to.

Comment 1

[Chris Boyle]

Attachment: Table width test

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows XP. Regression range is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2008-01-17+09%3A00&maxdate=2008-01-17+23%3A00
Sounds like caused by bug 368504.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> For me,
> the bug occurs when the width is 98% or greater.

Me too.  I'll investigate.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: reference 2

Comment 6

[Daniel Holbert [:dholbert]]

The only difference between testcase 2 and reference 2 is the %-width on the lower-right cell.

The situation in testcase 2 and reference 2 is this:
 - two-column colspan, whose min-width = 200px and pref-width = 400px
 - its first column has a percent-width
 - its second column has min and pref-width of 100px. (leaving 300px of pref-width for the first column's %-width to use up)

In the reference case, the 75.0%-width *exactly* uses up the 300px of available pref-width.

But in the testcase, the 75.1%-width asks for too much -- 300.4 px -- which we don't have available.  So that makes the table-layout snap down to forcing min-widths, somehow.

(Note that these testcases don't work quite right in FF2, because it doesn't support inline-block.  I'm making another set of testcases that don't rely on that.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

This testcase is like testcase 2, except it uses "float:left" instead of "display:inline-block" on the blue and green divs.  This gives an inline-block-type behavior that works under FF2 as well.  

(i.e. the divs start out on the same line in FF2, but they'll wrap if you constrain the width enough, e.g. if you shrink the browser width)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reference 3

(Everything stated in comment 6 basically applies to testcase 3 & reference 3 as well.)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: megatest 1

This 'megatest' repeats the table from testcase 3 across a variety of percent-widths.

FF2, IE7&8, and Safari 3.1 all show the blue and green cells on the same line.

FF3 / Trunk pushes the green cell to the next line starting at 75.1%.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: reference 3b

This reference is the same as testscase 3, except the outer wrapper-table is stripped away.  (which allows the blue & green lines to be on the same row for some reason)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #10)
> (which allows the blue & green lines to be on the same row for
> some reason)

s/row/line

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 4

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: reference 4

testcase 4 and reference 4 just accentuate the situation with testcase 3 and reference 3b.  They use 80% width on the orange cell.

Comment 14

[Daniel Holbert [:dholbert]]

Requesting blocking.

This is a somewhat scary table-layout regression / inconsistency...  Would like to at least better understand this, if not fix it, before closing for RC.

Comment 15

[Daniel Holbert [:dholbert]]

Summary of why I consider this bug broken & somewhat scary:

AFAIK, wrapping an element in an unstyled table usually shouldn't affect its layout. But in this case, it does affect it.

Compare testcase 4 (attachment 316092 [details]) to reference 4 (attachment 316093 [details]).  They only differ in that testcase 4 wraps its contents in a <table cellspacing=0 cellpadding=0>, and that messes up the rendering.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Haven't yet convinced myself that this is correct, but it fixes the problem.

Going to see if this passes reftests.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to bug 368504 comment #61)
> You may also want to change the condition to be
>   if (aWidthType != BTLS_FINAL_WIDTH && aWidth <= guess_min)
> and then move the equivalent of the assertion to outside the condition.
> 
> In fact, I'd think you could even change it to:
> 
>   if ((aWidthType == BTLS_MIN_WIDTH  && aWidth <= guess_min) ||
>       (aWidthType == BTLS_PREF_WIDTH && aWidth <= guess_pref))
> 
> although I'm not 100% sure that's ok (you should check).

Ah -- fix v1 here isn't exactly correct.

We in fact want to be using the first (one-line) check that dbaron suggested in the quote above, and *not* the second one, which is what's currently in the code.

Attaching a fix v2 shortly, along with reftests & an explanation of what's going on here.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 317473 [details] [diff] [review]
fix v2

Basically, this patch just reverts a minor optimization that was added to the bug 368504 patch in response to a review comment (referenced here in comment 17).  At the time, I'd thought that the optimization wouldn't affect our behavior, but it's now clear that it does break our behavior on this bug's testcases.  

This patch passes reftests.  It should be a very safe fix, because it's just rolling back a (minor) optimization.

I'll post more detail about how the optimization ended up affecting our behavior in my next comment.

Comment 20

[Daniel Holbert [:dholbert]]

More Detailed Description of What's Broken
==========================================

So the problem situation that we're running into is this:
 - We have a column with a high percent-width, which is part of a colspan.  In particular, the percent-width is high enough that the column asks for *more of the colspan's pref-width than is actually available*.  (e.g  colspan_width = 400px, col0_width = 75.1%, and col1_width = 100px)

 - During a BTLS_PREF_WIDTH run of DistributeWidthToColumns:
    -- Because our column has a high percent-width, we end up with a large value for guess_pref, here: http://tinyurl.com/3ljr46 (e.g. 300.4 px + 100px = 400.4px)
    --We see that the "available width" aWidth (e.g. 400 px) is less than guess_pref (e.g. 400.4px), so we hit the return-early clause, here: http://tinyurl.com/3t9a4p

THIS IS BAD because we never actually end up setting the preferred width of the percent-width column!! Hence, the preferred width ends up being *whatever that column's min-width is*.

This problem is actually masked in many cases (i.e. when we don't have nested tables), because even though we assign the wrong preferred width in the BTLS_PREF_WIDTH run, we end up saving ourselves later during the BTLS_FINAL_WIDTH run by allocating any extra available width to the percent-width column.

The wrapper-table complicates things, though, and prevents us from saving ourselves in this way.  Basically, before the BTLS_FINAL_WIDTH run, the wrapper-table queries its child table for a preferred width, and the child returns a width that's much too small because of this bug.  Then, that incorrect width ends up being how much space the child table has to work with during its BTLS_FINAL_WIDTH run.

Comment 21

[Daniel Holbert [:dholbert]]

dbaron: I know the previous comment is a bit confusing, and it glosses over a number of things -- I'm happy to explain things in more detail via IRC or phone if necessary.

In any case, the details aren't as important as the underlying idea that this was just due to an unexpected consequence of a minor optimization.

Comment 22

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

Comment on attachment 317473 [details] [diff] [review]
fix v2

r+sr=dbaron

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 317473 [details] [diff] [review]
fix v2

Requesting approval.  

Justification:
 - Very safe fix, per comment 19 (just reverts a minor optimization)
 - Includes regression test
 - Bug is nasty layout regression which made us disagree with all other browsers; definitely needs to be fixed.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317473 [details] [diff] [review]
fix v2

a1.9=beltzner

Comment 25

[Daniel Holbert [:dholbert]]

Fix v2 checked in:

RCS file: /cvsroot/mozilla/layout/reftests/bugs/428521-1-ref.html,v
done
Checking in layout/reftests/bugs/428521-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/428521-1-ref.html,v  <--  428521-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/428521-1a.html,v
done
Checking in layout/reftests/bugs/428521-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/428521-1a.html,v  <--  428521-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/428521-1b.html,v
done
Checking in layout/reftests/bugs/428521-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/428521-1b.html,v  <--  428521-1b.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/428521-1c.html,v
done
Checking in layout/reftests/bugs/428521-1c.html;
/cvsroot/mozilla/layout/reftests/bugs/428521-1c.html,v  <--  428521-1c.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.448; previous revision: 1.447
done
Checking in layout/tables/BasicTableLayoutStrategy.cpp;
/cvsroot/mozilla/layout/tables/BasicTableLayoutStrategy.cpp,v  <--  BasicTableLayoutStrategy.cpp
new revision: 3.269; previous revision: 3.268
done