Bug 106479

Summary: Available height should respect min and max height
Product: WebKit Reporter: Robert Hogan <robert>
Component: Layout and RenderingAssignee: Robert Hogan <robert>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, ojan.autocc, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 14762    
Attachments:
Description Flags
Reduction
none
Patch
none
Patch
none
Patch none

Description Robert Hogan 2013-01-09 11:50:34 PST
Created attachment 181963 [details]
Reduction

When calculating a relative positioned block's offset as a percentage of its container, respect the min and max height set on the container.
Comment 1 Robert Hogan 2013-01-09 12:33:28 PST
Created attachment 181970 [details]
Patch
Comment 2 Ojan Vafai 2013-01-09 13:19:50 PST
Comment on attachment 181970 [details]
Patch

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

The C++ side is fine. Please cleanup the tests and then I'm happy to r+.

> LayoutTests/fast/block/percent-top-respects-max-height.html:1
> +<!DOCTYPE html>

Can you fix up the indentation so that it's consistent and 4 spaces?

> LayoutTests/fast/block/percent-top-respects-max-height.html:4
> +		<style type="text/css">

nit: no need to include the type here.

> LayoutTests/fast/block/percent-top-respects-max-height.html:29
> +			.container	{
> +				width:	250px;
> +				background:	green;
> +				margin-right:	20px;
> +                position: relative;
> +			}
> +			.box	{
> +				height:	100px;
> +				position:	relative;
> +				top:	50%;
> +				background:	blue;
> +			}
> +			.ref	{
> +				height:	100px;
> +				width:	250px;
> +				position: absolute;
> +				top:	150px;
> +				background:	red;
> +			}
> +			.hasheight	{
> +				height:	600px;
> +			}
> +			.hassmallermaxheight	{
> +				max-height:	300px;
> +			}

Can you reduce this to the minimal CSS necessary? e.g. do you need the margin-right or the position: relative on .box? Do you need any of these widths?

> LayoutTests/fast/block/percent-top-respects-max-height.html:40
> +		<div class="container hasheight hassmallermaxheight">

Can you just make these all one class?

> LayoutTests/fast/block/percent-top-respects-max-height.html:46
> +            if (document.getElementById("box").offsetTop == 150)

Can you make this a check-layout.js test instead? Then you won't need custom code for this and the failure output is more helpful.

> LayoutTests/fast/block/percent-top-respects-min-height.html:1
> +<!DOCTYPE html>

Ditto all the comments for the above test. Or, you could just test both cases in a single test.
Comment 3 Robert Hogan 2013-01-09 14:12:52 PST
Created attachment 181981 [details]
Patch
Comment 4 Ojan Vafai 2013-01-09 14:24:02 PST
Comment on attachment 181981 [details]
Patch

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

> LayoutTests/fast/block/percent-top-respects-max-height.html:23
> +            .container    {
> +                background:    green;
> +                position: relative;
> +                height:    600px;
> +                max-height:    300px;
> +            }
> +            .box    {
> +                height:    100px;
> +                position:    relative;
> +                top:    50%;
> +                background:    blue;
> +            }
> +            .ref    {
> +                height:    100px;
> +                position: absolute;
> +                top:    150px;
> +                background:    red;
> +            }

Nit: typical style in most webkit tests is to put a single space between the selector and the curly brace and a single space between the property name and property value.

> LayoutTests/fast/block/percent-top-respects-max-height.html:32
> +            <div id="ref" class="ref"></div>
> +            <div id="box" class="box"></div>

With the changes below, you don't need either of these to have IDs.

> LayoutTests/fast/block/percent-top-respects-max-height.html:35
> +            document.getElementById("box").setAttribute('data-total-y', 150);

No need to set this in script. You can just set the attribute on the div's markup directly.

> LayoutTests/fast/block/percent-top-respects-max-height.html:36
> +            checkLayout('body > div > div')

How about:
checkLayout('.box');

Then you won't get two "PASS" lines when there's only one assert.

> LayoutTests/fast/block/percent-top-respects-min-height.html:1
> +<!DOCTYPE html>

Ditto the above comments.
Comment 5 Robert Hogan 2013-01-12 04:39:28 PST
Created attachment 182467 [details]
Patch
Comment 6 WebKit Review Bot 2013-01-12 05:01:20 PST
Comment on attachment 182467 [details]
Patch

Clearing flags on attachment: 182467

Committed r139548: <http://trac.webkit.org/changeset/139548>
Comment 7 WebKit Review Bot 2013-01-12 05:01:23 PST
All reviewed patches have been landed.  Closing bug.