Bug 29447

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:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Bug Depends on:

68933

Bug Blocks:

15359

Attachments:

Description	Flags
Test case (try to refresh a couple of times)	none
Rendering of test case on different browsers	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-03	none
Patch	none
Patch	none
Patch	none

Tor Arne Vestbø

Reported 2009-09-18 07:57:17 PDT

This bug report originated from issue QTBUG-4033 <http://bugreports.qt.nokia.com/browse/QTBUG-4033> --- Description --- When an image is in a span frame then it can end up in the top right corner of the area and resized to small size

Attachments
Test case (try to refresh a couple of times) (211 bytes, text/html) 2009-12-07 06:29 PST, Jocelyn Turcotte	no flags	Details
Rendering of test case on different browsers (464.90 KB, image/png) 2011-06-28 13:56 PDT, Robert Hogan	no flags	Details
Patch (9.77 KB, patch) 2011-06-28 14:19 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.24 MB, application/zip) 2011-06-28 15:46 PDT, WebKit Review Bot	no flags	Details
Patch (10.90 KB, patch) 2011-07-09 06:46 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Patch (20.90 KB, patch) 2011-09-28 12:03 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Patch (21.05 KB, patch) 2011-10-12 13:33 PDT, Robert Hogan	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jocelyn Turcotte

Comment 1 2009-12-07 06:29:55 PST

Created attachment 44410 [details] Test case (try to refresh a couple of times) Minimal test case extracted from the initial customer report

Robert Hogan

Comment 2 2011-06-21 10:59:08 PDT

I can reproduce this in Chromium so it's a WebCore problem.

Robert Hogan

Comment 3 2011-06-27 11:02:03 PDT

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.

Robert Hogan

Comment 4 2011-06-28 12:37:35 PDT

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>

Robert Hogan

Comment 5 2011-06-28 13:56:12 PDT

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.

Robert Hogan

Comment 6 2011-06-28 14:19:13 PDT

Created attachment 98971 [details] Patch

Robert Hogan

Comment 7 2011-06-28 14:20:45 PDT

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

Andreas Kling

Comment 8 2011-06-28 14:38:27 PDT

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

WebKit Review Bot

Comment 9 2011-06-28 15:46:03 PDT

Comment on attachment 98971 [details] Patch Attachment 98971 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8950891

WebKit Review Bot

Comment 10 2011-06-28 15:46:09 PDT

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

Robert Hogan

Comment 11 2011-07-09 06:46:17 PDT

Created attachment 100207 [details] Patch

Dave Hyatt

Comment 12 2011-07-12 15:10:30 PDT

Comment on attachment 100207 [details] Patch Give me a bit of time. I need to play with this.

Robert Hogan

Comment 13 2011-08-08 12:20:30 PDT

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

Dave Hyatt

Comment 14 2011-09-26 12:39:12 PDT

Comment on attachment 100207 [details] Patch r=me

Robert Hogan

Comment 15 2011-09-27 12:30:57 PDT

Committed r96139: <http://trac.webkit.org/changeset/96139>

Robert Hogan

Comment 16 2011-09-28 12:03:03 PDT

Created attachment 109045 [details] Patch

Robert Hogan

Comment 17 2011-09-28 12:07:48 PDT

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

Simon Fraser (smfr)

Comment 18 2011-09-28 12:59:35 PDT

Reopening, since there is still work to do.

Dave Hyatt

Comment 19 2011-10-12 13:16:39 PDT

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.

Robert Hogan

Comment 20 2011-10-12 13:33:40 PDT

Created attachment 110737 [details] Patch

Dave Hyatt

Comment 21 2011-10-13 13:39:56 PDT

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?

Robert Hogan

Comment 22 2011-10-14 15:49:06 PDT

Committed r97525: <http://trac.webkit.org/changeset/97525>

mitz

Comment 23 2011-10-16 13:47:14 PDT

(In reply to comment #22) > Committed r97525: <http://trac.webkit.org/changeset/97525> This caused bug 70204.

mitz

Comment 24 2011-10-16 14:02:08 PDT

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?

Dave Hyatt

Comment 25 2011-10-16 14:21:23 PDT

I recommend we roll it out.

Robert Hogan

Comment 26 2011-10-17 00:48:05 PDT

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

mitz

Comment 27 2011-10-17 11:20:24 PDT

(In reply to comment #22) > Committed r97525: <http://trac.webkit.org/changeset/97525> Reverted in r97635.

Alexey Proskuryakov

Comment 28 2011-10-17 12:00:27 PDT

Comment on attachment 110737 [details] Patch Clearing r+ from reverted patch.

Robert Hogan

Comment 29 2011-10-24 10:59:28 PDT

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

Robert Hogan

Comment 30 2012-12-26 07:19:05 PST

Something has fixed this - I cannot recreate it on r138211.

Note You need to log in before you can comment on or make changes to this bug.