Bug 91440

Summary:

Implement 'vmin' and 'vmax' from CSS3 values and units

Product:

WebKit

Reporter:

ben

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, cmarcelo, eric, koivisto, macpherson, menard, ojan.autocc, syoichi, tony, udaykiran4u, webkit.bugzilla, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths

Attachments:

Description	Flags
Proposed patch	none
Updated patch	none
Patch for landing	none

Description ben 2012-07-16 16:04:25 PDT

The revised W3C editor's draft specifies that what was called "vm" be renamed "vmin", and that "vmax" be added.

For the history of this see Bug 27160.

Comment 1 Uday Kiran 2013-01-27 02:29:23 PST

Created attachment 184907 [details]
Proposed patch

Comment 2 WebKit Review Bot 2013-01-27 02:31:03 PST

Attachment 184907 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle-expected.txt', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/CSSPrimitiveValue.idl', u'Source/WebCore/css/LengthFunctions.cpp', u'Source/WebCore/platform/Length.h', u'Source/WebCore/rendering/RenderBox.cpp']" exit_code: 1
Source/WebCore/css/CSSParser.cpp:1642:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSParser.cpp:1647:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/css/CSSPrimitiveValue.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:101:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 6 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 3 Uday Kiran 2013-01-27 02:41:52 PST

Comment on attachment 184907 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184907&action=review

>> Source/WebCore/css/CSSParser.cpp:1642
>> +           || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMAX)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Should I fix this?

>> Source/WebCore/css/CSSParser.cpp:1647
>> +           || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMAX));
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Ditto.

Comment 4 Tony Chang 2013-01-28 10:11:38 PST

Comment on attachment 184907 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184907&action=review

This looks fine to me, but it would be nice if Antti or Kling could take a look.

>>> Source/WebCore/css/CSSParser.cpp:1642
>>> +           || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMAX)
>> 
>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Should I fix this?

I would since you're here.  I suggested the same on the ch unit bug.

>> Source/WebCore/css/CSSPrimitiveValue.h:98
>> +        CSS_VMAX = 29,
> 
> enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]

This someone needs to fix as a separate refactor patch.

> Source/WebCore/css/CSSPrimitiveValue.idl:52
>      const unsigned short CSS_VW         = 26;
>      const unsigned short CSS_VH         = 27;
>      const unsigned short CSS_VMIN       = 28;
> +    const unsigned short CSS_VMAX       = 29;

Do these values need to be kept in sync with the enum in CSSPrimitiveValue.h?  If so, can we add a COMPILE_ASSERT to enforce it?

Comment 5 Uday Kiran 2013-01-28 10:35:47 PST

Comment on attachment 184907 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184907&action=review

Thanks for review.

>>>> Source/WebCore/css/CSSParser.cpp:1642
>>>> +           || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMAX)
>>> 
>>> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
>> 
>> Should I fix this?
> 
> I would since you're here.  I suggested the same on the ch unit bug.

Ok I will fix this.

>> Source/WebCore/css/CSSPrimitiveValue.idl:52
>> +    const unsigned short CSS_VMAX       = 29;
> 
> Do these values need to be kept in sync with the enum in CSSPrimitiveValue.h?  If so, can we add a COMPILE_ASSERT to enforce it?

CSS_DPPX, CSS_DPI, CSS_DPCM are not present in idl but present in CSSPrimitiveValue.h header file. So I think all values are not mandatory.

Comment 6 Uday Kiran 2013-01-28 12:14:24 PST

Created attachment 185030 [details]
Updated patch

Fixed suggested style errors.

Comment 7 WebKit Review Bot 2013-01-28 12:17:00 PST

Attachment 185030 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle-expected.txt', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/CSSPrimitiveValue.idl', u'Source/WebCore/css/LengthFunctions.cpp', u'Source/WebCore/platform/Length.h', u'Source/WebCore/rendering/RenderBox.cpp']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:101:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 8 Antti Koivisto 2013-02-06 10:50:48 PST

Comment on attachment 185030 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185030&action=review

r=me with the tests fixed

> LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-expected.html:19
> +<!DOCTYPE>
> +<html>
> +<style>
> + #element-container-vmax {
> +   background:green;
> + }
> +</style>
> +<div id="element-container-vmax">TEST PASSED</div>
> +<script>
> +function applyStyle() {
> +    var viewportMaxLength = Math.max(window.innerWidth, window.innerHeight);
> +    var elementStyle = document.getElementById("element-container-vmax").style;
> +    elementStyle.height = Math.floor(30 * viewportMaxLength / 100) + "px";
> +    elementStyle.width = Math.floor(30 * viewportMaxLength / 100) + "px";
> +    elementStyle.fontSize = Math.floor(3 * viewportMaxLength / 100) + "px";
> +    elementStyle.lineHeight = Math.floor(4 * viewportMaxLength / 100) + "px";
> +    elementStyle.textIndent = Math.floor(2 * viewportMaxLength / 100) + "px";
> +    elementStyle.marginLeft = Math.floor(2 * viewportMaxLength / 100) + "px";
> +    elementStyle.marginRight = Math.floor(2 * viewportMaxLength / 100) + "px";

Your tests and result files seems to have traded places. -expected.html has script in it.

Comment 9 Uday Kiran 2013-02-06 11:28:59 PST

Created attachment 186881 [details]
Patch for landing

Comment 10 WebKit Review Bot 2013-02-06 11:41:26 PST

Attachment 186881 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle-expected.txt', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-absolute.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax-expected.html', u'LayoutTests/css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-vmax.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParserValues.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/CSSPrimitiveValue.idl', u'Source/WebCore/css/LengthFunctions.cpp', u'Source/WebCore/platform/Length.h', u'Source/WebCore/rendering/RenderBox.cpp']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:98:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:99:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:100:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSPrimitiveValue.h:101:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 11 WebKit Review Bot 2013-02-06 12:59:41 PST

Comment on attachment 186881 [details]
Patch for landing

Clearing flags on attachment: 186881

Committed r142021: <http://trac.webkit.org/changeset/142021>

Comment 12 WebKit Review Bot 2013-02-06 12:59:46 PST

All reviewed patches have been landed.  Closing bug.