|Summary:||Wrong width calculation in table with fixed layout|
|Product:||WebKit||Reporter:||Beth Dakin <bdakin>|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Beth Dakin 2009-09-18 13:08:06 PDT
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] 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.