Bug 217320 - Add non-animated support for the CSS rotate property
Summary: Add non-animated support for the CSS rotate property
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks: 178117
  Show dependency treegraph
 
Reported: 2020-10-05 09:00 PDT by Antoine Quint
Modified: 2020-10-05 12:47 PDT (History)
20 users (show)

See Also:


Attachments
Patch (232.91 KB, patch)
2020-10-05 09:06 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (233.13 KB, patch)
2020-10-05 09:22 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2020-10-05 09:00:23 PDT
Add non-animated support for the CSS rotate property
Comment 1 Radar WebKit Bug Importer 2020-10-05 09:01:19 PDT
<rdar://problem/69956248>
Comment 2 Antoine Quint 2020-10-05 09:06:56 PDT
Created attachment 410525 [details]
Patch
Comment 3 Antoine Quint 2020-10-05 09:07:01 PDT
<rdar://problem/69956248>
Comment 4 Antoine Quint 2020-10-05 09:22:46 PDT
Created attachment 410527 [details]
Patch
Comment 5 Dean Jackson 2020-10-05 11:09:19 PDT
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....
Comment 6 Antoine Quint 2020-10-05 11:42:19 PDT
Committed r267985: <https://trac.webkit.org/changeset/267985>
Comment 7 Antoine Quint 2020-10-05 12:47:11 PDT
(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.