Bug 79678 - SVG transform-origin presentation attribute
Summary: SVG transform-origin presentation attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-27 09:30 PST by Hans Muller
Modified: 2012-03-06 07:05 PST (History)
3 users (show)

See Also:


Attachments
Patch (6.19 KB, patch)
2012-02-29 15:20 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (6.21 KB, patch)
2012-03-01 09:36 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 2012-02-27 09:30:51 PST
Add support for an SVG transform-origin attribute with the same semantics as the -webkit-transform-origin CSS property.
Comment 1 Hans Muller 2012-02-29 15:20:00 PST
Created attachment 129531 [details]
Patch
Comment 2 Hans Muller 2012-03-01 09:36:20 PST
Created attachment 129716 [details]
Patch
Comment 3 Dirk Schulze 2012-03-01 09:47:38 PST
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?
Comment 4 Hans Muller 2012-03-01 09:53:42 PST
(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.
Comment 5 Nikolas Zimmermann 2012-03-01 13:28:01 PST
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?
Comment 6 Nikolas Zimmermann 2012-03-01 13:29:17 PST
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.
Comment 7 Dirk Schulze 2012-03-01 15:17:51 PST
(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
Comment 8 Dirk Schulze 2012-03-01 15:18:28 PST
Comment on attachment 129716 [details]
Patch

Removing cq+ to give others time to comment.
Comment 9 Dirk Schulze 2012-03-01 16:41:44 PST
(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?
Comment 10 Nikolas Zimmermann 2012-03-06 06:31:20 PST
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!
Comment 11 WebKit Review Bot 2012-03-06 07:05:50 PST
Comment on attachment 129716 [details]
Patch

Clearing flags on attachment: 129716

Committed r109916: <http://trac.webkit.org/changeset/109916>
Comment 12 WebKit Review Bot 2012-03-06 07:05:55 PST
All reviewed patches have been landed.  Closing bug.