Summary: | Replaced elements squeezed when width is specified as percentage inside a table with Auto layout | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tor Arne Vestbø <vestbo> | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Robert Hogan <robert> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, hyatt, jturcotte, kling, mitz, robert, simon.fraser, webkit.review.bot | ||||||||||||||||
Priority: | P3 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Bug Depends on: | 68933 | ||||||||||||||||||
Bug Blocks: | 15359 | ||||||||||||||||||
Attachments: |
|
Description
Tor Arne Vestbø
2009-09-18 07:57:17 PDT
Created attachment 44410 [details]
Test case (try to refresh a couple of times)
Minimal test case extracted from the initial customer report
I can reproduce this in Chromium so it's a WebCore problem. This test case exhibits another symptom of the problem: <html><body> <table> <td> <img src="http://webkit.org/images/icon-gold.png" width="100%" align="LEFT" border="1"> </td> </table> </body></html> Zooming in and out on this page enlarges and shrinks the image in unexpected ways. This is a relative of bug 15359. The difference with this test case is that the only the width is expressed as a percentage rather than just the height. I'm not sure why 15359 only checked for height. So this renders normally: <html><body> <table> <tr> <td> <img src="http://webkit.org/images/icon-gold.png" height="100%" align="LEFT" border="1"> </td> </tr> </table> </body></html> but this does not: <html><body> <table> <tr> <td> <img src="http://webkit.org/images/icon-gold.png" width="100%" align="LEFT" border="1"> </td> </tr> </table> </body></html> Created attachment 98967 [details]
Rendering of test case on different browsers
Current WebKit on the left, then Firefox, Opera, and finally patched WebKit on the right.
Created attachment 98971 [details]
Patch
(In reply to comment #6) > Created an attachment (id=98971) [details] > Patch If someone (dhyatt?) can confirm this approach isn't wrong-headed I'll add a proper rendertext test. (In reply to comment #7) > If someone (dhyatt?) can confirm this approach isn't wrong-headed I'll add a proper rendertext test. FYI dhyatt is on vacation. Comment on attachment 98971 [details] Patch Attachment 98971 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8950891 Created attachment 98986 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 100207 [details]
Patch
Comment on attachment 100207 [details]
Patch
Give me a bit of time. I need to play with this.
(In reply to comment #12) > (From update of attachment 100207 [details]) > Give me a bit of time. I need to play with this. Hi there - did you get a chance to take a look? Comment on attachment 100207 [details]
Patch
r=me
Committed r96139: <http://trac.webkit.org/changeset/96139> Created attachment 109045 [details]
Patch
(In reply to comment #16) > Created an attachment (id=109045) [details] > Patch The problem with the previous patch, which caused it to fail table-percent-height.html was that it was not walking up through the ancestors to check whether the block was nested inside a table cell. The attached fixes this. Reopening, since there is still work to do. Comment on attachment 109045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109045&action=review > Source/WebCore/rendering/RenderBox.cpp:1244 > +int RenderBox::containingBlockReplacedLogicalWidthForContent() const Should be LayoutUnit not int. > Source/WebCore/rendering/RenderBox.cpp:1248 > + return max(shrinkToAvoidFloats() ? cb->availableLogicalWidthForLine(y(), false) : cb->availableLogicalWidth(), intrinsicLogicalWidth()); Should be logicalTop() not y(). > Source/WebCore/rendering/RenderBox.h:288 > + int containingBlockReplacedLogicalWidthForContent() const; Should be LayoutUnit not int. Created attachment 110737 [details]
Patch
Comment on attachment 110737 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110737&action=review r=me. > Source/WebCore/rendering/RenderBox.cpp:1274 > +enum IntrinsicDimension { Height, Width }; Should use LogicalHeight and LogicalWidth here. Let's not make the terms so generic either... How about LogicalHeightDimension, LogicalWidthDimension? Committed r97525: <http://trac.webkit.org/changeset/97525> (In reply to comment #22) > Committed r97525: <http://trac.webkit.org/changeset/97525> This caused bug 70204. Moreover, it doesn’t look like the patch fixed the original test case in attachment 44410 [details]. Can this be fixed or should the patch just be reverted?
I recommend we roll it out. (In reply to comment #25) > I recommend we roll it out. I'm away from my pc for a few days so can't do this myself at the moment. Sorry for the breakage. (In reply to comment #22) > Committed r97525: <http://trac.webkit.org/changeset/97525> Reverted in r97635. Comment on attachment 110737 [details]
Patch
Clearing r+ from reverted patch.
(In reply to comment #24) > Moreover, it doesn’t look like the patch fixed the original test case in attachment 44410 [details]. Can this be fixed or should the patch just be reverted? That's odd, because it did fix it for me and it still does. In order to progress this again I would need to get my hands on some of the markup in iChat that revealed the regression. Would that be possible? I don't have iChat or OSX myself. Something has fixed this - I cannot recreate it on r138211. |