Bug 64059

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:

Keywords:

HasReduction, InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Reduction	none
Patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
Patch v2	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01	none
Patch v3	none
Patch v4	none
Patch v5	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-02	none
Patch v6	rwlbuis: review+

mitz

Reported 2011-07-06 21:30:00 PDT

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.

Attachments
Reduction (226 bytes, text/html) 2011-07-06 21:40 PDT, mitz	no flags	Details
Patch (23.89 KB, patch) 2011-07-11 12:54 PDT, Nikolas Zimmermann	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.42 MB, application/zip) 2011-07-11 13:14 PDT, WebKit Review Bot	no flags	Details
Patch v2 (24.15 KB, patch) 2011-07-12 02:02 PDT, Nikolas Zimmermann	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.98 MB, application/zip) 2011-07-12 02:20 PDT, WebKit Review Bot	no flags	Details
Patch v3 (41.92 KB, patch) 2011-07-14 06:48 PDT, Nikolas Zimmermann	no flags	Details Formatted Diff Diff
Patch v4 (37.67 KB, patch) 2011-07-14 09:19 PDT, Nikolas Zimmermann	no flags	Details Formatted Diff Diff
Patch v5 (41.93 KB, patch) 2011-07-14 09:35 PDT, Nikolas Zimmermann	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.89 MB, application/zip) 2011-07-14 11:18 PDT, WebKit Review Bot	no flags	Details
Patch v6 (43.10 KB, patch) 2011-07-15 07:51 PDT, Nikolas Zimmermann	rwlbuis: review+	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

mitz

Comment 1 2011-07-06 21:40:50 PDT

Created attachment 99929 [details] Reduction Here’s a reduction.

mitz

Comment 2 2011-07-06 21:41:24 PDT

<rdar://problem/9735191>

Nikolas Zimmermann

Comment 3 2011-07-07 01:48:41 PDT

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.

mitz

Comment 4 2011-07-07 09:19:00 PDT

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()));

Nikolas Zimmermann

Comment 5 2011-07-08 04:44:16 PDT

(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!

Nikolas Zimmermann

Comment 6 2011-07-11 12:54:54 PDT

Created attachment 100352 [details] Patch Added Dans testcase within the patch. Would be great if you could have a look...

WebKit Review Bot

Comment 7 2011-07-11 13:14:11 PDT

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

WebKit Review Bot

Comment 8 2011-07-11 13:14:15 PDT

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

Nikolas Zimmermann

Comment 9 2011-07-12 02:02:39 PDT

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.

Nikolas Zimmermann

Comment 10 2011-07-12 02:06:53 PDT

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

WebKit Review Bot

Comment 11 2011-07-12 02:20:43 PDT

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

WebKit Review Bot

Comment 12 2011-07-12 02:20:48 PDT

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

Nikolas Zimmermann

Comment 13 2011-07-14 06:48:57 PDT

Created attachment 100802 [details] Patch v3 Next try...

Nikolas Zimmermann

Comment 14 2011-07-14 09:19:44 PDT

Created attachment 100815 [details] Patch v4

Nikolas Zimmermann

Comment 15 2011-07-14 09:35:21 PDT

Created attachment 100816 [details] Patch v5 Oops, forgot WebCore.

WebKit Review Bot

Comment 16 2011-07-14 11:17:53 PDT

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

WebKit Review Bot

Comment 17 2011-07-14 11:18:00 PDT

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

Nikolas Zimmermann

Comment 18 2011-07-15 03:37:38 PDT

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

Nikolas Zimmermann

Comment 19 2011-07-15 07:51:06 PDT

Created attachment 100976 [details] Patch v6

Nikolas Zimmermann

Comment 20 2011-07-15 08:28:14 PDT

Great, all regressions have been resolved. Dan/David/Simon can you have a look?

Simon Fraser (smfr)

Comment 21 2011-07-15 08:33:18 PDT

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.

Nikolas Zimmermann

Comment 22 2011-07-15 08:37:34 PDT

(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,  It's supposed to be bigger - I plan to look at this in a separated bug report.

Rob Buis

Comment 23 2011-07-17 07:21:56 PDT

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?

Nikolas Zimmermann

Comment 24 2011-07-18 05:49:56 PDT

(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?

Rob Buis

Comment 25 2011-07-18 07:52:51 PDT

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?

Nikolas Zimmermann

Comment 26 2011-07-19 00:58:49 PDT

(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 :-)

Nikolas Zimmermann

Comment 27 2011-07-19 01:13:25 PDT

Comment on attachment 100976 [details] Patch v6 Landed in r91242.

mitz

Comment 28 2011-07-19 01:25:07 PDT

Thanks!

Nikolas Zimmermann

Comment 29 2011-07-19 01:32:47 PDT

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

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