Bug 29501 - Wrong width calculation in table with fixed layout
Summary: Wrong width calculation in table with fixed layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-09-18 13:08 PDT by Beth Dakin
Modified: 2009-09-22 13:19 PDT (History)
1 user (show)

See Also:


Attachments
Reduction (299 bytes, text/html)
2009-09-18 13:08 PDT, Beth Dakin
no flags Details
Patch (122.63 KB, patch)
2009-09-18 13:17 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
New Patch (149.97 KB, patch)
2009-09-18 14:52 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Newer Patch (150.16 KB, patch)
2009-09-21 15:30 PDT, Beth Dakin
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2009-09-18 13:08:06 PDT
Created attachment 39782 [details]
Reduction

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
<rdar://problem/6925121>
Comment 2 Beth Dakin 2009-09-18 13:17:42 PDT
Created attachment 39783 [details]
Patch
Comment 3 mitz 2009-09-18 13:25:16 PDT
Comment on attachment 39783 [details]
Patch

> +    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

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