Bug 115248

Summary: [CSSRegions] Min-width and max-width for a region should support values other than length
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: chris.jones-gill, commit-queue, muralibilla, obzhirov, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Screenshot of firefox example
none
Screenshot of chrome example
none
Patch
none
Patch none

Description Mihnea Ovidenie 2013-04-26 05:21:29 PDT
Right now, code in RenderRegion::computePreferredLogicalWidths supports only length values. It should support other values like: percentage, calc or viewport relative
Comment 1 Anton Obzhirov 2013-06-11 08:52:17 PDT
I am going to try fixing this one.
Comment 2 Anton Obzhirov 2013-06-13 07:04:57 PDT
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.
Comment 3 Chris 2013-07-06 10:56:30 PDT
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).
Comment 4 Mihnea Ovidenie 2013-07-07 23:08:13 PDT
(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?
Comment 5 Chris 2013-07-10 06:22:01 PDT
Created attachment 206384 [details]
Screenshot of firefox example
Comment 6 Chris 2013-07-10 06:23:08 PDT
Created attachment 206385 [details]
Screenshot of chrome example
Comment 7 Chris 2013-07-10 06:27:06 PDT
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.
Comment 8 Anton Obzhirov 2013-07-10 08:24:34 PDT
Thanks! I will try convert to a ref test and fix the problem.
Comment 9 Anton Obzhirov 2013-08-01 07:07:47 PDT
(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.
Comment 10 Anton Obzhirov 2013-08-05 08:22:33 PDT
Created attachment 208126 [details]
Patch
Comment 11 Anton Obzhirov 2013-08-05 08:32:23 PDT
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.
Comment 12 Mihnea Ovidenie 2013-08-06 07:06:17 PDT
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.
Comment 13 Mihnea Ovidenie 2013-08-06 07:09:10 PDT
(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.
Comment 14 Anton Obzhirov 2013-08-06 07:29:15 PDT
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
Comment 15 Anton Obzhirov 2013-08-06 08:50:20 PDT
Created attachment 208194 [details]
Patch
Comment 16 Mihnea Ovidenie 2013-08-07 01:26:21 PDT
(In reply to comment #15)
> Created an attachment (id=208194) [details]
> Patch

LGTM
Comment 17 Alexandru Chiculita 2013-08-16 13:19:59 PDT
Comment on attachment 208194 [details]
Patch

r=me
Comment 18 WebKit Commit Bot 2013-08-22 09:56:44 PDT
Comment on attachment 208194 [details]
Patch

Clearing flags on attachment: 208194

Committed r154453: <http://trac.webkit.org/changeset/154453>
Comment 19 WebKit Commit Bot 2013-08-22 09:56:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 murali 2016-01-05 09:58:15 PST
(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>