Bug 91440 – Implement 'vmin' and 'vmax' from CSS3 values and units

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

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.

[Uday Kiran]

Created attachment 184907 [details]
Proposed patch

[WebKit Review Bot]

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.

[Uday Kiran]

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.

[Tony Chang]

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?

[Uday Kiran]

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.

[Uday Kiran]

Created attachment 185030 [details]
Updated patch

Fixed suggested style errors.

[WebKit Review Bot]

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.

[Antti Koivisto]

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.

[Uday Kiran]

Created attachment 186881 [details]
Patch for landing

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 186881 [details]
Patch for landing

Clearing flags on attachment: 186881

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

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.