If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Nested table very narrow when a column has high percentage width

RESOLVED FIXED

Status

()

Core
Layout: Tables
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Chris Boyle, Assigned: dholbert)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 315112 [details]
Table width test
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.
Blocks: 368504
Status: UNCONFIRMED → NEW
Component: General → Layout: Tables
Ever confirmed: true
Keywords: regression, testcase
OS: Linux → All
Product: Firefox → Core
QA Contact: general → layout.tables
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Comment 3

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

Me too.  I'll investigate.
Assignee: nobody → dholbert
(Assignee)

Comment 4

10 years ago
Created attachment 316058 [details]
testcase 2
(Assignee)

Comment 5

10 years ago
Created attachment 316059 [details]
reference 2
(Assignee)

Comment 6

10 years ago
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.)
(Assignee)

Comment 7

10 years ago
Created attachment 316068 [details]
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)
(Assignee)

Comment 8

10 years ago
Created attachment 316069 [details]
reference 3

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

Comment 9

10 years ago
Created attachment 316070 [details]
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%.
(Assignee)

Comment 10

10 years ago
Created attachment 316071 [details]
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)
(Assignee)

Comment 11

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

s/row/line
(Assignee)

Comment 12

10 years ago
Created attachment 316092 [details]
testcase 4
(Assignee)

Comment 13

10 years ago
Created attachment 316093 [details]
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.
(Assignee)

Comment 14

10 years ago
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.
Flags: blocking1.9?
(Assignee)

Comment 15

10 years ago
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.
(Assignee)

Comment 16

10 years ago
Created attachment 317466 [details] [diff] [review]
fix v1

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

Going to see if this passes reftests.
(Assignee)

Comment 17

10 years ago
(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.
(Assignee)

Comment 18

10 years ago
Created attachment 317473 [details] [diff] [review]
fix v2
Attachment #317466 - Attachment is obsolete: true
(Assignee)

Comment 19

10 years ago
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.
Attachment #317473 - Flags: superreview?(dbaron)
Attachment #317473 - Flags: review?(dbaron)
(Assignee)

Comment 20

10 years ago
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.
(Assignee)

Comment 21

10 years ago
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.
Status: NEW → ASSIGNED
Comment on attachment 317473 [details] [diff] [review]
fix v2

r+sr=dbaron
Attachment #317473 - Flags: superreview?(dbaron)
Attachment #317473 - Flags: superreview+
Attachment #317473 - Flags: review?(dbaron)
Attachment #317473 - Flags: review+
(Assignee)

Comment 23

10 years ago
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.
Attachment #317473 - Flags: approval1.9?
Comment on attachment 317473 [details] [diff] [review]
fix v2

a1.9=beltzner
Attachment #317473 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 25

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.