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: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 14762 | ||||||||||||
Attachments: |
|
Created attachment 181970 [details]
Patch
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. Created attachment 181981 [details]
Patch
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. Created attachment 182467 [details]
Patch
Comment on attachment 182467 [details] Patch Clearing flags on attachment: 182467 Committed r139548: <http://trac.webkit.org/changeset/139548> All reviewed patches have been landed. Closing bug. |
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.