Bug 11645 – Table with percentage column widths doesn't scale to fill the entire width of a table containing it

• Component: Tables
• Status: RESOLVED FIXED

[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.

Created attachment 11567 [details]
Test case

[Robert Blaut]

*** Bug 15870 has been marked as a duplicate of this bug. ***

See also bug 17657.

*** Bug 17616 has been marked as a duplicate of this bug. ***

[Arpita Bahuguna]

Created attachment 162957 [details]
Patch

[Arpita Bahuguna]

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.

[WebKit Review Bot]

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

[Arpita Bahuguna]

(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.

[Arpita Bahuguna]

Created attachment 164394 [details]
Screenshot-025

(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.

[Julien Chaffraix]

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.

[Arpita Bahuguna]

Created attachment 167297 [details]
Patch

[Arpita Bahuguna]

(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.

[Build Bot]

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

[Arpita Bahuguna]

Created attachment 167479 [details]
Patch

[Arpita Bahuguna]

Uploaded another patch for passing the mac-ews.

[Julien Chaffraix]

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.

[Arpita Bahuguna]

Created attachment 169099 [details]
Patch

[Arpita Bahuguna]

(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.

[WebKit Review Bot]

Comment on attachment 169099 [details]
Patch

Clearing flags on attachment: 169099

Committed r133037: <http://trac.webkit.org/changeset/133037>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.