Bug 15359

Summary:

JPEG image not shown when height is specified as percentage inside a table

Product:

WebKit

Reporter:

Ton Bäcker <ton.baecker>

Component:

Layout and Rendering

Assignee:

David Kilzer (:ddkilzer) <ddkilzer>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

arpitabahuguna, ddkilzer, dstorey, hyatt, mitz, roberto.avanzi

Priority:

Version:

419.x

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

29447

Bug Blocks:

58006

Attachments:

Description	Flags
code snippet	none
JPG file	none
Online test case	none
Hack v1	ddkilzer: review-
Patch v1	ddkilzer: review-
Patch v2	darin: review+

Ton Bäcker

Reported 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>

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Ton Bäcker

Comment 1 2007-10-03 05:56:58 PDT

Created attachment 16518 [details] code snippet

Ton Bäcker

Comment 2 2007-10-03 05:57:42 PDT

Created attachment 16519 [details] JPG file

David Kilzer (:ddkilzer)

Comment 3 2007-10-04 04:28:46 PDT

Created attachment 16530 [details] Online test case

David Kilzer (:ddkilzer)

Comment 4 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.

David Kilzer (:ddkilzer)

Comment 5 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.

David Kilzer (:ddkilzer)

Comment 6 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

David Kilzer (:ddkilzer)

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

David Kilzer (:ddkilzer)

Comment 8 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.

David Kilzer (:ddkilzer)

Comment 9 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.

David Kilzer (:ddkilzer)

Comment 10 2007-10-07 17:25:37 PDT

(In reply to comment #9) > Safari's new behavior. WebKit's new behavior.

David Kilzer (:ddkilzer)

Comment 11 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

Dave Hyatt

Comment 12 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).

Maciej Stachowiak

Comment 13 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

David Kilzer (:ddkilzer)

Comment 14 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.

David Kilzer (:ddkilzer)

Comment 15 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.

mitz

Comment 16 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.

David Kilzer (:ddkilzer)

Comment 17 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?

Darin Adler

Comment 18 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

David Kilzer (:ddkilzer)

Comment 19 2007-12-30 09:06:38 PST

Committed revision 29039.

Alexey Proskuryakov

Comment 20 2010-06-11 17:22:05 PDT

*** Bug 5789 has been marked as a duplicate of this bug. ***

Arpita Bahuguna

Comment 21 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

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