Bug 23908 – Add CSS parsing and style selection for 3D properties

• Component: CSS
• Status: RESOLVED FIXED

[Chris Marrin]

Need to add parsing of 3D transform functions and 3D related properties (perspective, perspective-origin, transform-style-3d, and backface-visibility).

[Chris Marrin]

Created attachment 27582 [details]
Patch with test

[Simon Fraser (smfr)]

Comment on attachment 27582 [details]
Patch with test

> Index: WebCore/css/CSSComputedStyleDeclaration.cpp
> ===================================================================

> -    RefPtr<WebKitCSSTransformValue> transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperation);
> +    RefPtr<WebKitCSSTransformValue>  transformVal;

Extra space there.

> +    if (transform.isAffine()) {
> +        transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::MatrixTransformOperation);
> +
> +        transformVal->append(CSSPrimitiveValue::create(transform.a(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.b(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.c(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.d(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.e(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.f(), CSSPrimitiveValue::CSS_NUMBER));
> +    } else {
> +        transformVal = WebKitCSSTransformValue::create(WebKitCSSTransformValue::Matrix3DTransformOperation);
> +
> +        transformVal->append(CSSPrimitiveValue::create(transform.m11(), CSSPrimitiveValue::CSS_NUMBER));
> +        transformVal->append(CSSPrimitiveValue::create(transform.m12(), CSSPrimitiveValue::CSS_NUMBER));

Add a FIXME comment about maybe printing out the individual operations if we can.

> +        case CSSPropertyWebkitBackfaceVisibility:
> +            return CSSPrimitiveValue::createIdentifier(style->backfaceVisibility() ? CSSValueVisible : CSSValueHidden);

This should use the enum values you added for backfaceVisibility().

> +        case CSSPropertyWebkitTransformStyle:
> +			return CSSPrimitiveValue::createIdentifier(style->transformStyle3D() ? CSSValuePreserve3d : CSSValueFlat);

Weird spacing, or tabs there. Also should use the enum values for transformStyle3D().

> Index: WebCore/css/CSSParser.cpp
> ===================================================================
> --- WebCore/css/CSSParser.cpp	(revision 40876)
> +++ WebCore/css/CSSParser.cpp	(working copy)
> @@ -1298,11 +1298,52 @@ bool CSSParser::parseValue(int propId, b
>          break;
>      case CSSPropertyWebkitTransformOrigin:
>      case CSSPropertyWebkitTransformOriginX:
> -    case CSSPropertyWebkitTransformOriginY: {
> +    case CSSPropertyWebkitTransformOriginY:
> +    case CSSPropertyWebkitTransformOriginZ:
> +    {

Brace should be on previous line.

> +        else if (equalIgnoringCase(name, "scalez(")) {
> +            m_unit = CSSParser::FNumber;
> +            m_type = WebKitCSSTransformValue::ScaleZTransformOperation;
> +        } else if (equalIgnoringCase(name, "scale3d(")) {
> +            m_type = WebKitCSSTransformValue::Scale3DTransformOperation;
> +            m_argCount = 5;
> +            m_unit = CSSParser::FNumber;
> +        } else if (equalIgnoringCase(name, "rotatex(") ||
> +                   equalIgnoringCase(name, "rotatey(") ||
> +                   equalIgnoringCase(name, "rotatez(")) {
> +            m_unit = CSSParser::FAngle;
> +            if (equalIgnoringCase(name, "rotatex("))
> +                m_type = WebKitCSSTransformValue::RotateXTransformOperation;
> +            else if (equalIgnoringCase(name, "rotatey("))
> +                m_type = WebKitCSSTransformValue::RotateYTransformOperation;
> +            else
> +                m_type = WebKitCSSTransformValue::RotateZTransformOperation;
> +        } else if (equalIgnoringCase(name, "rotate3d(")) {
> +            m_type = WebKitCSSTransformValue::Rotate3DTransformOperation;
> +            m_argCount = 7;
> +            m_unit = CSSParser::FNumber;
> +        } else if (equalIgnoringCase(name, "translatez(")) {
> +            m_unit = CSSParser::FLength | CSSParser::FPercent;
> +            m_type = WebKitCSSTransformValue::TranslateZTransformOperation;
> +        } else if (equalIgnoringCase(name, "translate3d(")) {
> +            m_type = WebKitCSSTransformValue::Translate3DTransformOperation;
> +            m_argCount = 5;
> +            m_unit = CSSParser::FLength | CSSParser::FPercent;
> +        } else if (equalIgnoringCase(name, "matrix3d(")) {
> +            m_type = WebKitCSSTransformValue::Matrix3DTransformOperation;
> +            m_argCount = 31;
> +            m_unit = CSSParser::FNumber;
> +        } else if (equalIgnoringCase(name, "perspective(")) {
> +            m_type = WebKitCSSTransformValue::PerspectiveTransformOperation;
> +            m_unit = CSSParser::FNumber;

I think we should intersperse the 3d properties with the related 2d ones, rather than separating
them out like this.


> -            if (!validUnit(a, unit, true))
> +            if (info.type() == WebKitCSSTransformValue::Rotate3DTransformOperation && argNumber == 3) {

Could use a comment to say why the 3rd param to Rotate3DTransformOperation is special.

> -            parseFillPosition(value, value2);
> -            // Unlike the other functions, parseFillPosition advances the m_valueList pointer
> +            parseTransformOrigin(value, value2, value3);
> +            // Unlike the other functions, parseTransformOrigin advances the m_valueList pointer

So now there are two functions that advance the m_valueList pointer? Comments need updating.

> +        case CSSPropertyWebkitTransformOriginZ: {
> +            if (validUnit(m_valueList->current(), FLength, m_strict))
> +            value = CSSPrimitiveValue::create(m_valueList->current()->fValue,
> +                                         (CSSPrimitiveValue::UnitTypes)m_valueList->current()->unit);

Bad indentation.

> -    return value;
> +    return (value != 0);

No need for this change.

> +    return (value != 0);

"return value;" for consistency.

> Index: WebCore/css/CSSParser.h
> ===================================================================
> --- WebCore/css/CSSParser.h	(revision 40876)
> +++ WebCore/css/CSSParser.h	(working copy)
> @@ -97,6 +97,7 @@ namespace WebCore {
>          PassRefPtr<CSSValue> parseAnimationProperty();
>          PassRefPtr<CSSValue> parseAnimationTimingFunction();
>  
> +        void parseTransformOrigin(RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&);
>          bool parseTimingFunctionValue(CSSParserValueList*& args, double& result);
>          bool parseAnimationProperty(int propId, RefPtr<CSSValue>&);
>          bool parseTransitionShorthand(bool important);
> @@ -144,8 +145,8 @@ namespace WebCore {
>          bool parseGradient(RefPtr<CSSValue>&);
>  
>          PassRefPtr<CSSValueList> parseTransform();
> -        bool parseTransformOrigin(int propId, int& propId1, int& propId2, RefPtr<CSSValue>&, RefPtr<CSSValue>&);
> -
> +        bool parseTransformOrigin(int propId, int& propId1, int& propId2, int& propId3, RefPtr<CSSValue>&, RefPtr<CSSValue>&, RefPtr<CSSValue>&);
> +        bool parsePerspectiveOrigin(int propId, int& propId1, int& propId,  RefPtr<CSSValue>&, RefPtr<CSSValue>&);
>          bool parseVariable(CSSVariablesDeclaration*, const String& variableName, const String& variableValue);
>          void parsePropertyWithResolvedVariables(int propId, bool important, CSSMutableStyleDeclaration*, CSSParserValueList*);

I really don't like the overloading; maybe rename new methods to avoid adding more?

> Index: WebCore/css/CSSStyleSelector.cpp
> ===================================================================

> +    case CSSPropertyWebkitBackfaceVisibility:
> +        HANDLE_INHERIT_AND_INITIAL(backfaceVisibility, BackfaceVisibility)
> +        m_style->setBackfaceVisibility((primitiveValue && primitiveValue->getIdent() == CSSValueVisible) ? BackfaceVisibilityVisible : BackfaceVisibilityHidden);

I'd do the if (primitiveValue) test separately like the other places do.

> +    case CSSPropertyWebkitTransformOriginZ: {
> +        HANDLE_INHERIT_AND_INITIAL(transformOriginZ, TransformOriginZ)
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +        float f;
> +        int type = primitiveValue->primitiveType();
> +        if (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG)
> +            f = (float) primitiveValue->computeLengthIntForLength(style());

static_cast<float>

> +    case CSSPropertyWebkitTransformStyle:
> +        HANDLE_INHERIT_AND_INITIAL(transformStyle3D, TransformStyle3D)
> +        m_style->setTransformStyle3D((primitiveValue && primitiveValue->getIdent() == CSSValuePreserve3d) ? TransformStyle3DPreserve3D : TransformStyle3DFlat);

Test primitiveValue separately.

> +        return;     
                  ^^^^^^  extra whitespace.
                  
> +        float perspectiveValue = (float) primitiveValue->getDoubleValue();

static_cast<>

> +    case CSSPropertyWebkitPerspectiveOrigin:
> +        HANDLE_INHERIT_AND_INITIAL(perspectiveOriginX, PerspectiveOriginX)
> +        HANDLE_INHERIT_AND_INITIAL(perspectiveOriginY, PerspectiveOriginY)
> +        return;
> +    case CSSPropertyWebkitPerspectiveOriginX: {
> +        HANDLE_INHERIT_AND_INITIAL(perspectiveOriginX, PerspectiveOriginX)
> +        CSSPrimitiveValue* primitiveValue = static_cast<CSSPrimitiveValue*>(value);
> +        Length l;
> +        int type = primitiveValue->primitiveType();
> +        if (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG)

It's odd seeing the (type > CSSPrimitiveValue::CSS_PERCENTAGE && type < CSSPrimitiveValue::CSS_DEG)
test everywhere. Seems like a good candidate for an inline function (isLengthType()),
and replace all uses.

> +                case WebKitCSSTransformValue::ScaleZTransformOperation:
> +                case WebKitCSSTransformValue::Scale3DTransformOperation: {

No need to separate this code from the 2d scale logic. Intersperse them, I think.

> +                case WebKitCSSTransformValue::TranslateZTransformOperation:
> +                case WebKitCSSTransformValue::Translate3DTransformOperation: {

Ditto.

> +                case WebKitCSSTransformValue::RotateXTransformOperation:
> +                case WebKitCSSTransformValue::RotateYTransformOperation:
> +                case WebKitCSSTransformValue::RotateZTransformOperation: {

Ditto.

> @@ -5908,6 +6081,33 @@ bool CSSStyleSelector::createTransformOp
>                      operations.operations().append(MatrixTransformOperation::create(a, b, c, d, e, f));
>                      break;
>                  }
> +                case WebKitCSSTransformValue::Matrix3DTransformOperation: {
> +                    TransformationMatrix matrix(static_cast<CSSPrimitiveValue*>(val->itemWithoutBoundsCheck(0))->getFloatValue(),

Use getDoubleValue everywhere now?

> Index: WebCore/css/WebKitCSSTransformValue.idl
> ===================================================================
> --- WebCore/css/WebKitCSSTransformValue.idl	(revision 40876)
> +++ WebCore/css/WebKitCSSTransformValue.idl	(working copy)
> @@ -48,6 +48,16 @@ module css {
>          const unsigned short CSS_SKEWX       = 9;
>          const unsigned short CSS_SKEWY       = 10;
>          const unsigned short CSS_MATRIX      = 11;
> +        const unsigned short CSS_TRANSLATEZ  = 12;
> +        const unsigned short CSS_TRANSLATE3D = 13;
> +        const unsigned short CSS_ROTATEX     = 14;
> +        const unsigned short CSS_ROTATEY     = 15;
> +        const unsigned short CSS_ROTATEZ     = 16;

Do we want CSS_ROTATEZ as a separate type from CSS_ROTATE?

I think another round would be good, and I'd like hyatt to do the final review.

[Chris Marrin]

Created attachment 27609 [details]
replacement patch

This patch addresses all issues, except the last (ROTATE vs ROTATE_Z), which is a spec issue and requires more discussion.

[Dave Hyatt]

Comment on attachment 27609 [details]
replacement patch

r=me

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/40939