Bug 106479 - Available height should respect min and max height
Summary: Available height should respect min and max height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on:
Blocks: 14762
  Show dependency treegraph
 
Reported: 2013-01-09 11:50 PST by Robert Hogan
Modified: 2013-01-12 05:01 PST (History)
4 users (show)

See Also:


Attachments
Reduction (750 bytes, text/html)
2013-01-09 11:50 PST, Robert Hogan
no flags Details
Patch (6.46 KB, patch)
2013-01-09 12:33 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (6.47 KB, patch)
2013-01-09 14:12 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (6.22 KB, patch)
2013-01-12 04:39 PST, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.