Perspective property should only allow positive lengths

Perspective property should only allow positive lengths

Perspective property did check for non-negative lengths,
but not zero lengths, so add that check for both
prefixed and unprefixed versions.

transforms/perspective-parsing.html now passes.

BUG=301241
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192019

[rwlbuis, 2015-03-10 21:52:51]

rob.buis@samsung.com changed reviewers:
+ dstockwell@chromium.org

[rwlbuis, 2015-03-10 21:52:52]

PTAL

[dstockwell, 2015-03-10 22:32:26]

lgtm

[dstockwell, 2015-03-10 23:10:37]

dstockwell@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2015-03-10 23:17:20]

On 2015/03/10 21:52:52, rwlbuis wrote:
> PTAL

I can't imagine this working properly for calcs :\

[rwlbuis, 2015-03-12 01:35:53]

On 2015/03/10 23:17:20, Timothy Loh wrote:
> On 2015/03/10 21:52:52, rwlbuis wrote:
> > PTAL
> 
> I can't imagine this working properly for calcs :\

My latest patch set should fix it, PTAL.

[alancutter (OOO until 2018), 2015-03-12 03:43:07]

alancutter@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2015-03-12 03:43:08]

https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
File Source/core/css/CSSCalculationValue.h (right):

https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
Source/core/css/CSSCalculationValue.h:122: bool isPositive() const { return
m_expression->doubleValue() > 0; }
The doubleValue() function is broken for lengths and should probably
ASSERT_NOT_REACHED() in these cases.

The calc spec defers clamping of the value until a final value can be produced:
http://dev.w3.org/csswg/css-values-3/#calc-range

We should just allow calcs in the parser and clamp appropriately in
convertPerspectiveLength().
At the moment it clamps to >= 0 which seems incorrect for perspective. Instead
we should probably clamp to a small value like 1e-8 otherwise we treat 0 as none
which is an implementation detail rather than spec described behaviour.

[rwlbuis, 2015-03-12 21:18:57]

On 2015/03/12 03:43:08, alancutter wrote:
>
https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
> File Source/core/css/CSSCalculationValue.h (right):
> 
>
https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
> Source/core/css/CSSCalculationValue.h:122: bool isPositive() const { return
> m_expression->doubleValue() > 0; }
> The doubleValue() function is broken for lengths and should probably
> ASSERT_NOT_REACHED() in these cases.
> 
> The calc spec defers clamping of the value until a final value can be
produced:
> http://dev.w3.org/csswg/css-values-3/#calc-range
> 
> We should just allow calcs in the parser and clamp appropriately in
> convertPerspectiveLength().
> At the moment it clamps to >= 0 which seems incorrect for perspective. Instead
> we should probably clamp to a small value like 1e-8 otherwise we treat 0 as
none
> which is an implementation detail rather than spec described behaviour.

Isn't that a different bug? Is there a testcase that will not work with my
patch? I added quite
a few calc tests in my two testcases but it was not exhaustive. If you can tell
me what would
fail I maybe can understand above better.
Maybe we need a FPositiveLength too? It seemed quite hard to me to change
validUnit and the
calc path and not slow down the general case...

[alancutter (OOO until 2018), 2015-03-13 01:46:41]

On 2015/03/12 at 21:18:57, rob.buis wrote:
> On 2015/03/12 03:43:08, alancutter wrote:
> >
https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
> > File Source/core/css/CSSCalculationValue.h (right):
> > 
> >
https://codereview.chromium.org/993013002/diff/60001/Source/core/css/CSSCalcu...
> > Source/core/css/CSSCalculationValue.h:122: bool isPositive() const { return
> > m_expression->doubleValue() > 0; }
> > The doubleValue() function is broken for lengths and should probably
> > ASSERT_NOT_REACHED() in these cases.
> > 
> > The calc spec defers clamping of the value until a final value can be
produced:
> > http://dev.w3.org/csswg/css-values-3/#calc-range
> > 
> > We should just allow calcs in the parser and clamp appropriately in
> > convertPerspectiveLength().
> > At the moment it clamps to >= 0 which seems incorrect for perspective.
Instead
> > we should probably clamp to a small value like 1e-8 otherwise we treat 0 as
none
> > which is an implementation detail rather than spec described behaviour.
> 
> Isn't that a different bug? Is there a testcase that will not work with my
patch? I added quite
> a few calc tests in my two testcases but it was not exhaustive. If you can
tell me what would
> fail I maybe can understand above better.
> Maybe we need a FPositiveLength too? It seemed quite hard to me to change
validUnit and the
> calc path and not slow down the general case...

Here's a test that shows the behaviour I'm suggesting:
http://jsfiddle.net/dozypsr6/
All calc()s used in perspective are valid at parse time.

I discussed this with tabatkins (one of the editors of the calc spec). His
opinion was that the perspective spec should define a non-zero clamping
behaviour for calc and we should clamp at style application time to something >
0 for calc perspectives.
The problem with isPositive() and isNegative() on calc is that we don't always
know at parse time whether an expression will come out positive or negative when
different units are mixed e.g. calc(100px - 50%). We could of course detect
cases that are certain like calc(-1px) and reject those at parse time but I
think this makes parser rejection logic needlessly complex.

[rwlbuis, 2015-03-14 00:08:04]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-14 00:08:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/993013002/#ps80001 (title:
" ")

[rwlbuis, 2015-03-14 00:08:06]

The CQ bit was unchecked by rob.buis@samsung.com

[rwlbuis, 2015-03-16 21:31:36]

On 2015/03/13 01:46:41, alancutter wrote:
> On 2015/03/12 at 21:18:57, rob.buis wrote:
> > Maybe we need a FPositiveLength too? It seemed quite hard to me to change
> validUnit and the
> > calc path and not slow down the general case...
> 
> Here's a test that shows the behaviour I'm suggesting:
> http://jsfiddle.net/dozypsr6/
> All calc()s used in perspective are valid at parse time.
> 
> I discussed this with tabatkins (one of the editors of the calc spec). His
> opinion was that the perspective spec should define a non-zero clamping
> behaviour for calc and we should clamp at style application time to something
>
> 0 for calc perspectives.
> The problem with isPositive() and isNegative() on calc is that we don't always
> know at parse time whether an expression will come out positive or negative
when
> different units are mixed e.g. calc(100px - 50%). We could of course detect
> cases that are certain like calc(-1px) and reject those at parse time but I
> think this makes parser rejection logic needlessly complex.

Great point. In my patch set #5 I no longer check calcs used by perspective to
be positive. WDYT? Not sure if the non-zero clamping on calc perspective should
be part of this patch or not...

[alancutter (OOO until 2018), 2015-03-16 23:33:15]

lgtm with nit.

I'm fine with having the calc clamping in a separate patch.

https://codereview.chromium.org/993013002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/993013002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1163: }
This can probably be simplified as:
if (id == CSSValueNone)
    validPrimitive = true;
else if (validUnit(value, FLength) && (m_parsedCalculation || value->fValue >
0))
    validPrimitive = true;
else
    return false;

Similarly below.

[rwlbuis, 2015-03-17 13:51:51]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-17 13:51:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org, alancutter@chromium.org
Link to the patchset: https://codereview.chromium.org/993013002/#ps100001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-03-17 13:52:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/993013002/100001

[rwlbuis, 2015-03-17 14:39:03]

On 2015/03/16 23:33:15, alancutter wrote:
> lgtm with nit.
> 
> I'm fine with having the calc clamping in a separate patch.

Actually it seems we are in luck. StyleBuilderConverter::convertPerspective
already
clamps to 0.0. Also LayoutStyle::hasPerspective does check for perspective > 0,
so
negative and zero values for perspective will not have any effect.

>
https://codereview.chromium.org/993013002/diff/80001/Source/core/css/parser/C...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/993013002/diff/80001/Source/core/css/parser/C...
> Source/core/css/parser/CSSPropertyParser.cpp:1163: }
> This can probably be simplified as:
> if (id == CSSValueNone)
>     validPrimitive = true;
> else if (validUnit(value, FLength) && (m_parsedCalculation || value->fValue >
> 0))
>     validPrimitive = true;
> else
>     return false;
> 
> Similarly below.

Fixed.

[commit-bot: I haz the power, 2015-03-17 16:28:26]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192019

[alancutter (OOO until 2018), 2015-03-17 22:59:28]

On 2015/03/17 at 14:39:03, rob.buis wrote:
> On 2015/03/16 23:33:15, alancutter wrote:
> > lgtm with nit.
> > 
> > I'm fine with having the calc clamping in a separate patch.
> 
> Actually it seems we are in luck. StyleBuilderConverter::convertPerspective
already
> clamps to 0.0. Also LayoutStyle::hasPerspective does check for perspective >
0, so
> negative and zero values for perspective will not have any effect.

This is actually a problem as 0 is not a valid value for perspective, we just
use it internally to represent none. We should be clamping to something > 0
instead.
calcs that resolve to values outside the allowed range are supposed to get
clamped accordingly. It doesn't make sense that clamping -1px to the range (0,
inf) would result in the keyword 'none'.
The perspective spec still needs updating to clarify appropriate clamping
behaviour for an open range such as (0, inf) but in the meantime tab has
indicated that clamping to a user agent decided value like 1e-8 is fine.