Bug 103812

Summary:

Only examine the containing block when computing the used height of a replaced element with percentage height

Product:

WebKit

Reporter:

Robert Hogan <robert>

Component:

CSS

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, jchaffraix, krit, ojan.autocc, ojan, robert, schenney, webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

14762, 28035

Bug Blocks:

Attachments:

Description	Flags
Reduction	none
Patch	none
Patch	none

Robert Hogan

Reported 2012-12-01 04:47:53 PST

Created attachment 177103 [details] Reduction .

Attachments
Reduction (710 bytes, text/html) 2012-12-01 04:47 PST, Robert Hogan	no flags	Details
Patch (4.86 KB, patch) 2012-12-02 14:07 PST, Robert Hogan	no flags	Details Formatted Diff Diff
Patch (7.92 KB, patch) 2012-12-10 13:05 PST, Robert Hogan	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Robert Hogan

Comment 1 2012-12-02 14:07:52 PST

Created attachment 177151 [details] Patch

Robert Hogan

Comment 2 2012-12-10 10:47:57 PST

Comment on attachment 177151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177151&action=review > Source/WebCore/ChangeLog:10 > + Percentage height "is calculated with respect to the height of the generated box's containing block." So when > + calculating the used height of a replaced element do not crawl through ancestor blocks except when traversing > + anonymous blocks. Should probably add the reference here: http://www.w3.org/TR/CSS21/visudet.html#the-height-property and also the following sentence from that section: "If the height of the containing block is not specified explicitly (i.e., it depends on content height), and this element is not absolutely positioned, the value computes to 'auto'. " Needless to say this is how the other browsers do it too: http://roberthogan.net/css/computed-image-width-with-percent-height-and-fixed-ancestor.html

Julien Chaffraix

Comment 3 2012-12-10 11:27:32 PST

Comment on attachment 177151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177151&action=review > Source/WebCore/rendering/RenderReplaced.cpp:249 > + while (cb->isAnonymousBlock()) > + cb = cb->containingBlock(); I don't think this is right: an image inside an anonymous table cell will return the wrong result. Example code: <div style="display: table; height: auto"> <img src="resources/square-blue-100x100.png"> </div> This used to return false as we would consider the anonymous table cell (which is the image's containing block) but now we will go to the table. > LayoutTests/fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor-expected.html:1 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> Can we use the HTML5 doctype instead? > LayoutTests/fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor-expected.html:9 > + The square below should be 100px by 100px. Bug number + title. > LayoutTests/fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor.html:5 > + img { height:100%; } Space after the colon.

Robert Hogan

Comment 4 2012-12-10 12:25:04 PST

Comment on attachment 177151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177151&action=review >> Source/WebCore/rendering/RenderReplaced.cpp:249 >> + cb = cb->containingBlock(); > > I don't think this is right: an image inside an anonymous table cell will return the wrong result. > > Example code: > <div style="display: table; height: auto"> > <img src="resources/square-blue-100x100.png"> > </div> > > This used to return false as we would consider the anonymous table cell (which is the image's containing block) but now we will go to the table. Great spot, but interestingly our result for that test is wrong now though and this patch doesn't change that! There's no good reason I can see in the spec for a containing block which is a table cell to say that it does not have an auto height. This was introduced in http://trac.webkit.org/changeset/91242

Robert Hogan

Comment 5 2012-12-10 12:27:28 PST

Hi Nikolas, cc'ing you on this. Why does hasAutoHeightOrContainingBlockWithAutoHeight() special case table cells? This causes it to mis-render the case pointed out by Julien: <div style="display: table; height: auto"> <img src="resources/square-blue-100x100.png"> </div>

Robert Hogan

Comment 6 2012-12-10 13:05:26 PST

Created attachment 178613 [details] Patch

Robert Hogan

Comment 7 2012-12-22 02:19:30 PST

Hi Nikolas - Can you take a look at this and comment #5?

Robert Hogan

Comment 8 2013-01-24 10:55:25 PST

Comment on attachment 178613 [details] Patch Taking this to bug 14762 instead.

Stephen Chenney

Comment 9 2013-01-28 15:05:57 PST

(In reply to comment #5) > Hi Nikolas, > > cc'ing you on this. Why does hasAutoHeightOrContainingBlockWithAutoHeight() special case table cells? This causes it to mis-render the case pointed out by Julien: > > <div style="display: table; height: auto"> > <img src="resources/square-blue-100x100.png"> > </div> Channeling Niko, I suspect that either (a) the table cell is treated specially in an attempt to handle svg content in table cells, where the SVG really wants a size in order to lay out (but we're broken in this respect anyway) or (b) because it's just an error that got through. Table layout is not Niko's thing. I would not place too much weight on the change he made in this regard. That is, defer to Julien.

Dirk Schulze

Comment 10 2013-01-29 08:22:18 PST

(In reply to comment #9) > (In reply to comment #5) > > Hi Nikolas, > > > > cc'ing you on this. Why does hasAutoHeightOrContainingBlockWithAutoHeight() special case table cells? This causes it to mis-render the case pointed out by Julien: > > > > <div style="display: table; height: auto"> > > <img src="resources/square-blue-100x100.png"> > > </div> > > Channeling Niko, I suspect that either (a) the table cell is treated specially in an attempt to handle svg content in table cells, where the SVG really wants a size in order to lay out (but we're broken in this respect anyway) or (b) because it's just an error that got through. > > Table layout is not Niko's thing. I would not place too much weight on the change he made in this regard. That is, defer to Julien. I think it was (a), but not sure if I recover it correctly. We probably have a test for it, if not, please write a test to check the behavior <div style="display: table; height: auto"> <svg><rect width="100%" height="100%" fill="green"/></svg> </div> Should be enough.

Robert Hogan

Comment 11 2013-02-21 11:21:33 PST

Resolved by 28035 and 14762

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