Bug 27160 – Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units

• Component: CSS
• Status: RESOLVED FIXED

[Aryeh Gregor]

CSS 3 Values and Units specifies vw, vh, and vm units.  These are respectively proportional to the width of the viewport, the height of the viewport, and the minimum of the two.  These would be extremely useful for authors -- quite a few very nasty hacks could be retired if we could use these (like <http://www.alistapart.com/articles/fauxcolumns/> and <http://matthewjamestaylor.com/blog/equal-height-columns-cross-browser-css-no-hacks>).  dhyatt said in #webkit that I should file a bug asking for this, and that "they're very easy to implement".

As far as I know, no other rendering engine implements any of these units yet, but I could be mistaken.  I'm going to file a bug against Gecko in a minute as well.  See also this discussion on www-style:

http://lists.w3.org/Archives/Public/www-style/2009Jul/0021.html

[Aryeh Gregor]

The Mozilla bug is:

https://bugzilla.mozilla.org/show_bug.cgi?id=503720

[Chris J. Shull]

IE9 has these: http://msdn.microsoft.com/en-us/ie/ff468705.aspx#_CSS3_Values_Units

[Ben Webb]

I've been working with the win8 dev preview and I'm waiting to see support for the new units in a real (non-ie) browser. If I had the foggiest idea how I would contribute support myself, but sadly I don't. Is anyone working on this problem?

[Joe Thomas]

I am working on this.

[Ben Webb]

Joe you're my hero.

[Joe Thomas]

Created attachment 121518 [details]
Initial Version

[Joe Thomas]

Created attachment 121519 [details]
testCase

Test Case for vw and vh unit which I found in https://bugzilla.mozilla.org/show_bug.cgi?id=503720

[Peter Beverloo]

The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate?

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

[Joe Thomas]

(In reply to comment #8)
> The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate?
> 
> http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths

I omitted vm deliberately based on http://www.w3.org/TR/css3-values/#viewport-relative-lengths. I can add the support during the next iteration after the patch review.

Can someone please review the proposed patch for vw/vh implementation?

[Eric Seidel (no email)]

Comment on attachment 121518 [details]
Initial Version

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Need tests :(

[Eric Seidel (no email)]

Also, your ChangeLog shoudl explain your change in greater detail.

[Joe Thomas]

Created attachment 122128 [details]
Patch2

Added new test case. Also more details to ChangeLog.

[Joe Thomas]

Can someone please review this?  Thanks.

[Luke Macpherson]

What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.

Code style wise, it would be nice to not have to add another parameter to computeLengthDouble and friends. Perhaps this is an opportunity to clean up that code by providing some more useful parameter that can provide the values necessary for the computation.

[Tab Atkins]

(In reply to comment #14)
> What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.

It's basically identical to using a lot of percentages, except it can potentially apply more widely.

[Ben Webb]

(In reply to comment #15)
> (In reply to comment #14)
> > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> 
> It's basically identical to using a lot of percentages, except it can potentially apply more widely.

This is how it works in the current IE10 implementation.

[Joe Thomas]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> > 
> > It's basically identical to using a lot of percentages, except it can potentially apply more widely.
> 
> This is how it works in the current IE10 implementation.

Thanks for the inputs. I will make a new patch which works with window resize.

[Joe Thomas]

Comment on attachment 122128 [details]
Patch2

vw vh are relative units and it needs to be updated with window resize which is missing in the current patch.

[Luke Macpherson]

(In reply to comment #15)
> (In reply to comment #14)
> > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> 
> It's basically identical to using a lot of percentages, except it can potentially apply more widely.

That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.

[Joe Thomas]

> That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.

Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one?

Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length ?

[Joe Thomas]

(In reply to comment #20)

> Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one?

One new LengthType should be sufficient. And a new sub-category of relative viewport Lengths can be added.
> 
> Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length?

A new function argument can be added to the getter functions in Length. But that does not look nice. Any other better approach?

[Joe Thomas]

Created attachment 125981 [details]
Patch-with-windowResize-Support

This patch includes support for window-resizing(the unit value changes if the window gets resized.)

[WebKit Review Bot]

Comment on attachment 125981 [details]
Patch-with-windowResize-Support

Attachment 125981 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11459279

New failing tests:
transforms/cssmatrix-3d-interface.xhtml
svg/dynamic-updates/SVG-dynamic-css-transform.html
transforms/svg-vs-css.xhtml
svg/dom/css-transforms.xhtml
transforms/cssmatrix-2d-interface.xhtml

[WebKit Review Bot]

Comment on attachment 125981 [details]
Patch-with-windowResize-Support

Attachment 125981 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11460250

New failing tests:
transforms/cssmatrix-3d-interface.xhtml
svg/dynamic-updates/SVG-dynamic-css-transform.html
transforms/svg-vs-css.xhtml
svg/dom/css-transforms.xhtml
transforms/cssmatrix-2d-interface.xhtml

[Joe Thomas]

Created attachment 126010 [details]
Patch-updated

Fixed the failing tests

[Luke Macpherson]

Comment on attachment 126010 [details]
Patch-updated

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

> Source/WebCore/css/CSSStyleSelector.cpp:2379
> +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:2411
> +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:2416
> +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:5692
> +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)

This code almost certainly belongs in CSSPrimitiveValue::computeLength.

> Source/WebCore/css/CSSStyleSelector.h:479
> +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);

captialization

> Source/WebCore/platform/Length.h:248
> +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }

I would like to s/RelativeViewport/ViewportRelative throughout this patch.

[Luke Macpherson]

Comment on attachment 126010 [details]
Patch-updated

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

> Source/WebCore/css/CSSStyleSelector.cpp:3606
> +        createTransformOperations(value, this, operations);

I think after you patch "this" is the only value passed in. In that case this parameter should be removed.

[Joe Thomas]

(In reply to comment #26)
> (From update of attachment 126010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> 
Thanks for the review.

> > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.

> > Source/WebCore/css/CSSStyleSelector.cpp:5692
> > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)
> 
> This code almost certainly belongs in CSSPrimitiveValue::computeLength.

I have not done this for 2 reasons. (1) CSSPrimitiveValue::ComputeLength is used mainly for "Fixed" length types for computing the actual length in pixels. It is not used for other Length types like "Percentage" or "Number". I think we should handle RelativeViewport length similar to "Percentage" since we are not calculating the pixel value in CSSPrimitiveValue class.  (2) RelativeViewport Length type needs a way to fetch the viewport size (currently I introduced an overloaded constructor and passing Document as an argument). So if I modify the arguments of CSSPrimitiveValue::computeLength templatized function, then I need to make changes in lots of places wherever this function is called. 

So I think it is better to keep it separate.
> 
> > Source/WebCore/css/CSSStyleSelector.h:479
> > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);
> 
> captialization

I will make this change.
> 
> > Source/WebCore/platform/Length.h:248
> > +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }
> 
> I would like to s/RelativeViewport/ViewportRelative throughout this patch.

OK. I will change RelativeViewport to ViewportRelative everywhere.

[Joe Thomas]

(In reply to comment #27)
> (From update of attachment 126010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:3606
> > +        createTransformOperations(value, this, operations);
> 
> I think after you patch "this" is the only value passed in. In that case this parameter should be removed.

I didn't understand this. Could you please elaborate?

[Luke Macpherson]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 126010 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> > 
> Thanks for the review.
> 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.

Yes, that sounds good to me.

> > > Source/WebCore/css/CSSStyleSelector.cpp:5692
> > > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)
> > 
> > This code almost certainly belongs in CSSPrimitiveValue::computeLength.
> 
> I have not done this for 2 reasons. (1) CSSPrimitiveValue::ComputeLength is used mainly for "Fixed" length types for computing the actual length in pixels. It is not used for other Length types like "Percentage" or "Number". I think we should handle RelativeViewport length similar to "Percentage" since we are not calculating the pixel value in CSSPrimitiveValue class.  (2) RelativeViewport Length type needs a way to fetch the viewport size (currently I introduced an overloaded constructor and passing Document as an argument). So if I modify the arguments of CSSPrimitiveValue::computeLength templatized function, then I need to make changes in lots of places wherever this function is called. 

That is a fair argument. I personally think that percentage and number-based lengths should be in computeLength too, but perhaps that is something to look at later on.

> So I think it is better to keep it separate.
> > 
> > > Source/WebCore/css/CSSStyleSelector.h:479
> > > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);
> > 
> > captialization
> 
> I will make this change.
> > 
> > > Source/WebCore/platform/Length.h:248
> > > +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }
> > 
> > I would like to s/RelativeViewport/ViewportRelative throughout this patch.
> 
> OK. I will change RelativeViewport to ViewportRelative everywhere.

[Joe Thomas]

(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (From update of attachment 126010 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> > > 
> > Thanks for the review.
> > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > > > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > > > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > > > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.
> 
> Yes, that sounds good to me.
> 
It's slightly difficult to make these functions as non-static member function of CSSStyleSelector. 

CSSStyleSelector::createTransformOperations is called from WebKitCSSMatrix::setMatrixValue and this class does not have access to CSSStyleSelector. It will be difficult to access this function if we make it non-static. It just passes null for style and rootElementStyle while calling CSSStyleSelector::createTransformOperations. 

convertToFloatLength & convertToIntLength are called from CSSStyleSelector::createTransformOperations. Since CSSStyleSelector argument is passed as NULL to createTransformOperations when called from WebKitCSSMatrix, it is not possible to call convertToFloatLength & convertToIntLength if they are member functions.

[Joe Thomas]

Created attachment 126385 [details]
Patch-updated-I

Review comments incorporated

[Antti Koivisto]

Comment on attachment 126385 [details]
Patch-updated-I

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

Someone more familiar with these standard should review (smfr, hyatt?).

> Source/WebCore/css/CSSStyleSelector.cpp:2433
> -static Length convertToLength(CSSPrimitiveValue* primitiveValue, RenderStyle* style, RenderStyle* rootStyle, bool toFloat, double multiplier = 1, bool *ok = 0)
> +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)

I don't like passing CSSStyleSelector*. Just try passing the minimal information you actually need for this (viewport?)

[Antti Koivisto]

Comment on attachment 126385 [details]
Patch-updated-I

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

> Source/WebCore/platform/Length.h:308
> +    unsigned char m_viewportLengthType;
> +    Document *m_document;

You can't add this stuff to Length. It is a memory-critical basic type.

[Antti Koivisto]

Not to mention Lengths may get shared between documents for various reasons and putting Document pointer there is nonsensical.

[Joe Thomas]

Created attachment 127238 [details]
Patch-BasedOn-Antti's Review Comments

Patch based on Antti Koivisto's review comments

Included viewportSize as one of the parameter in the getter functions of Length structure (calcValue, calcMinValue, calcFloatValue) with a default value IntSize().

Currently, the viewportSize parameter is used only when the width and height is calculated in RenderBox. Length getters are used extensively in webCore. I think it is better to include the viewport parameter in other places on a need basis. Any thoughts on this?

[Luke Macpherson]

Comment on attachment 127238 [details]
Patch-BasedOn-Antti's Review Comments

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

This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work.

> Source/WebCore/css/CSSStyleSelector.cpp:5768
> +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue)

Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon.

> Source/WebCore/platform/Length.h:248
> +        return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; 

This can simply be return type() >= ViewportRelativeWidth;

> Source/WebCore/rendering/RenderBox.cpp:1753
> +        logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize()));

Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1078
> +    LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true);

Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1084
> +    LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true);

Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

[Antti Koivisto]

Looks good to me, please just address Luke's comments.

[Joe Thomas]

(In reply to comment #37)
> (From update of attachment 127238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127238&action=review
> 
> This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work.
> 
Yes, viewport relative units can be used in most of the places where we call calcValue/CalcMinValue. I will make that change, but I am afraid that the patch is going to be big. 

> > Source/WebCore/css/CSSStyleSelector.cpp:5768
> > +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue)
> 
> Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon.
> 
I will move this function to CSSPrimitiveValue

> > Source/WebCore/platform/Length.h:248
> > +        return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; 
> 
> This can simply be return type() >= ViewportRelativeWidth;
> 
OK.

> > Source/WebCore/rendering/RenderBox.cpp:1753
> > +        logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize()));
> 
> Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based?
> 
I will make the change.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1078
> > +    LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true);
> 
> Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084
> > +    LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true);
> 
> Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[Joe Thomas]

Created attachment 127439 [details]
Patch-02-16

Patch which uses viewport relative units more widely

[Antti Koivisto]

The test coverage seems weak.

[Luke Macpherson]

Code LGTM. I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit. Agree that more test coverage is needed. Sorry, I know that's the least fun part of adding a feature like this.

[Tab Atkins Jr.]

Yes, ideal test coverage would be at least one usage of all three units in every property that can take lengths.  Easy to auto-generate.

[Joe Thomas]

OK. I will add more test cases. Tab, Could you please give me more details on how to auto-generate the test case?

[Joe Thomas]

While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.

[Joe Thomas]

Created attachment 128091 [details]
Patch-with-More-testCases

Patch with more test cases added.

[Joe Thomas]

Created attachment 128343 [details]
patch-Updated-TestCases

Added more test cases.

[Joe Thomas]

Antti, Could you please review the patch? I added more test cases as per your review comments.

[Antti Koivisto]

(In reply to comment #46)
> While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.

What was the problem? How did you address it?

[Joe Thomas]

(In reply to comment #50)
> (In reply to comment #46)
> > While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.
> 
> What was the problem? How did you address it?

Viewport Size was not passed to calcValue function in some of the cases (line-height, margin, etc). I added support for those missing cases.

[Joe Thomas]

Hyatt, Antti Koivisto wanted you also to take a look at the rendering changes as per the IRC conversation. Patch looks good to him.

Could you please review the changes?

[Dave Hyatt]

Comment on attachment 128343 [details]
patch-Updated-TestCases

I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it.

[Joe Thomas]

Created attachment 129988 [details]
Patch-incorporated-Hyatt's-Review-Comments

Incorporated Hyatt's Review comments

[Joe Thomas]

(In reply to comment #53)
> (From update of attachment 128343 [details])
> I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it.

Done. Uploaded new patch.

[Antti Koivisto]

r=me

[WebKit Review Bot]

Comment on attachment 129988 [details]
Patch-incorporated-Hyatt's-Review-Comments

Clearing flags on attachment: 129988

Committed r109656: <http://trac.webkit.org/changeset/109656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

(In reply to comment #43)
> I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit.

Was this comment ever addressed? I have to agree with Luke here, it looks like we're adding a pointless function call to the common case where passing a Document* would suffice.

[Antti Koivisto]

Passing Document as-is to Length::calcValue is not an option as that would be a layering violation. Maybe an abstract interface? Or just take care that getting viewport is fast on all platforms.

[Erik Arvidsson]

This seems to have regressed Parser/html5-full-render by about 10%

http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=1330759211690,1330784299690&displayrange=7&datatype=running

[Antti Koivisto]

This was rolled out in http://trac.webkit.org/changeset/109785

[Joe Thomas]

Created attachment 131175 [details]
Patch-With-Abstract-Interface

Added Abstract interface class for fetching the Viewport size.

I ran Parser/html5-full-render on Mac Snow-Leopard with webkit revision r110376 with and without the new patch and below are the values

Webkit rev 110376 - {16540.5ms, 16448ms, 16431ms, 16506.5ms, 16580ms}

Webkit rev 110376 with Patch - {16463ms, 16517.5ms, 16593ms, 16543ms, 16506ms}


The values looks more or less same in Mac SL but there is a variation of around 150-200 ms. Is it possible test the patch on performance bots for any possible regressions?

[WebKit Review Bot]

Comment on attachment 131175 [details]
Patch-With-Abstract-Interface

Attachment 131175 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11935467

New failing tests:
dom/svg/level3/xpath/Comment_Nodes.svg
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
http/tests/inspector/inspect-element.html
http/tests/misc/SVGFont-delayed-load.html
accessibility/aria-disabled.html
css2.1/20110323/replaced-intrinsic-005.htm
css2.1/20110323/replaced-intrinsic-004.htm
css2.1/20110323/replaced-intrinsic-003.htm
css2.1/20110323/background-intrinsic-002.htm
dom/svg/level3/xpath/Conformance_ID.svg
css2.1/20110323/background-intrinsic-007.htm
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
css2.1/20110323/background-intrinsic-009.htm
compositing/masks/multiple-masks.html
css2.1/20110323/background-intrinsic-001.htm
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
dom/svg/level3/xpath/Attribute_Nodes.svg
css2.1/20110323/background-intrinsic-008.htm
css3/images/cross-fade-invalidation.html
dom/svg/level3/xpath/Conformance_Expressions.svg
css2.1/20110323/background-intrinsic-003.htm

[WebKit Review Bot]

Created attachment 131221 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joe Thomas]

Created attachment 131229 [details]
Patch-With-Abstract-Interface-Updated

Fixed the chromium test failures.

Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms?

[Ryosuke Niwa]

(In reply to comment #66)
> Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms?

There's no EWS bot for performance tests. You need to land the patch and see what happens.

[Antti Koivisto]

Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation.

So instead of something like

return offset + style()->top().calcValue(containingBlock->availableHeight(), view());

you would have

return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view());

where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar).

It would be good to do this refactoring first, separate from the viewport size feature. How does this sound?

[Joe Thomas]

(In reply to comment #68)
> Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation.
> 
> So instead of something like
> 
> return offset + style()->top().calcValue(containingBlock->availableHeight(), view());
> 
> you would have
> 
> return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view());
> 
> where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar).
> 
> It would be good to do this refactoring first, separate from the viewport size feature. How does this sound?

I think it is a good idea. Bug 80897 is created for this.

[Joe Thomas]

Created attachment 133734 [details]
Patch-Updated

Ran PerformanceTests/Parser/html5-full-render.html(which got regressed previously) with and without the patch and there is no performance difference on MAC SL.

[WebKit Review Bot]

Comment on attachment 133734 [details]
Patch-Updated

Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12130744

New failing tests:
css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html

[WebKit Review Bot]

Created attachment 133743 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 133734 [details]
Patch-Updated

Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12131718

New failing tests:
css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html

[WebKit Review Bot]

Created attachment 133750 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joe Thomas]

(In reply to comment #73)
> (From update of attachment 133734 [details])
> Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12131718
> 
> New failing tests:
> css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html

Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac.

[Joe Thomas]

(In reply to comment #75)
> > 
> > New failing tests:
> > css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html
> 
> Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac.

Scrollbar width was not considered while getting the viewport size during length calculation which caused the mismatch. I will update the patch.

[Joe Thomas]

Created attachment 134068 [details]
Patch

Previously failed test case should fix by this patch.

[Antti Koivisto]

Comment on attachment 134068 [details]
Patch

r=me

[Antti Koivisto]

r=me

[WebKit Review Bot]

Comment on attachment 134068 [details]
Patch

Clearing flags on attachment: 134068

Committed r112301: <http://trac.webkit.org/changeset/112301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

font-size specified with vw, vh, or vmin not adjusting on browser resize -- see https://bugs.webkit.org/show_bug.cgi?id=106035