| Summary: | REGRESSION (r88913): Preview in Safari’s snippet editor has a fixed height instead of filling the entire pane | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | mitz | ||||||||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Major | CC: | dglazkov, hyatt, krit, rwlbuis, simon.fraser, thestig, webkit.review.bot, zimmermann | ||||||||||||||||||||||
| Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
Created attachment 99929 [details]
Reduction
Here’s a reduction.
Dan, thanks for the report!
I've checked the testcase and know what part of my patch is responsible for the difference:
int RenderReplaced::computeReplacedLogicalHeight() const
{
// 10.5 Content height: the 'height' property: http://www.w3.org/TR/CSS21/visudet.html#propdef-height
// 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'.
bool heightIsAuto = style()->logicalHeight().isAuto();
if (!document()->inQuirksMode() && !isPositioned() && style()->logicalHeight().isPercent()) {
if (RenderObject* containingBlock = this->containingBlock()) {
while (containingBlock->isAnonymous())
containingBlock = containingBlock->containingBlock();
heightIsAuto = !containingBlock->style()->logicalHeight().isSpecified();
}
}
This piece of code is new, and closely related to the recently reported regression at bug 62769.
<!DOCTYPE html>
<div style="background-color: red; position: absolute; left: 10px; right: 10px; top: 10px; bottom: 10px;">
<iframe style="border: none; background-color: green; width: 100%; height: 100%;"></iframe>
</div>
When the <iframe> height is determined in RenderReplaced::computeReplacedLogicalHeight, we're finding out that the <iframe> is not positioned and has a percentage height in your testcase.
Though as the containingBlock doesn't have an explicit height property set it will assume height is auto.
Thinking about this, I probably interpreted "If the height of the containing block is not specified explicitly" wrong, in the following braces it says "(i.e., it depends on content height)".
The outer <div> doesn't explicitely set height, though it's implicitely defined, and the outer <div> size does _not_ depend on the iframe size, so I've clearly introduced a bug there.
Just checking "containingBlock->style()->logicalHeight().isSpecified()" is not enough.
Dan, do you have a suggestion what's the best way to determine whether the size of the containingBlock is dependant on the content?
This is closely related to bug 62769.
I believe you’re looking for logic along the lines of what RenderBox::computePercentageLogicalHeight() does, namely:
// A positioned element that specified both top/bottom or that specifies height should be treated as though it has a height
// explicitly specified that can be used for any percentage computations.
// FIXME: We can't just check top/bottom here.
// https://bugs.webkit.org/show_bug.cgi?id=46500
bool isPositionedWithSpecifiedHeight = cb->isPositioned() && (!cb->style()->logicalHeight().isAuto() || (!cb->style()->top().isAuto() && !cb->style()->bottom().isAuto()));
(In reply to comment #4) > I believe you’re looking for logic along the lines of what RenderBox::computePercentageLogicalHeight() does, namely: > > > // A positioned element that specified both top/bottom or that specifies height should be treated as though it has a height > // explicitly specified that can be used for any percentage computations. > // FIXME: We can't just check top/bottom here. > // https://bugs.webkit.org/show_bug.cgi?id=46500 > bool isPositionedWithSpecifiedHeight = cb->isPositioned() && (!cb->style()->logicalHeight().isAuto() || (!cb->style()->top().isAuto() && !cb->style()->bottom().isAuto())); Superb! I'm going to prepare a patch! Created attachment 100352 [details]
Patch
Added Dans testcase within the patch. Would be great if you could have a look...
Comment on attachment 100352 [details] Patch Attachment 100352 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9006990 New failing tests: css2.1/20110323/block-replaced-height-007.htm css2.1/20110323/float-replaced-height-007.htm css2.1/20110323/inline-replaced-height-007.htm css2.1/20110323/inline-block-replaced-height-007.htm Created attachment 100355 [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
Created attachment 100451 [details]
Patch v2
Oops, fix regressions of the css2.1 *intrinsic*-007* tests. Fixed the spec quotations: the paragraph I quoted only applies to percentage heights, I forgot about that as the comment wasn't complete.
*** Bug 62769 has been marked as a duplicate of this bug. *** Comment on attachment 100451 [details] Patch v2 Attachment 100451 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9022089 New failing tests: http/tests/misc/slow-loading-mask.html tables/mozilla/bugs/bug50695-2.html http/tests/local/drag-over-remote-content.html tables/mozilla/bugs/bug131020.html fast/blockflow/japanese-rl-selection.html fast/backgrounds/background-leakage.html tables/mozilla/bugs/bug137388-3.html fast/backgrounds/repeat/negative-offset-repeat.html svg/W3C-SVG-1.1/struct-use-01-t.svg tables/mozilla/bugs/bug137388-2.html fast/box-shadow/scaled-box-shadow.html tables/mozilla/bugs/bug137388-1.html svg/transforms/text-with-mask-with-svg-transform.svg transforms/2d/hindi-rotated.html svg/repaint/filter-repaint.svg fast/blockflow/japanese-lr-selection.html fast/replaced/percent-height-in-anonymous-block-in-table.html Created attachment 100454 [details]
Archive of layout-test-results from ec2-cr-linux-01
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 100802 [details]
Patch v3
Next try...
Created attachment 100815 [details]
Patch v4
Created attachment 100816 [details]
Patch v5
Oops, forgot WebCore.
Comment on attachment 100816 [details] Patch v5 Attachment 100816 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9058357 New failing tests: tables/mozilla/bugs/bug50695-2.html http/tests/local/drag-over-remote-content.html fast/css/replaced-element-implicit-size.html Created attachment 100832 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #16) The patch still has a regression in bug5096-2.html, the rest is fine. > New failing tests: > tables/mozilla/bugs/bug50695-2.html > http/tests/local/drag-over-remote-content.html > fast/css/replaced-element-implicit-size.html <html> <body> <form> <iframe src="../images/raptor.jpg" height=50%></iframe> </form> </body> </html> This <iframe> receives a height of 150, instead of 50% of the whole document size, as the expected.png suggests. I'll look into that. Created attachment 100976 [details]
Patch v6
Great, all regressions have been resolved. Dan/David/Simon can you have a look? Comment on attachment 100976 [details] Patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review > LayoutTests/ChangeLog:18 > + * platform/mac/svg/zoom/page/zoom-svg-through-object-with-text-expected.png: > + * platform/mac/svg/zoom/page/zoom-svg-through-object-with-text-expected.txt: Does this test have to be so tiny? seem like pixel changes will often fall under the tolerance threshold. (In reply to comment #21) > (From update of attachment 100976 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review > > > LayoutTests/ChangeLog:18 > > + * platform/mac/svg/zoom/page/zoom-svg-through-object-with-text-expected.png: > > + * platform/mac/svg/zoom/page/zoom-svg-through-object-with-text-expected.txt: > > Does this test have to be so tiny? seem like pixel changes will often fall under the tolerance threshold. Note, this test doesn't work atm, <!-- FIXME: This is broken, text doesn't correctly zoom, its font size remains the same upon scaling --> It's supposed to be bigger - I plan to look at this in a separated bug report. Comment on attachment 100976 [details] Patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review Looks fine overall, r- because of my question. > Source/WebCore/ChangeLog:16 > + wheter the height property is set on the containing block, there are other ways to implicitly specify wheter -> whether > Source/WebCore/rendering/RenderReplaced.cpp:323 > + return false; Can you tell me how the above while relates to this for? For instance where does the isTableCell check come from? (In reply to comment #23) > (From update of attachment 100976 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review > > Looks fine overall, r- because of my question. > > > Source/WebCore/ChangeLog:16 > > + wheter the height property is set on the containing block, there are other ways to implicitly specify Fixed. > > wheter -> whether > > > Source/WebCore/rendering/RenderReplaced.cpp:323 > > + return false; > > Can you tell me how the above while relates to this for? For instance where does the isTableCell check come from? Short version: if you leave out the table cell check you'll see regressions. Long version: The code is inspired by RenderBlock::isSelfCollapsingBlock, which contains similar (but not identical) logic to determine whether a block has auto height or not: Length logicalHeightLength = style()->logicalHeight(); bool hasAutoHeight = logicalHeightLength.isAuto(); if (logicalHeightLength.isPercent() && !document()->inQuirksMode()) { hasAutoHeight = true; for (RenderBlock* cb = containingBlock(); !cb->isRenderView(); cb = cb->containingBlock()) { if (cb->style()->logicalHeight().isFixed() || cb->isTableCell()) hasAutoHeight = false; } } I'm determining whether a replaced object has auto height or not, and can't reuse the code as we have have to take more things into account for replaced elements, as indicated in my comment: // For percentage heights: The percentage is calculated with respect to the height of the generated box's // containing block. 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'. Hope that resolves the question? Comment on attachment 100976 [details] Patch v6 View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review r+ after the explanation, and assuming no regressions? Have a look before landing that the if conditions are ordered according to the chance they are hit, so if one of them is more likely to occur than the others, test it first. >>> Source/WebCore/ChangeLog:16 >>> + wheter the height property is set on the containing block, there are other ways to implicitly specify >> >> wheter -> whether > > Fixed. wheter -> whether >>> Source/WebCore/rendering/RenderReplaced.cpp:323 >>> + return false; >> >> Can you tell me how the above while relates to this for? For instance where does the isTableCell check come from? > > Short version: if you leave out the table cell check you'll see regressions. > > Long version: The code is inspired by RenderBlock::isSelfCollapsingBlock, which contains similar (but not identical) logic to determine whether a block has auto height or not: > > Length logicalHeightLength = style()->logicalHeight(); > bool hasAutoHeight = logicalHeightLength.isAuto(); > if (logicalHeightLength.isPercent() && !document()->inQuirksMode()) { > hasAutoHeight = true; > for (RenderBlock* cb = containingBlock(); !cb->isRenderView(); cb = cb->containingBlock()) { > if (cb->style()->logicalHeight().isFixed() || cb->isTableCell()) > hasAutoHeight = false; > } > } > > I'm determining whether a replaced object has auto height or not, and can't reuse the code as we have have to take more things into account for replaced elements, as indicated in my comment: > > // For percentage heights: The percentage is calculated with respect to the height of the generated box's > // containing block. 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'. > > Hope that resolves the question? Can you tell me how the above while relates to this for? For instance where does the isTableCell check come from? (In reply to comment #25) > (From update of attachment 100976 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=100976&action=review > > r+ after the explanation, and assuming no regressions? Have a look before landing that the if conditions are ordered according to the chance they are hit, so if one of them is more likely to occur than the others, test it first. I already did that! Thanks for the review. There are no more regressions and this bug and the other regression from r88913 is fixed. cr-linux-ews which runs pixel tests is also green! > > Hope that resolves the question? > > Can you tell me how the above while relates to this for? For instance where does the isTableCell check come from? Heh, sorry, I left the main part of your question unanswered, was a bit late for me... When we're determining the inline replaced height in RenderReplaced, we have to check whether our containing block specifies a height. That's done by traversing the containing block hierarchy. If any our intermediate parents is a RenderTableCell, we have to skip it as it's height value is unset - as for them the table manages the cell heights (see comment below): void RenderBox::computeLogicalHeight() { // Cell height is managed by the table and inline non-replaced elements do not support a height property. if (isTableCell() || (isInline() && !isReplaced())) return; ... Took me a while as well to dig into it :-) Comment on attachment 100976 [details] Patch v6 Landed in r91242. Thanks! (In reply to comment #28) > Thanks! You're welcome! I hope now that no regressions are left in that area, I can continue moving RenderImage to the new size-negotiation logic. object/iframe/embed/.. all properly handle intrinsic size negotiation now, images are left todo (and background-image/border-image/etc. as well). I have an already working SVGImage-size-negotiation-rewrite patch, with some regressions left, that implement intrinsic size negotiation for svg-as-<html:image>, svg-as-border-image/background-image, etc. - marking the end of "include svg by reference" implementation journey (all ways to include an external SVG are tested in terms of size negotiation and work great :-) This leaves "only" inline SVG+XHTML which still needs a lot of work.. |
Steps to reproduce: 1. In Safari, choose Develop > Show Snippet Editor 2. If necessary, resize the bottom pane of the snippet editor to be taller than 150 pixels 3. In the top pane of the snippet editor, type <div style="border: solid; height: 200px;"> Notice that the bottom of the div is clipped. You can scroll it within a 150px-tall area.