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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihnea Ovidenie]

Right now, code in RenderRegion::computePreferredLogicalWidths supports only length values. It should support other values like: percentage, calc or viewport relative

[Anton Obzhirov]

I am going to try fixing this one.

[Anton Obzhirov]

After playing with css region width, min-width, max-width properties it seems that the region width is calculated properly for other cases as well (percent, calculated, viewport relative, ...). I will double check it and add a new test.

[Chris]

Seems that  
  
min-width: xx%;  
  
doesn't work when  
  
display: table;  
or  
display: table-cell;  
  
is set on the element, or parent element.  
  
See http://www.martialartworld.co.uk/ for an example - the top menu items *should* be equal widths (min-width: 14%), but the percentage is ignored.  
  
Tested in Chrome 27.0.1453.116 and Chrome 30.0.1559.0 canary.  
  
[note: http://www.martialartworld.co.uk/ is under development at the moment, but I've opened it up to viewing to demonstrate this issue, so the CSS is in a bit of a mess :-(  ]  
  
Compare the menu viewed using Chrome to how it looks in Firefox 22.0 (latest release as of this date).

[Mihnea Ovidenie]

(In reply to comment #3)
> Seems that  
> 
> min-width: xx%;  
> 
> doesn't work when  
> 
> display: table;  
> or  
> display: table-cell;  
> 
> is set on the element, or parent element.  
> 
> See http://www.martialartworld.co.uk/ for an example - the top menu items *should* be equal widths (min-width: 14%), but the percentage is ignored.  
> 
> Tested in Chrome 27.0.1453.116 and Chrome 30.0.1559.0 canary.  
> 
> [note: http://www.martialartworld.co.uk/ is under development at the moment, but I've opened it up to viewing to demonstrate this issue, so the CSS is in a bit of a mess :-(  ]  
> 
> Compare the menu viewed using Chrome to how it looks in Firefox 22.0 (latest release as of this date).

Thanks for the info. Would you be willing to create a small html sample that reproduces the problem and upload it here?

[Chris]

Created attachment 206384 [details]
Screenshot of firefox example

[Chris]

Created attachment 206385 [details]
Screenshot of chrome example

[Chris]

I have created a simple fiddle here: http://jsfiddle.net/chrisjg/q7nhQ/

example HTML

<nav role="navigation">
    <ul class="nav menu">
        <li class="item-1">short</li>
        <li class="item-2">a bit longer</li>
        <li class="item-3">this is a lot longer</li>
        <li class="item-4">short</li>
    </ul>
</nav>
-----------------

example CSS

nav {
    font-size: 16px;
    letter-spacing: -0.5px;
    list-style: none outside none;
}
.nav, nav ul {
    display: table;
    margin: 0;
    width: 100%;
}
.nav {
    float: left;
    left: 0;
    position: relative;
    list-style: none outside none;
    margin-bottom: 1.42857rem;
    margin-left: 0;
    width: 400px;
}

li {
    border: 1px solid black;
    display: table-cell;
    height: 34px;
    min-width: 25%;
    text-align: center;
    vertical-align: middle;
}

-------
The important bits are
.nav display: table;
li display: table-cell;
li min-width: 25%;

It should result in 4 equally sized boxes, 
as in the firefox screenshot I have attached. But in chrome it auto-sizes each box to fit the content - as in the chrome screenshot I also attached.

Hope this helps.

Chris.

[Anton Obzhirov]

Thanks! I will try convert to a ref test and fix the problem.

[Anton Obzhirov]

(In reply to comment #7)
> I have created a simple fiddle here: http://jsfiddle.net/chrisjg/q7nhQ/
> 
> example HTML
> 
> <nav role="navigation">
>     <ul class="nav menu">
>         <li class="item-1">short</li>
>         <li class="item-2">a bit longer</li>
>         <li class="item-3">this is a lot longer</li>
>         <li class="item-4">short</li>
>     </ul>
> </nav>
> -----------------
> 
> example CSS
> 
> nav {
>     font-size: 16px;
>     letter-spacing: -0.5px;
>     list-style: none outside none;
> }
> .nav, nav ul {
>     display: table;
>     margin: 0;
>     width: 100%;
> }
> .nav {
>     float: left;
>     left: 0;
>     position: relative;
>     list-style: none outside none;
>     margin-bottom: 1.42857rem;
>     margin-left: 0;
>     width: 400px;
> }
> 
> li {
>     border: 1px solid black;
>     display: table-cell;
>     height: 34px;
>     min-width: 25%;
>     text-align: center;
>     vertical-align: middle;
> }
> 
> -------
> The important bits are
> .nav display: table;
> li display: table-cell;
> li min-width: 25%;
> 
> It should result in 4 equally sized boxes, 
> as in the firefox screenshot I have attached. But in chrome it auto-sizes each box to fit the content - as in the chrome screenshot I also attached.
> 
> Hope this helps.
> 
> Chris.

Hi,

Checked the example html now - I think it is probably intended for another bug or new bug should be created for this. From what I see it doesn't use CSS  regions. Chris, could you confirm that - may be I am missing something.

[Anton Obzhirov]

Created attachment 208126 [details]
Patch

[Anton Obzhirov]

Submitted new test to test min/max width cases for regions - the test is pass. There is an issue with red line in the last region which in my opinion is not related to this bug but should be investigate in a new one. If is ok I will create a new bug for the red line issue.

[Mihnea Ovidenie]

Comment on attachment 208126 [details]
Patch

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.

[Mihnea Ovidenie]

(In reply to comment #11)
> Submitted new test to test min/max width cases for regions - the test is pass. There is an issue with red line in the last region which in my opinion is not related to this bug but should be investigate in a new one. If is ok I will create a new bug for the red line issue.

The redness you are seeing in the last region is not a bug. You can avoid it by making sure that each slice of content has 20px, like i said in the review.

[Anton Obzhirov]

Comment on attachment 208126 [details]
Patch

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

[Anton Obzhirov]

Created attachment 208194 [details]
Patch

[Mihnea Ovidenie]

(In reply to comment #15)
> Created an attachment (id=208194) [details]
> Patch

LGTM

[Alexandru Chiculita]

Comment on attachment 208194 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 208194 [details]
Patch

Clearing flags on attachment: 208194

Committed r154453: <http://trac.webkit.org/changeset/154453>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[murali]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=208194) [details]
> > Patch
> 
> LGTM

When i ran the final test case with webkit1 (r164000), im getting failed for Max-width cases. Min-width cases are  passing. Needed help..Copying below the failed cases:

FAIL:
 Expected 10 for width, but got 2. 
  <div id="region1" class="box region" data-expected-width="10"></div>
 
FAIL:
  Expected 10 for width, but got 2. 
  <div id="region2" class="box region" data-expected-width="10"></div>
 
FAIL:
 Expected 8 for width, but got 2. 
 <div id="region3" class="box region" data-expected-width="8"></div>

FAIL:
 Expected 18 for width, but got 2. 
 <div id="region4" class="box region" data-expected-width="18"></div>
 
FAIL:
 Expected 20 for width, but got 2. 
 <div id="region5" class="box region" data-expected-width="20"></div>
 
FAIL:
 Expected 3 for width, but got 2. 
 <div id="region6" class="box region" data-expected-width="3"></div>