RESOLVED FIXED 103812
Only examine the containing block when computing the used height of a replaced element with percentage height 
https://bugs.webkit.org/show_bug.cgi?id=103812
Summary Only examine the containing block when computing the used height of a replace...
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
DetailsFormatted DiffDiff
Patch (7.92 KB, patch)
2012-12-10 13:05 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
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.