Bug 217320 – Add non-animated support for the CSS rotate property

• Component: CSS
• Status: RESOLVED FIXED

[Antoine Quint]

Add non-animated support for the CSS rotate property

[Radar WebKit Bug Importer]

<rdar://problem/69956248>

[Antoine Quint]

Created attachment 410525 [details]
Patch

[Antoine Quint]

<rdar://problem/69956248>

[Antoine Quint]

Created attachment 410527 [details]
Patch

[Dean Jackson]

Comment on attachment 410527 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:658
> +    if (!rendererCanBeTransformed(renderer) || !rotate)

swap these?

> Source/WebCore/css/CSSValueKeywords.in:1143
> +// x
> +// y

Typically you'd leave these ones because they were already present, and they come first in the file (and comment the rotate versions)

> Source/WebCore/css/TransformFunctions.cpp:476
> +    return RotateTransformOperation::create(x, y, z, angle, type);

Are the x, y, z values normalized after parsing?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2141
> +    RefPtr<CSSValue> parsedValue;

This can go inside the loop.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2168
> +        if (parsedValue) {
> +            // If we've encountered an axis identifier, then this valus is invalid.
> +            if (axisIdentifier)
> +                return nullptr;
> +            list->append(*parsedValue);
> +        } else {
> +            // Then, attempt to parse an angle. We try this as a fallback rather than the first option because
> +            // a unitless 0 angle would be consumed as an angle.
> +            parsedValue = consumeAngle(range, cssParserMode, UnitlessQuirk::Forbid);
> +            if (parsedValue) {
> +                // If we had already parsed an angle or numbers but not 3 in a row, this value is invalid.
> +                if (angle || (list->length() && list->length() != 3))
> +                    return nullptr;
> +                angle = parsedValue;
> +            } else {
> +                // Finally, attempt to parse one of the axis identifiers.
> +                parsedValue = consumeIdent<CSSValueX, CSSValueY, CSSValueZ>(range);
> +                // If we failed to find one of those identifiers or one was already specified, or we'd previously
> +                // encountered numbers to specify a rotation axis, then this value is invalid.
> +                if (!parsedValue || axisIdentifier || list->length())
> +                    return nullptr;
> +                axisIdentifier = parsedValue;
> +            }
> +        }

These might read easier with "continue" statements rather than "else".

e.g.

// 1. Attempt to parse a number.
parsedValue = consumeNumber();
if (parsedValue) {
  // If we've encountered an axis...
  if (axisIdentifier)
    return nullptr;

  list->append(*parsedValue);
  continue;
}

// 2. Attempt to parse an angle....

[Antoine Quint]

Committed r267985: <https://trac.webkit.org/changeset/267985>

[Antoine Quint]

(In reply to Dean Jackson from comment #5)
> Comment on attachment 410527 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410527&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:658
> > +    if (!rendererCanBeTransformed(renderer) || !rotate)
> 
> swap these?

Fixed in commit.

> > Source/WebCore/css/CSSValueKeywords.in:1143
> > +// x
> > +// y
> 
> Typically you'd leave these ones because they were already present, and they
> come first in the file (and comment the rotate versions)

From the ChangeLog:

Define the "z" value for the "rotate" property and ensure the "x" and "y" values are
defined as well since -webkit-scroll-snap-type may not be enabled.

> > Source/WebCore/css/TransformFunctions.cpp:476
> > +    return RotateTransformOperation::create(x, y, z, angle, type);
> 
> Are the x, y, z values normalized after parsing?

I'm not sure what you mean there. The x/y/z values are just defined as <number> in CSS.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2141
> > +    RefPtr<CSSValue> parsedValue;
> 
> This can go inside the loop.

Fixed in commit.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2168
> > +        if (parsedValue) {
> > +            // If we've encountered an axis identifier, then this valus is invalid.
> > +            if (axisIdentifier)
> > +                return nullptr;
> > +            list->append(*parsedValue);
> > +        } else {
> > +            // Then, attempt to parse an angle. We try this as a fallback rather than the first option because
> > +            // a unitless 0 angle would be consumed as an angle.
> > +            parsedValue = consumeAngle(range, cssParserMode, UnitlessQuirk::Forbid);
> > +            if (parsedValue) {
> > +                // If we had already parsed an angle or numbers but not 3 in a row, this value is invalid.
> > +                if (angle || (list->length() && list->length() != 3))
> > +                    return nullptr;
> > +                angle = parsedValue;
> > +            } else {
> > +                // Finally, attempt to parse one of the axis identifiers.
> > +                parsedValue = consumeIdent<CSSValueX, CSSValueY, CSSValueZ>(range);
> > +                // If we failed to find one of those identifiers or one was already specified, or we'd previously
> > +                // encountered numbers to specify a rotation axis, then this value is invalid.
> > +                if (!parsedValue || axisIdentifier || list->length())
> > +                    return nullptr;
> > +                axisIdentifier = parsedValue;
> > +            }
> > +        }
> 
> These might read easier with "continue" statements rather than "else".

Used `continue` in the commit.