[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
*** 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. ***
Created attachment 162957 [details] Patch
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 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.
(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.
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.
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.
Created attachment 167297 [details] Patch
(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 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
Created attachment 167479 [details] Patch
Uploaded another patch for passing the mac-ews.
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.
Created attachment 169099 [details] Patch
(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 on attachment 169099 [details] Patch Clearing flags on attachment: 169099 Committed r133037: <http://trac.webkit.org/changeset/133037>
All reviewed patches have been landed. Closing bug.