Bug 91440 - Implement 'vmin' and 'vmax' from CSS3 values and units
: Implement 'vmin' and 'vmax' from CSS3 values and units
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dev.w3.org/csswg/css3-values/#...
:
:
:
  Show dependency treegraph
 
Reported: 2012-07-16 16:04 PST by
Modified: 2013-02-06 12:59 PST (History)


Attachments
Proposed patch (31.80 KB, patch)
2013-01-27 02:29 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (32.03 KB, patch)
2013-01-28 12:14 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (32.07 KB, patch)
2013-02-06 11:28 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-07-16 16:04:25 PST
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 From 2013-01-27 02:29:23 PST -------
Created an attachment (id=184907) [details]
Proposed patch
------- Comment #2 From 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 From 2013-01-27 02:41:52 PST -------
(From update of attachment 184907 [details])
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 From 2013-01-28 10:11:38 PST -------
(From update of attachment 184907 [details])
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 From 2013-01-28 10:35:47 PST -------
(From update of attachment 184907 [details])
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 From 2013-01-28 12:14:24 PST -------
Created an attachment (id=185030) [details]
Updated patch

Fixed suggested style errors.
------- Comment #7 From 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 From 2013-02-06 10:50:48 PST -------
(From update of attachment 185030 [details])
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 From 2013-02-06 11:28:59 PST -------
Created an attachment (id=186881) [details]
Patch for landing
------- Comment #10 From 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 From 2013-02-06 12:59:41 PST -------
(From update of attachment 186881 [details])
Clearing flags on attachment: 186881

Committed r142021: <http://trac.webkit.org/changeset/142021>
------- Comment #12 From 2013-02-06 12:59:46 PST -------
All reviewed patches have been landed.  Closing bug.