Bug 14762 - percentage top value of position:relative element not calculated using parent's min-height unless height set
Summary: percentage top value of position:relative element not calculated using parent...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC All
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 106479
Blocks: 103812 113077
  Show dependency treegraph
 
Reported: 2007-07-25 03:06 PDT by Andrew Stibbard
Modified: 2013-04-24 09:32 PDT (History)
11 users (show)

See Also:


Attachments
Reduced testcase demonstrating min-height and relative top issues (1.89 KB, text/html)
2007-07-25 03:10 PDT, Andrew Stibbard
no flags Details
Patch (14.10 KB, patch)
2013-01-23 13:30 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.41 KB, patch)
2013-02-07 11:51 PST, Robert Hogan
jchaffraix: review+
jchaffraix: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Stibbard 2007-07-25 03:06:38 PDT
A position:relative element with a percentage top value is calculated incorrectly if its parent only has min-height set. Applying a non-percentage height to the parent, or changing top to px fixes the calc.

Tested on build 522.13.1. Firefox 2 also has this issue. Please see Bug 14749 for a related issue with percentages.

Testcase to follow.
Comment 1 Andrew Stibbard 2007-07-25 03:10:07 PDT
Created attachment 15679 [details]
Reduced testcase demonstrating min-height and relative top issues

Testcase demonstrating issues with min-height and percentage top positioning.
Comment 2 David Kilzer (:ddkilzer) 2007-07-25 05:50:42 PDT
(In reply to comment #1)
> Created an attachment (id=15679) [edit]
> Reduced testcase demonstrating min-height and relative top issues

The first two boxes and the last box should be 50% green on top, and 50% green on bottom.  (The larger one in the middle is green/blue/green, but not evenly distributed.)

Opera 9.21 apparently gets all of these test cases correct.

This is not a regression as Safari 2.0.4 (419.3) with original WebKit on Mac OS X 10.4.10 (8R218) behaves the same way.  Tested with a local debug build of WebKit r24586 with Safari 3 Public Beta v. 3.0.2 (522.12) on 10.4.10.

Comment 3 Robert Hogan 2013-01-23 13:30:16 PST
Created attachment 184298 [details]
Patch
Comment 4 WebKit Review Bot 2013-01-23 14:54:59 PST
Comment on attachment 184298 [details]
Patch

Attachment 184298 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16084301

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment 5 Julien Chaffraix 2013-01-31 17:17:37 PST
Comment on attachment 184298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184298&action=review

The change makes sense, some comments. There were some questions in bug 103812 that may be good to clear out here too.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:394
> +    while (cb->isAnonymousBlock() && !cb->isTableCell())
> +        cb = cb->containingBlock();

If you hit an anonymous cell, you are done as it *cannot* have anything other that the initial values for logical height, top and bottom.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:396
> +    if (!cb->style()->logicalHeight().isAuto() || (!cb->style()->top().isAuto() && !cb->style()->bottom().isAuto()))

This line looks wrong (and was wrong before you moved it here) as we check the *logical* height but the *physical* top and bottom.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:428
> -        && (!containingBlock->style()->height().isAuto()
> +        && (!containingBlock->hasAutoHeightOrContainingBlockWithAutoHeight()

Note that this replaced height() with logicalHeight() in this check which means that we should probably have a test for vertical writing modes.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:434
> -        && (!containingBlock->style()->height().isAuto()
> +        && (!containingBlock->hasAutoHeightOrContainingBlockWithAutoHeight()

Ditto.

> LayoutTests/fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor.html:13
> +    <!-- In strict mode, an image with height set to 100% and a fixed height ancestor should expand to its instrinsic height and width. -->
> +    <!-- Percentage height "is calculated with respect to the height of the generated box's containing block." --> 

This is a super useful explanation to dump in your output.
Comment 6 Robert Hogan 2013-02-07 11:51:31 PST
Created attachment 187140 [details]
Patch
Comment 7 Julien Chaffraix 2013-02-14 13:01:59 PST
Comment on attachment 187140 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=187140&action=review

> LayoutTests/ChangeLog:13
> +        * fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor-expected.html: Added.
> +        * fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor.html: Added.
> +        * fast/replaced/computed-image-width-with-percent-height-inside-table-cell-and-fixed-ancestor-expected.html: Added.
> +        * fast/replaced/computed-image-width-with-percent-height-inside-table-cell-and-fixed-ancestor.html: Added.

Could it be possible to have vertical-writing mode version of these tests?

> LayoutTests/fast/block/percent-top-parent-respects-min-height.html:39
> +        <script>
> +            checkLayout('.box')
> +        </script>

You can put that on an onload listener:

<body onload="checkLayout('.box')">

> LayoutTests/fast/replaced/computed-image-width-with-percent-height-and-fixed-ancestor-expected.html:19
> +

Stray empty line (present on all the following files but not repeated).
Comment 8 Robert Hogan 2013-02-16 05:08:00 PST
Committed r143102: <http://trac.webkit.org/changeset/143102>
Comment 9 Alexey Proskuryakov 2013-04-24 09:32:15 PDT
This caused bug 113077, bug 113526 and bug 115090.