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

Tue Aug 6 07:29:35 PDT 2013

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

--- Comment #14 from Anton Obzhirov <a.obzhirov at samsung.com>  2013-08-06 07:29:15 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.

ok

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

ok

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

ok

>> 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; }

yes, thanks for pointing it - made a mistake below as well.

>> LayoutTests/fast/regions/region-min-max-width-support.html:37
>> +<body font-size: 20px; font-family: ahem;>
> 
> Are these style declarations for body used?

made a mistake here - forgot "style=""".

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

ok

>> LayoutTests/fast/regions/region-min-max-width-support.html:78
>> +document.body.offsetTop; // force layout
> 
> Why do you need to force layout here?

it was copy paste from other test - forgot to remove.

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

Yep, will look better

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

ok

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