Bug 79678 – SVG transform-origin presentation attribute

• Component: SVG
• Status: RESOLVED FIXED

[Hans Muller]

Add support for an SVG transform-origin attribute with the same semantics as the -webkit-transform-origin CSS property.

[Hans Muller]

Created attachment 129531 [details]
Patch

[Hans Muller]

Created attachment 129716 [details]
Patch

[Dirk Schulze]

Comment on attachment 129716 [details]
Patch

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

> Source/WebCore/svg/SVGStyledElement.cpp:65
> +    if (!propertyId && attrName == transform_originAttr)
> +        propertyId = CSSPropertyWebkitTransformOrigin; // cssPropertyID("-webkit-transform-origin")

We assert 'propertyId > 0' afterwards. Is it guaranteed that we get 0 back for -webkit-transform-origin?

[Hans Muller]

(In reply to comment #3)
> (From update of attachment 129716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129716&action=review
> 
> > Source/WebCore/svg/SVGStyledElement.cpp:65
> > +    if (!propertyId && attrName == transform_originAttr)
> > +        propertyId = CSSPropertyWebkitTransformOrigin; // cssPropertyID("-webkit-transform-origin")
> 
> We assert 'propertyId > 0' afterwards. Is it guaranteed that we get 0 back for -webkit-transform-origin?

Yes, the cssPropertyId() function returns 0 if a match can't be found.

[Nikolas Zimmermann]

Comment on attachment 129716 [details]
Patch

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

Sorry, I have to r- this patch.

> Source/WebCore/svg/SVGStyledElement.cpp:64
> +    if (!propertyId && attrName == transform_originAttr)

Hm, seems high impact for this special case.
Why not change the singe call-site that calls mapAttributetoCSSProperty with transform_originAttr??
Add a new mapWebKitTransformOriginAttrToCSSProperty(..) method.

Also are you sure we want to add this unprefixed for SVG? Is there agreement that a transform-origin pres attr should exist? Where is it specified?

[Nikolas Zimmermann]

I'd like to hear some background info first - where is this specified? We add -webkit- prefixed CSS props, but unprefixed, unspecified SVG attrs? That's weird, please explain.

[Dirk Schulze]

(In reply to comment #6)
> I'd like to hear some background info first - where is this specified? We add -webkit- prefixed CSS props, but unprefixed, unspecified SVG attrs? That's weird, please explain.

That are absolutely reasonable concerns.

At first this is part of CSS3 Transforms [1]. A merged specification for CSS 2D Transforms, CSS 3D Transforms and SVG Transforms (in SVG 1.1 and the single module).

This specifies that all new CSS properties introduced with this specification will be available as presentation attributes in SVG.

To your concerns about prefixed attributes. I can absolutely understand this. We discussed this internally as well:
1)  "-webkit-" is not valid as an attribute name according to the XML syntax. So the only way would be to use webkit-transform-origin or webkitTransform-origin which doesn't look like an prefixed attribute but more as a proprietary attribute (which it is not).
2) There is no specification for prefixed attributes. This just does not happen :)
3) We had to take into account the time frame and the cost to change the attribute name when the property names get unprefixed. The current plan of the CSS WG is, that CSS3 Transform properties get unprefixed in May - independent of the state of the spec (what is kind of weird).

Therefore we thought it is better to add the attribute name directly, even if the specification is not in CR. The current hack can get removed in 2 months and a prefix wouldn't be covered by any specification anyway.


[1] http://dev.w3.org/csswg/css3-transforms/#svg-transform

[Dirk Schulze]

Comment on attachment 129716 [details]
Patch

Removing cq+ to give others time to comment.

[Dirk Schulze]

(In reply to comment #5)
> (From update of attachment 129716 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129716&action=review
> 
> Sorry, I have to r- this patch.
> 
> > Source/WebCore/svg/SVGStyledElement.cpp:64
> > +    if (!propertyId && attrName == transform_originAttr)
> 
> Hm, seems high impact for this special case.
> Why not change the singe call-site that calls mapAttributetoCSSProperty with transform_originAttr??
> Add a new mapWebKitTransformOriginAttrToCSSProperty(..) method.
> 
I'd like to mention that the map gets filled once during the whole runtime! The code will just stay as long as 'transform-origin' is prefixed (which won't be long). So adding new functions might be a bit heavy. What do you think?

[Nikolas Zimmermann]

Comment on attachment 129716 [details]
Patch

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

>>> Source/WebCore/svg/SVGStyledElement.cpp:64
>>> +    if (!propertyId && attrName == transform_originAttr)
>> 
>> Hm, seems high impact for this special case.
>> Why not change the singe call-site that calls mapAttributetoCSSProperty with transform_originAttr??
>> Add a new mapWebKitTransformOriginAttrToCSSProperty(..) method.
>> 
>> Also are you sure we want to add this unprefixed for SVG? Is there agreement that a transform-origin pres attr should exist? Where is it specified?
> 
> I'd like to mention that the map gets filled once during the whole runtime! The code will just stay as long as 'transform-origin' is prefixed (which won't be long). So adding new functions might be a bit heavy. What do you think?

Good point, I think its fine as-is!

[WebKit Review Bot]

Comment on attachment 129716 [details]
Patch

Clearing flags on attachment: 129716

Committed r109916: <http://trac.webkit.org/changeset/109916>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.