[Webkit-unassigned] [Bug 115248] [CSSRegions] Min-width and max-width for a region should support values other than length

Tue Aug 6 07:06:37 PDT 2013

https://bugs.webkit.org/show_bug.cgi?id=115248

--- Comment #12 from Mihnea Ovidenie <mihnea at adobe.com>  2013-08-06 07:06:17 PST ---
(From update of attachment 208126)
View in context: https://bugs.webkit.org/attachment.cgi?id=208126&action=review

> LayoutTests/ChangeLog:3
> +        [CSSRegions] Min/max-width should support values other than length

(This is an informal review)
I think you should have a new pass on this test to clean it up a bit. Please note that I changed the bug description to make it more clear.

> LayoutTests/fast/regions/region-min-max-width-support.html:1
> +<!doctype html>

doctype -> DOCTYPE

> LayoutTests/fast/regions/region-min-max-width-support.html:5
> +<style>

You can reduce the style declarations in the (possible) following way:

.box { height: 20px; border: 1px solid black; position: absolute; left: 200px; }
.region { -webkit-flow-from: flow; }
.reference { background-color: red; }

#region1 { max-width: 1%;  top: 100px;}
#reference1 { max-width: 1%; top: 100px; }

and later you can use the above classes when setting the properties for regions and their corresponding reference elements:
<div id="reference1" class="box reference">region1</div>
<div id="region1" class="box region"></div>

> LayoutTests/fast/regions/region-min-max-width-support.html:6
> +    #content { -webkit-flow-into: flow; background-color: green;}

If you want to avoid seeing the red for the last region, you need to ensure that height of each slice of content flowed in each region has 20px. You can do that in the following way:
body { font: 16px/1.25 monospace; }

> LayoutTests/fast/regions/region-min-max-width-support.html:37
> +<body font-size: 20px; font-family: ahem;>

Are these style declarations for body used?

> LayoutTests/fast/regions/region-min-max-width-support.html:38
> +<p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=115248"> [CSSRegions] Min/max-width should support values other than length</a>.</p>

Please update the test with the new bug title. Also, it would be nice if you could add below something like:
<p>On success you should see PASS below</p>

> LayoutTests/fast/regions/region-min-max-width-support.html:78
> +document.body.offsetTop; // force layout

Why do you need to force layout here?

> LayoutTests/fast/regions/region-min-max-width-support.html:79
> +document.getElementById("region1").setAttribute("data-expected-width", document.getElementById('reference1').offsetWidth);

All the below statements can be written in a for loop.

> LayoutTests/fast/regions/region-min-max-width-support.html:91
> +checkLayout('#region1');

You can use something like this instead of repeating all the checkLayout statements:
<body onload="checkLayout('.region')"> should you want to use a class for the region elements.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.