Bug 29501

Summary: Wrong width calculation in table with fixed layout
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: TablesAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: bdakin
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Description Flags
New Patch
Newer Patch hyatt: review+

Description Beth Dakin 2009-09-18 13:08:06 PDT
Created attachment 39782 [details]

Safari does a different width calculation than Firefox or IE for a cell of a table that has table-layout:fixed. This leads to serious problems with our UIs in Safari. In the other browsers the width for such containers is done by taking the last parent container with a defined width - this seems not to be the case for Safari.                                 
See attached HTML
Comment 1 Beth Dakin 2009-09-18 13:08:28 PDT
Comment 2 Beth Dakin 2009-09-18 13:17:42 PDT
Created attachment 39783 [details]
Comment 3 mitz 2009-09-18 13:25:16 PDT
Comment on attachment 39783 [details]

> +    int maxPercentValue = m_table->style()->width().isPercent() ? m_table->style()->width().calcValue(m_table->view()->width()) : 0;

I don’t understand why you use the RenderView here. Will this do the right thing if the table’s container is wider than the RenderView?
Comment 4 Beth Dakin 2009-09-18 13:27:02 PDT
I used the RenderView because I found all other options in my test cases to be too small. I didn't have a test where the table's container is wider than the view though. I will write a quick test and get back to you on that.
Comment 5 Beth Dakin 2009-09-18 14:02:38 PDT
Dan and I talked about this a bunch on IRC. Let me summarize here. In my debugging, I discovered that in this case, we can set the maxPrefWidth to anything ridiculously large, and we will get the correct result. Basically, the number is meaningless, it just needs to be Big Enough so it doesn't prevent calculations later down the line. Logically speaking, it seems like we are looking for the first container of the fixed table that has a non auto width, and we want to use that as out arbitrarily-large  size. (I call this solution firstNonAutoContainer.) However, for some reason this solution was always a few pixels short for me. Very frustrating!! Here are our options as I see them:

1. Set maxPrefWidth to the value of the biggest container we have. 
2. Set it to some other constant BIG number. 
3. Add a bool or something that answers isMyMaxPrefWidthEvenMeaningful(). 
4. Figure out why firstNonAutoContainer is always a few pixels short. 
5. New approach

I am going to whip up 1, because it will be fast, and it is what I meant for this patch to be all along. However, Dan pointed out that going with the size of the view is overly simple. We can always refine this is we decide that one of the other options is much better.
Comment 6 Beth Dakin 2009-09-18 14:52:20 PDT
Created attachment 39792 [details]
New Patch
Comment 7 Beth Dakin 2009-09-21 15:30:11 PDT
Created attachment 39886 [details]
Newer Patch

Here's a new iteration of this patch that implements the same idea. Dan and I were looking into this more, and we saw a very very similar quirk in RenderBlock's prefWidth calculation. This iteration of the patch makes this fix similar to that quirk we found in RenderBlock. 

The similar quirk was implemented in http://trac.webkit.org/changeset/4316 Hyatt wrote that patch and probably has the most expertise in this area, so we think he should probably review.
Comment 8 Dave Hyatt 2009-09-22 12:58:20 PDT
Comment on attachment 39886 [details]
Newer Patch

Comment 9 Beth Dakin 2009-09-22 13:19:59 PDT
Fixed in revision 48647.