RESOLVED FIXED 106479
Available height should respect min and max height 
https://bugs.webkit.org/show_bug.cgi?id=106479
Summary Available height should respect min and max height
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
DetailsFormatted DiffDiff
Patch (6.47 KB, patch)
2013-01-09 14:12 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (6.22 KB, patch)
2013-01-12 04:39 PST, Robert Hogan 		no flags
DetailsFormatted DiffDiff
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.