Bug 15359 - JPEG image not shown when height is specified as percentage inside a table
Summary: JPEG image not shown when height is specified as percentage inside a table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 419.x
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
: 5789 (view as bug list)
Depends on: 29447
Blocks: 58006
  Show dependency treegraph
 
Reported: 2007-10-03 05:54 PDT by Ton Bäcker
Modified: 2012-03-14 00:13 PDT (History)
6 users (show)

See Also:


Attachments
code snippet (311 bytes, text/html)
2007-10-03 05:56 PDT, Ton Bäcker
no flags Details
JPG file (76.21 KB, image/jpeg)
2007-10-03 05:57 PDT, Ton Bäcker
no flags Details
Online test case (306 bytes, text/html)
2007-10-04 04:28 PDT, David Kilzer (:ddkilzer)
no flags Details
Hack v1 (557 bytes, patch)
2007-10-04 09:47 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
Details | Formatted Diff | Diff
Patch v1 (334.98 KB, patch)
2007-10-07 17:21 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
Details | Formatted Diff | Diff
Patch v2 (347.79 KB, patch)
2007-12-27 20:18 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ton Bäcker 2007-10-03 05:54:21 PDT
JPG image is not shown with following code snippet.
When the height="75%" attribute is removed the image is shown.
This happens in Safari 2.0.4 (419.3) and the latest nightly build of webkit.
In Firefox 2.0.0.4 the image is shown.

!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<html>
	<head>
	</head>
	<body>
		<table cellpadding="0" cellspacing="0" border="0" width="100%">
			<tr>
				<td>
					<img src="Beeldcrypto.jpg" alt="crypto" width="75%" height="75%"> 
				</td>
			</tr>
		</table>
	</body>
</html>
Comment 1 Ton Bäcker 2007-10-03 05:56:58 PDT
Created attachment 16518 [details]
code snippet
Comment 2 Ton Bäcker 2007-10-03 05:57:42 PDT
Created attachment 16519 [details]
JPG file
Comment 3 David Kilzer (:ddkilzer) 2007-10-04 04:28:46 PDT
Created attachment 16530 [details]
Online test case
Comment 4 David Kilzer (:ddkilzer) 2007-10-04 09:12:18 PDT
Confirmed with a local debug build of WebKit r25955 with Safari 3 Public Beta v. 3.0.3 (522.12.1) on Mac OS X 10.4.10 (8R218).  As Comment #0 mentions, also occurs with Safari 2.0.4 (419.3) with original WebKit.

Comment 5 David Kilzer (:ddkilzer) 2007-10-04 09:47:53 PDT
Created attachment 16536 [details]
Hack v1

This is a hack that will allow the image to be display correctly within the table (e.g., it displays the same as if the table were removed from the test).  I'm setting the review flag to see if this is the correct approach.

Note that because of what isHeightSpecified() checks, this change in RenderImage::calcReplacedHeight():

+    if (isHeightSpecified() && style()->height().type() != Percent)

could really just be:

+    if (style()->height().type() == Fixed)

If this is not the correct approach, then I'd guess that RenderBox::calcReplacedHeightUsing() needs to be taught how to use instrinsicSize() for <img> tags.  (Am I in the ballpark here? :)

Layout tests running now with this change to check for any failures.
Comment 6 David Kilzer (:ddkilzer) 2007-10-04 10:43:35 PDT
Comment on attachment 16536 [details]
Hack v1

(In reply to comment #5)
> Layout tests running now with this change to check for any failures.

These tests failed, so this is obviously not the correct approach:

fast/replaced/absolute-position-percentage-height.html
fast/replaced/replaced-child-of-absolute-with-auto-height.html
fast/replaced/width100percent-image.html
tables/mozilla_expected_failures/bugs/bug85016.html
tables/mozilla_expected_failures/bugs/bug137388-2.html
Comment 7 David Kilzer (:ddkilzer) 2007-10-07 10:23:32 PDT
I believe this bug may be generalized to rendering replaced elements with a percentage height within a table cell with auto height (e.g., no height specified).

The default (intrinsic) size of a replaced element is 300px wide by 150px high.  Thus, this example should render a green 300x112 canvas element:

<table><tr><td>
<canvas style="background-color: #00ff00; height: 75%;"></canvas>
</td></tr></table>

On a local debug build of WebKit r26074 with Safari 3 Public Beta v. 3.0.3, this renders a 300x0 canvas element (as verified by the Web Inspector).
On Opera 9.22, this renders as a 300x1 canvas element.
On Firefox 2.0.0.7, this renders a 300x112 canvas element, but the containing table cell is 300x150 (instead of 300x112).
On FIrefox 3.0a9pre (07-Oct-2007), this renders as expected with a 300x112 canvas element.

Similar, this example referencing a 600x718 image should render as 450x538 (scaled by 75% proportionally):

<table><tr><td>
<img src="http://bugs.webkit.org/attachment.cgi?id=16519" alt="crypto" height="75%">
</td></tr></table>

On a local debug build of WebKit r26074 with Safari 3 Public Beta v. 3.0.3, this renders as a 0x0 image element (as verified by the Web Inspector).
On Opera 9.22, this renders as a 600x718 image element.
On Firefox 2.0.0.7, this renders a 450x538 image element, but the containing table cell is 600x718 (instead of 450x538).
On FIrefox 3.0a9pre (07-Oct-2007), this renders as a 600x718 image element.

Comment 8 David Kilzer (:ddkilzer) 2007-10-07 11:11:51 PDT
(In reply to comment #7)
> The default (intrinsic) size of a replaced element is 300px wide by 150px high.
>  Thus, this example should render a green 300x112 canvas element:
> 
> <table><tr><td>
> <canvas style="background-color: #00ff00; height: 75%;"></canvas>
> </td></tr></table>
> 
> On Firefox 2.0.0.7, this renders a 300x112 canvas element, but the containing
> table cell is 300x150 (instead of 300x112).
> On FIrefox 3.0a9pre (07-Oct-2007), this renders as expected with a 300x112
> canvas element.

Actually, on Firefox 3.0a9pre, this is a 300x150 canvas element.

Comment 9 David Kilzer (:ddkilzer) 2007-10-07 17:21:53 PDT
Created attachment 16582 [details]
Patch v1

Proposed patch.

I think I understand what's going on here (see Comment #7 and WebCore/ChangeLog entry), but I don't have MSIE 6/7 available to test its behavior compared to Safari's new behavior.

All layout tests pass that were passing before.  A ChangeLog entry, a new test, and updated test results are included in the patch.

Hyatt or Mitz should definitely review this.
Comment 10 David Kilzer (:ddkilzer) 2007-10-07 17:25:37 PDT
(In reply to comment #9)
> Safari's new behavior.

WebKit's new behavior.

Comment 11 David Kilzer (:ddkilzer) 2007-10-07 17:56:10 PDT
(In reply to comment #9)
> Created an attachment (id=16582) [edit]
> Patch v1

Urgh.  LayoutTests changes that may need to be fixed:

- Results for LayoutTests/fast/replaced/table-percent-height.html were placed in LayoutTests/platform/mac, but should be platform-independent.  (How do I check or generate results for Qt?)
- Need updated results for:
  * LayoutTests/platform/qt/fast/replaced/width100percent-image-expected.txt
  * LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug137388-1-expected.txt
  * LayoutTests/platform/qt/tables/mozilla_expected_failures/bugs/bug137388-2-expected.txt

Comment 12 Dave Hyatt 2007-10-08 10:29:33 PDT
What about form controls.... they need to be tested also (even though they use a different code path).
Comment 13 Maciej Stachowiak 2007-10-13 19:33:47 PDT
Comment on attachment 16582 [details]
Patch v1

The changes to fast/encoding/char-encoding.html seem unrelated. Please move those to a separate patch. Please also fix the test results as you described.

Otherwise, r=me. I think hyatt's concern about form controls can be adressed in a separate patch, since this patch would not regress their behavior and does fix images.

r=me
Comment 14 David Kilzer (:ddkilzer) 2007-10-25 08:15:34 PDT
Comment on attachment 16582 [details]
Patch v1

Changing mjs' review+ flag to review- since:

- I moved fast/encoding/char-encoding.html  to a different patch (which already landed).
- I want to write additional test cases for this patch (in separate test files per Comment #12).
- I need to update the test results per Comment #11.
Comment 15 David Kilzer (:ddkilzer) 2007-12-27 20:18:15 PST
Created attachment 18137 [details]
Patch v2

Changes since Patch v1:

- Added tests to LayoutTests/fast/replaced/table-percent-height.html:  embed, object, button, input (button, checkbox, file, image, password, radio, reset, submit, text), isindex, select, and textarea.

Note that with this patch results changed for:  canvas, embed, img, object, and input type="image".

- Removed changes to LayoutTests/fast/encoding/char-encoding.html and LayoutTests/fast/js/resources/js-test-post-function.js.

- Regenerated pixel test results.
Comment 16 mitz 2007-12-27 20:36:07 PST
Comment on attachment 18137 [details]
Patch v2

I wonder why you went through all the trouble of making a text-only test that uses getComputedStyle. Seems like the direct approach (using the render tree dump) would work just as well.
Comment 17 David Kilzer (:ddkilzer) 2007-12-27 20:46:29 PST
(In reply to comment #16)
> (From update of attachment 18137 [details] [edit])
> I wonder why you went through all the trouble of making a text-only test that
> uses getComputedStyle. Seems like the direct approach (using the render tree
> dump) would work just as well.

One less pixel test.  Can you dump the render tree without creating pixel test results?

Comment 18 Darin Adler 2007-12-29 14:26:14 PST
Comment on attachment 18137 [details]
Patch v2

I think this would probably read better if the max() expression was just inside the calcValue -- or maybe not, since there would be no place for the comment.

r=me
Comment 19 David Kilzer (:ddkilzer) 2007-12-30 09:06:38 PST
Committed revision 29039.

Comment 20 Alexey Proskuryakov 2010-06-11 17:22:05 PDT
*** Bug 5789 has been marked as a duplicate of this bug. ***
Comment 21 Arpita Bahuguna 2012-03-14 00:13:00 PDT
The fix for this issue renders the replaced elements inside the table cell properly. But now the table cell width is not getting set properly thereby causing an overlap with its adjacent table cell.

New bug added for the same: https://bugs.webkit.org/show_bug.cgi?id=81084