Bug 106479

Summary:

Available height should respect min and max height

Product:

WebKit

Reporter:

Robert Hogan <robert>

Component:

Layout and Rendering

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, ojan.autocc, robert, webkit.review.bot

Priority:

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

Robert Hogan

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

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Robert Hogan

Comment 1 2013-01-09 12:33:28 PST

Created attachment 181970 [details] Patch

Ojan Vafai

Comment 2 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.

Robert Hogan

Comment 3 2013-01-09 14:12:52 PST

Created attachment 181981 [details] Patch

Ojan Vafai

Comment 4 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.

Robert Hogan

Comment 5 2013-01-12 04:39:28 PST

Created attachment 182467 [details] Patch

WebKit Review Bot

Comment 6 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>

WebKit Review Bot

Comment 7 2013-01-12 05:01:23 PST

All reviewed patches have been landed. Closing bug.

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