Bug 11645 - Table with percentage column widths doesn't scale to fill the entire width of a table containing it
Summary: Table with percentage column widths doesn't scale to fill the entire width of...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction
: 15870 17616 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-11-18 13:41 PST by mitz
Modified: 2012-10-31 09:32 PDT (History)
11 users (show)

See Also:


Attachments
Test case (648 bytes, text/html)
2006-11-18 13:42 PST, mitz
no flags Details
Patch (33.72 KB, patch)
2012-09-08 05:09 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Screenshot-025 (36.42 KB, image/png)
2012-09-17 07:44 PDT, Arpita Bahuguna
no flags Details
Patch (25.40 KB, patch)
2012-10-05 03:21 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (25.84 KB, patch)
2012-10-07 02:46 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (28.61 KB, patch)
2012-10-16 23:59 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-11-18 13:41:42 PST
[This was found by Anders]

In the attached test case, the green table should fill the entire width of the red table (minus 2px padding on either side).

The logic that decides not to scale in this case is in shouldScaleColumns() in AutoTableLayout.cpp.
Comment 1 mitz 2006-11-18 13:42:08 PST
Created attachment 11567 [details]
Test case
Comment 2 Robert Blaut 2008-02-15 14:01:12 PST
*** Bug 15870 has been marked as a duplicate of this bug. ***
Comment 3 mitz 2008-03-03 22:14:02 PST
See also bug 17657.
Comment 4 mitz 2008-03-09 15:58:07 PDT
*** Bug 17616 has been marked as a duplicate of this bug. ***
Comment 5 Arpita Bahuguna 2012-09-08 05:09:26 PDT
Created attachment 162957 [details]
Patch
Comment 6 Arpita Bahuguna 2012-09-08 05:20:50 PDT
Proposed patch uploaded.

Am unsure as to why the check in shouldScaleColumns() in AutoTableLayout.cpp, for disallowing the scaling of columns in case of the containing table having percent width was placed in the first place.

AFAICT removing that check fixes our issue and also makes our rendering of such scenarios similar to that of FF, Opera and I.E.

I would really appreciate if someone would have any pointers regarding the same.
Comment 7 WebKit Review Bot 2012-09-08 06:57:00 PDT
Comment on attachment 162957 [details]
Patch

Attachment 162957 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13799202

New failing tests:
fast/table/025.html
Comment 8 mitz 2012-09-15 09:47:43 PDT
Comment on attachment 162957 [details]
Patch

The new result in fast/table/025.html looks wrong, since now the two tables don’t match. In Firefox 15.0.1 they do.
Comment 9 Arpita Bahuguna 2012-09-17 07:43:45 PDT
(In reply to comment #8)
> (From update of attachment 162957 [details])
> The new result in fast/table/025.html looks wrong, since now the two tables don’t match. In Firefox 15.0.1 they do.
Hi Dan,
Even the existing expected result for fast/table/025.html shows a similar discrepancy between the two tables present in the test.
[https://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/table/025-expected.png]

Strangely though, if I were to directly load this test on chrome (from my cr-linux workspace), no such difference is visible.
[Have attached a screenshot of the same - Screenshot-025.png]

Am unsure on how to proceed with this since every time I generate the results using my chrome workspace I always get this discrepancy between the two tables, in spite of it displaying the test properly (i.e. without the difference between the two tables) on chrome from the same workspace.
Comment 10 Arpita Bahuguna 2012-09-17 07:44:22 PDT
Created attachment 164394 [details]
Screenshot-025
Comment 11 mitz 2012-09-17 08:11:17 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 162957 [details] [details])
> > The new result in fast/table/025.html looks wrong, since now the two tables don’t match. In Firefox 15.0.1 they do.
> Hi Dan,
> Even the existing expected result for fast/table/025.html shows a similar discrepancy between the two tables present in the test.
> [https://trac.webkit.org/browser/trunk/LayoutTests/platform/chromium-linux/fast/table/025-expected.png]
> 
> Strangely though, if I were to directly load this test on chrome (from my cr-linux workspace), no such difference is visible.
> [Have attached a screenshot of the same - Screenshot-025.png]
> 
> Am unsure on how to proceed with this since every time I generate the results using my chrome workspace I always get this discrepancy between the two tables, in spite of it displaying the test properly (i.e. without the difference between the two tables) on chrome from the same workspace.

I see now. The tables differ when the viewport is made narrow enough. This happens both in TOT WebKit (as evident by the expected results) and in Firefox.
Comment 12 Julien Chaffraix 2012-10-02 17:45:25 PDT
Comment on attachment 162957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162957&action=review

The code change is fine, AFAICT the test is progressing but would like to see another round with the proper justifications.

> LayoutTests/ChangeLog:14
> +        * platform/chromium-linux/fast/table/025-expected.png:
> +        * platform/chromium-win/fast/table/025-expected.png:

You should only need the chromium-linux baseline here. The fact that it fails leads me to think that your machine's configuration doesn't match the EWS. Unfortunately, they don't upload their baselines to the bugs anymore and I don't know if those are available somewhere.

> LayoutTests/ChangeLog:16
> +        Existing test case's results have been modified as per this change.

As pointed out by Dan, you should explain what the difference is and why this is a progression.

As written, this line answers none of those questions and thus adds no value to the reader.

> LayoutTests/fast/table/scale-nested-percent-width-cols-expected.html:13
> +                        <table style="width: 100%; background: green;">

The difference between the expected and the normal result is really not clear cut which makes it hard to say if it's correct.

Also if you used to not scale when you encountered any percent width, the test as written would pass while actually failing. The expected result should probably have fixed lengths to avoid any such ambiguity.
Comment 13 Arpita Bahuguna 2012-10-05 03:21:57 PDT
Created attachment 167297 [details]
Patch
Comment 14 Arpita Bahuguna 2012-10-05 04:14:57 PDT
(In reply to comment #12)
> (From update of attachment 162957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162957&action=review
> 
Thanks for the review Julien.

> The code change is fine, AFAICT the test is progressing but would like to see another round with the proper justifications.
> 
> > LayoutTests/ChangeLog:14
> > +        * platform/chromium-linux/fast/table/025-expected.png:
> > +        * platform/chromium-win/fast/table/025-expected.png:
> 
> You should only need the chromium-linux baseline here. The fact that it fails leads me to think that your machine's configuration doesn't match the EWS. Unfortunately, they don't upload their baselines to the bugs anymore and I don't know if those are available somewhere.
> 
I faced the same problem with 13598 as well and tried several combinations but all attempts failed.
Meanwhile, I added the expected files for the qt port (they show similar diff as cr-linux) and have added this test in the chromium testexpectations file to be rebaselined later.

> > LayoutTests/ChangeLog:16
> > +        Existing test case's results have been modified as per this change.
> 
> As pointed out by Dan, you should explain what the difference is and why this is a progression.
> 
> As written, this line answers none of those questions and thus adds no value to the reader.
> 
Have added explanation for the above in the changelog.

> > LayoutTests/fast/table/scale-nested-percent-width-cols-expected.html:13
> > +                        <table style="width: 100%; background: green;">
> 
> The difference between the expected and the normal result is really not clear cut which makes it hard to say if it's correct.
> 
Normally, i.e. without the fix, I would not expect my innermost table (with the green bg color to expand up to its containing table. This would make the containing table's bg color (which is red) to cover most of the table dimensions.
After the fix, since our innermost table would now scale, the green bg color of this table would now mask that of the containing tables.

> Also if you used to not scale when you encountered any percent width, the test as written would pass while actually failing. The expected result should probably have fixed lengths to avoid any such ambiguity.
I understand how it can perhaps be confusing, have thus changed the widths to fixed for my expected test.
Comment 15 Build Bot 2012-10-05 19:14:43 PDT
Comment on attachment 167297 [details]
Patch

Attachment 167297 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14181575

New failing tests:
fast/table/025.html
Comment 16 Arpita Bahuguna 2012-10-07 02:46:59 PDT
Created attachment 167479 [details]
Patch
Comment 17 Arpita Bahuguna 2012-10-07 05:22:32 PDT
Uploaded another patch for passing the mac-ews.
Comment 18 Julien Chaffraix 2012-10-16 17:38:52 PDT
Comment on attachment 167479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167479&action=review

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:1
> +<!DOCTYPE html>

FWIW, this could have used check-layout.js (http://lists.webkit.org/pipermail/webkit-dev/2012-October/022490.html). The upside would be to remove the ref-test that ends up being super close to the test case.

Note that the ref-test approach works OK in this case too.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:6
> +<p>No red background color should be visible for the following two tables.</p>

For the record, you *do* see some red which makes this sentence confusing for anyone not familiar with tables.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:10
> +            <table style="width: 100%; background-color: red;">

Add: style="...; border-spacing: 0px;" to your list...

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:12
> +                    <td>

... and add style="padding: 0px" here too and no red should be shown.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:13
> +                        <table style="background-color: green;">

I would support moving this common style into a class shared by the 2 examples.

> LayoutTests/platform/chromium/TestExpectations:3758
> +# Requires rebaselining after https://bugs.webkit.org/show_bug.cgi?id=11645
> +webkit.org/b/11645 fast/table/025.html [ Failure ]

You are right to add that.

A couple of comments:
* What happened to gtk, win and efl?
* For Chromium, you really have the choice. Disabling it and doing the rebaselining yourself with webkit-patch rebaseline-server (or rebaseline-expectations) or just omitting so that the gardener picks the new baselines when the patch land.
Comment 19 Arpita Bahuguna 2012-10-16 23:59:57 PDT
Created attachment 169099 [details]
Patch
Comment 20 Arpita Bahuguna 2012-10-17 03:31:58 PDT
(In reply to comment #18)
> (From update of attachment 167479 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167479&action=review
> 
Thanks Julien for the review.

> > LayoutTests/fast/table/scale-nested-percent-width-cols.html:1
> > +<!DOCTYPE html>
> 
> FWIW, this could have used check-layout.js (http://lists.webkit.org/pipermail/webkit-dev/2012-October/022490.html). The upside would be to remove the ref-test that ends up being super close to the test case.
> 
> Note that the ref-test approach works OK in this case too.
> 
I wasn't aware of check-layout.js until now. Shall further study its usage for future use.

> > LayoutTests/fast/table/scale-nested-percent-width-cols.html:6
> > +<p>No red background color should be visible for the following two tables.</p>
> 
> For the record, you *do* see some red which makes this sentence confusing for anyone not familiar with tables.
> 
> > LayoutTests/fast/table/scale-nested-percent-width-cols.html:10
> > +            <table style="width: 100%; background-color: red;">
> 
> Add: style="...; border-spacing: 0px;" to your list...
> 
> > LayoutTests/fast/table/scale-nested-percent-width-cols.html:12
> > +                    <td>
> 
> ... and add style="padding: 0px" here too and no red should be shown.
> 
I understand, since red was still visible the statement would have been misleading. Have made the necessary changes as suggested by you.

> > LayoutTests/fast/table/scale-nested-percent-width-cols.html:13
> > +                        <table style="background-color: green;">
> 
> I would support moving this common style into a class shared by the 2 examples.
> 
Done.

> > LayoutTests/platform/chromium/TestExpectations:3758
> > +# Requires rebaselining after https://bugs.webkit.org/show_bug.cgi?id=11645
> > +webkit.org/b/11645 fast/table/025.html [ Failure ]
> 
> You are right to add that.
> 
> A couple of comments:
> * What happened to gtk, win and efl?
> * For Chromium, you really have the choice. Disabling it and doing the rebaselining yourself with webkit-patch rebaseline-server (or rebaseline-expectations) or just omitting so that the gardener picks the new baselines when the patch land.
Have added the existing failing test in the TextExpectation files for the remaining ports as well.
Comment 21 WebKit Review Bot 2012-10-31 09:32:48 PDT
Comment on attachment 169099 [details]
Patch

Clearing flags on attachment: 169099

Committed r133037: <http://trac.webkit.org/changeset/133037>
Comment 22 WebKit Review Bot 2012-10-31 09:32:54 PDT
All reviewed patches have been landed.  Closing bug.