Bug 231361 – implement transform: perspective(none)

• Component: CSS
• Status: RESOLVED FIXED

[L. David Baron]

This is a bug to implement perspective(none), that is, the "none" keyword as an argument to the perspective transform function.  This was agreed by the CSS Working Group in https://github.com/w3c/csswg-drafts/issues/6488 and added to the spec in https://github.com/w3c/csswg-drafts/pull/6691 .  It was implemented in Gecko at https://bugzilla.mozilla.org/show_bug.cgi?id=1725207 and will be implemented in Chromium at https://bugs.chromium.org/p/chromium/issues/detail?id=1253596 .

There are currently a few tests in WPT in:
css/css-transforms/animation/transform-interpolation-001.html
css/css-transforms/parsing/transform-valid.html
and I plan to add one more in:
css/css-transforms/parsing/transform-computed.html

[Radar WebKit Bug Importer]

<rdar://problem/84124161>

[Martin Robinson]

Created attachment 442367 [details]
Patch

[Martin Robinson]

Created attachment 442471 [details]
Patch

[Martin Robinson]

Created attachment 442480 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 442480 [details]
Patch

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

The calc issues are not new with this patch but if testing shows that calc is broken here, please file a followup bug.

> Source/WebCore/css/TransformFunctions.cpp:325
> +            Length perspectiveLength = Length(0, LengthType::Fixed);

Doesn't need to be initialized since every branch has an assignment.

> Source/WebCore/css/TransformFunctions.cpp:333
> +                double doubleValue = firstValue.doubleValue();
> +                ASSERT(doubleValue >= 0);

Do we reject negative values earlier, or will perspective(-100px) assert here? We don't want assertions for any successfully parsed user input.

> Source/WebCore/css/TransformFunctions.cpp:334
> +                perspectiveLength =  Length(clampToPositiveInteger(doubleValue), LengthType::Fixed);

Integer is weird. Why can't perspective values be floating point? (not new).

Does this parsing work for calc()?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1843
> +    if (auto perspective = consumeNumberRaw(args, ValueRange::NonNegative)) {
> +        transformValue->append(CSSPrimitiveValue::create(*perspective, CSSUnitType::CSS_PX));
> +        return true;
> +    }

Does this work with calc()?

[Martin Robinson]

Created attachment 443207 [details]
Patch

[Martin Robinson]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 442480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442480&action=review

Thanks for the review. After chatting with you yesterday, I've modified this change to use std::optional<Length> for storing the perspective value.

> The calc issues are not new with this patch but if testing shows that calc
> is broken here, please file a followup bug.

I've filed a followup for this at:  https://bugs.webkit.org/show_bug.cgi?id=232669

> 
> > Source/WebCore/css/TransformFunctions.cpp:325
> > +            Length perspectiveLength = Length(0, LengthType::Fixed);
> 
> Doesn't need to be initialized since every branch has an assignment.

I've reworked this code block a bit and now this is fixed.

> > Source/WebCore/css/TransformFunctions.cpp:333
> > +                double doubleValue = firstValue.doubleValue();
> > +                ASSERT(doubleValue >= 0);
> 
> Do we reject negative values earlier, or will perspective(-100px) assert
> here? We don't want assertions for any successfully parsed user input.

I've removed the assertion. It's unclear if we can get negative values as a result of a calc here.

> > Source/WebCore/css/TransformFunctions.cpp:334
> > +                perspectiveLength =  Length(clampToPositiveInteger(doubleValue), LengthType::Fixed);
> 
> Integer is weird. Why can't perspective values be floating point? (not new).
> 
> Does this parsing work for calc()?

I think there are indeed some issues with this part of the code. I've filed a new bug to investigate them. calc() does seem to work (there is a WPT test for it), but I think maybe the correct context isn't passed in here. I suspect that it's broken for certain values.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:1843
> > +    if (auto perspective = consumeNumberRaw(args, ValueRange::NonNegative)) {
> > +        transformValue->append(CSSPrimitiveValue::create(*perspective, CSSUnitType::CSS_PX));
> > +        return true;
> > +    }
> 
> Does this work with calc()?

The test is still passing, but I think there's more work to do here.

[EWS]

Committed r285255 (243866@main): <https://commits.webkit.org/243866@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443207 [details].