Bug 29447 - Replaced elements squeezed when width is specified as percentage inside a table with Auto layout
Summary: Replaced elements squeezed when width is specified as percentage inside a tab...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P3 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 68933
Blocks: 15359
  Show dependency treegraph
 
Reported: 2009-09-18 07:57 PDT by Tor Arne Vestbø
Modified: 2012-12-26 07:19 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 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
Comment 1 Jocelyn Turcotte 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
Comment 2 Robert Hogan 2011-06-21 10:59:08 PDT
I can reproduce this in Chromium so it's a WebCore problem.
Comment 3 Robert Hogan 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.
Comment 4 Robert Hogan 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>
Comment 5 Robert Hogan 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.
Comment 6 Robert Hogan 2011-06-28 14:19:13 PDT
Created attachment 98971 [details]
Patch
Comment 7 Robert Hogan 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.
Comment 8 Andreas Kling 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.
Comment 9 WebKit Review Bot 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
Comment 10 WebKit Review Bot 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
Comment 11 Robert Hogan 2011-07-09 06:46:17 PDT
Created attachment 100207 [details]
Patch
Comment 12 Dave Hyatt 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.
Comment 13 Robert Hogan 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?
Comment 14 Dave Hyatt 2011-09-26 12:39:12 PDT
Comment on attachment 100207 [details]
Patch

r=me
Comment 15 Robert Hogan 2011-09-27 12:30:57 PDT
Committed r96139: <http://trac.webkit.org/changeset/96139>
Comment 16 Robert Hogan 2011-09-28 12:03:03 PDT
Created attachment 109045 [details]
Patch
Comment 17 Robert Hogan 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.
Comment 18 Simon Fraser (smfr) 2011-09-28 12:59:35 PDT
Reopening, since there is still work to do.
Comment 19 Dave Hyatt 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.
Comment 20 Robert Hogan 2011-10-12 13:33:40 PDT
Created attachment 110737 [details]
Patch
Comment 21 Dave Hyatt 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?
Comment 22 Robert Hogan 2011-10-14 15:49:06 PDT
Committed r97525: <http://trac.webkit.org/changeset/97525>
Comment 23 mitz 2011-10-16 13:47:14 PDT
(In reply to comment #22)
> Committed r97525: <http://trac.webkit.org/changeset/97525>

This caused bug 70204.
Comment 24 mitz 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?
Comment 25 Dave Hyatt 2011-10-16 14:21:23 PDT
I recommend we roll it out.
Comment 26 Robert Hogan 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.
Comment 27 mitz 2011-10-17 11:20:24 PDT
(In reply to comment #22)
> Committed r97525: <http://trac.webkit.org/changeset/97525>

Reverted in r97635.
Comment 28 Alexey Proskuryakov 2011-10-17 12:00:27 PDT
Comment on attachment 110737 [details]
Patch

Clearing r+ from reverted patch.
Comment 29 Robert Hogan 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.
Comment 30 Robert Hogan 2012-12-26 07:19:05 PST
Something has fixed this - I cannot recreate it on r138211.