Bug 71309 – Allow SVG elements to be transformed using webkit-transform

• Component: SVG
• Status: RESOLVED FIXED

[Raul Hudea]

Have the SVG renderer use the RenderStyle::transform() rather than the transform attribute, in the case where the RenderStyle's transform is not null. This means no existing SVG content should break. People will be able to use CSS transforms on SVG through the -webkit-transform property (only via a style sheet)

What's expected to work:
- simple CSS styling with -webkit-transform
- CSS Animations for -webkit-transform
- CSS Transitions for -webkit-transform
- SVG animation for an SVG element transform with -webkit-transform

[Raul Hudea]

Created attachment 113204 [details]
Proposed patch

[WebKit Review Bot]

Attachment 113204 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/mac/transforms/svg-cs..." exit_code: 1

Source/WebCore/svg/SVGStyledTransformableElement.cpp:78:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 113204 [details]
Proposed patch

Attachment 113204 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10148030

New failing tests:
transforms/svg-css-transforms.xhtml

[Dirk Schulze]

Comment on attachment 113204 [details]
Proposed patch

You're missing the changelogs. Also, why haven't you changed SVGTextElement as well? r-

Even so, I disagree with the implementation in general. I'd implement 'transform' as a CSS property and the transform attribute would be mapped to the CSS property. I don't believe that this solution would help us a lot. Of course this needs more harmonization with the CSS parser and SVG DOM. I'd start rather with the CSS parser than with a separate implementation of 'transform' attr and  '-webkit-transform'.
Just see the following example to understand what I mean. The CSS property has the value of the attribute:

<svg xmlns="http://www.w3.org/2000/svg" onload="start()">
<script type="text/ecmascript"><![CDATA[
function start() {
var e = document.getElementsByTagName('text')[0];
var c = window.getComputedStyle(e, null);
alert(c.getPropertyCSSValue('display').cssText);
}
]]></script>
<text x="40" y="40" display="none">ABC</text>
</svg>

[Dean Jackson]

(In reply to comment #4)
> (From update of attachment 113204 [details])
> You're missing the changelogs. Also, why haven't you changed SVGTextElement as well? r-
> 
> Even so, I disagree with the implementation in general. I'd implement 'transform' as a CSS property and the transform attribute would be mapped to the CSS property.

Hmmm.. this is the implementation I was thinking of. We can't make "transform" a CSS property at the moment since the spec isn't done - it needs to be prefixed. Given that, it makes sense to take the first step here, which is to use the existing -webkit-transform style if it is present.

This also has the guarantee that any existing content that breaks will clearly be doing something wrong :) 

Later on we'll move it to an unprefixed "transform".

> I don't believe that this solution would help us a lot. Of course this needs more harmonization with the CSS parser and SVG DOM. I'd start rather with the CSS parser than with a separate implementation of 'transform' attr and  '-webkit-transform'.

I disagree, but I might not be fully understanding the reasoning behind your position. The approach in the patch seems to be the nicest first step: Zero change to any existing SVG content. Prefixed property to use CSS. Uses all the existing code for parsing/setting -webkit-transform.

Dean

> Just see the following example to understand what I mean. The CSS property has the value of the attribute:
> 
> <svg xmlns="http://www.w3.org/2000/svg" onload="start()">
> <script type="text/ecmascript"><![CDATA[
> function start() {
> var e = document.getElementsByTagName('text')[0];
> var c = window.getComputedStyle(e, null);
> alert(c.getPropertyCSSValue('display').cssText);
> }
> ]]></script>
> <text x="40" y="40" display="none">ABC</text>
> </svg>

[Dean Jackson]

Raul,

When you have your changes staged you can run ./Tools/Scripts/prepare-ChangeLog to generate ChangeLog entries. Edit the files it changes and include them in your patch.

Dean

[Nikolas Zimmermann]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 113204 [details] [details])
> > You're missing the changelogs. Also, why haven't you changed SVGTextElement as well? r-
> > 
> > Even so, I disagree with the implementation in general. I'd implement 'transform' as a CSS property and the transform attribute would be mapped to the CSS property.
I partly agree with Dirk, as long-term solution. The SVG transform attribute should be mapped to a unified CSS property, shared between CSS Transforms & SVG. This will resolve the problem, that you have to decide which property to take (which is what the current patch is doing).

> 
> Hmmm.. this is the implementation I was thinking of. We can't make "transform" a CSS property at the moment since the spec isn't done - it needs to be prefixed. Given that, it makes sense to take the first step here, which is to use the existing -webkit-transform style if it is present.
> 
> This also has the guarantee that any existing content that breaks will clearly be doing something wrong :) 
> 
> Later on we'll move it to an unprefixed "transform".
If that implies to switching SVG transform and both CSS transforms to this new unprefix CSS "transform" property, then I agree.

> 
> > I don't believe that this solution would help us a lot. Of course this needs more harmonization with the CSS parser and SVG DOM. I'd start rather with the CSS parser than with a separate implementation of 'transform' attr and  '-webkit-transform'.
> 
> I disagree, but I might not be fully understanding the reasoning behind your position. The approach in the patch seems to be the nicest first step: Zero change to any existing SVG content. Prefixed property to use CSS. Uses all the existing code for parsing/setting -webkit-transform.

If Raul and or other folks are continuing to work towards a unified unprefixed CSS property and this is only the first step, then I'm completely fine with it. Though I strongly think we shouldn't ever ship this as-is, even if the rule is "only if the prefixed -webkit-transform is specified, it has precedence over the SVG transform attribute".

Ok, say we take this route, then I'd request more testcases.

1)
Several code paths walk the transform hiearchy from a certain renderer up to RenderSVGRoot to figure out the absolute on-screen size, to create ImageBuffers for clipping/masking/patterns etc. in the correct size to avoid pixelation. This computed transform is then used to render on that ImageBuffer.
It's important that -webkit-transform is tested to work as well in this context.
eg.
<circle cx="10" cy="10" style="-webkit-transform: scale(10)" (with the transform origin set to match SVG of course)
used as clipPath.

2)
getCTM() on a -webkit-transform'ed <rect>. Test how SVGTransformList looks (should be identity, as currently SVG transform and -webkit-transform is decoupled). Very important!
We actually need tests that demonstrate which property currently has precendence!

3)
SVG transform and -webkit-transform set on a <rect>.
Dump SVGTransformList, dump CSS trafo, assure that they are not both applied currently.

4)
Influence on hit testing. Create copy of existing pointer-event / hittesting test cases, and replace some transforms by webkit-transforms, see if it still works :-)

....
I'd just like to be sure that we document the new behavior precisely, by tests.

[Raul Hudea]

(In reply to comment #6)

> When you have your changes staged you can run ./Tools/Scripts/prepare-ChangeLog to generate ChangeLog entries. Edit the files it changes and include them in your patch.
> 

I had them, but they weren't committed, hence missing from git diff :)

[Raul Hudea]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 113204 [details] [details] [details])
> > > You're missing the changelogs. Also, why haven't you changed SVGTextElement as well? r-
> > > 
> > > Even so, I disagree with the implementation in general. I'd implement 'transform' as a CSS property and the transform attribute would be mapped to the CSS property.
> I partly agree with Dirk, as long-term solution. The SVG transform attribute should be mapped to a unified CSS property, shared between CSS Transforms & SVG. This will resolve the problem, that you have to decide which property to take (which is what the current patch is doing).
> 
> > 
> > Hmmm.. this is the implementation I was thinking of. We can't make "transform" a CSS property at the moment since the spec isn't done - it needs to be prefixed. Given that, it makes sense to take the first step here, which is to use the existing -webkit-transform style if it is present.
> > 
> > This also has the guarantee that any existing content that breaks will clearly be doing something wrong :) 
> > 
> > Later on we'll move it to an unprefixed "transform".
> If that implies to switching SVG transform and both CSS transforms to this new unprefix CSS "transform" property, then I agree.
> 
> > 
> > > I don't believe that this solution would help us a lot. Of course this needs more harmonization with the CSS parser and SVG DOM. I'd start rather with the CSS parser than with a separate implementation of 'transform' attr and  '-webkit-transform'.
> > 
> > I disagree, but I might not be fully understanding the reasoning behind your position. The approach in the patch seems to be the nicest first step: Zero change to any existing SVG content. Prefixed property to use CSS. Uses all the existing code for parsing/setting -webkit-transform.
> 
> If Raul and or other folks are continuing to work towards a unified unprefixed CSS property and this is only the first step, then I'm completely fine with it. Though I strongly think we shouldn't ever ship this as-is, even if the rule is "only if the prefixed -webkit-transform is specified, it has precedence over the SVG transform attribute".

This is only the first step, as the final goal is to unify both CSS and SVG transforms.

> 
> Ok, say we take this route, then I'd request more testcases.
> 
> 1)
> Several code paths walk the transform hiearchy from a certain renderer up to RenderSVGRoot to figure out the absolute on-screen size, to create ImageBuffers for clipping/masking/patterns etc. in the correct size to avoid pixelation. This computed transform is then used to render on that ImageBuffer.
> It's important that -webkit-transform is tested to work as well in this context.
> eg.
> <circle cx="10" cy="10" style="-webkit-transform: scale(10)" (with the transform origin set to match SVG of course)
> used as clipPath.
> 
> 2)
> getCTM() on a -webkit-transform'ed <rect>. Test how SVGTransformList looks (should be identity, as currently SVG transform and -webkit-transform is decoupled). Very important!
> We actually need tests that demonstrate which property currently has precendence!
> 
> 3)
> SVG transform and -webkit-transform set on a <rect>.
> Dump SVGTransformList, dump CSS trafo, assure that they are not both applied currently.
> 
> 4)
> Influence on hit testing. Create copy of existing pointer-event / hittesting test cases, and replace some transforms by webkit-transforms, see if it still works :-)
> 

Thanks for the new tests suggestions. I will add them.

[Dean Jackson]

(In reply to comment #7)

> I partly agree with Dirk, as long-term solution. The SVG transform attribute should be mapped to a unified CSS property, shared between CSS Transforms & SVG. 

Yes, I think we all agree on this.

> If that implies to switching SVG transform and both CSS transforms to this new unprefix CSS "transform" property, then I agree.

Yes.

> If Raul and or other folks are continuing to work towards a unified unprefixed CSS property and this is only the first step, then I'm completely fine with it. Though I strongly think we shouldn't ever ship this as-is, even if the rule is "only if the prefixed -webkit-transform is specified, it has precedence over the SVG transform attribute".

This is only the first step. I don't see how we can avoid it, because we should not ship with transform as a unprefixed CSS property while the spec is not complete.

[Raul Hudea]

Created attachment 114992 [details]
Added more tests and allow SVGTextElements to be CSS transformed

[WebKit Review Bot]

Attachment 114992 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/svg/SVGTextElement.cpp:126:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/svg/SVGStyledTransformableElement.cpp:79:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 114992 [details]
Added more tests and allow SVGTextElements to be CSS transformed

Attachment 114992 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10361035

New failing tests:
svg/dynamic-updates/SVGClipPathElement-css-transform-influences-hitTesting.html
svg/clip-path/clip-path-css-transform-1.svg
svg/dom/css-transforms.xhtml
svg/custom/pointer-events-text-css-transform.svg
svg/clip-path/clip-path-css-transform-2.svg
svg/transforms/svg-css-transforms.xhtml
svg/custom/pointer-events-image-css-transform.svg
svg/transforms/svg-css-transforms-clip-path.xhtml

[Nikolas Zimmermann]

Comment on attachment 114992 [details]
Added more tests and allow SVGTextElements to be CSS transformed

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

Great job! I'd still have some open issues that currently lead to r-, sorry for being strict!

> LayoutTests/svg/clip-path/clip-path-css-transform-1.svg:5
> +    <!-- second rect causes masking -->

It's a circle :-)
Excellent that you've added tests for both clipping code paths, once with buffers and once without.

> LayoutTests/svg/custom/clip-path-with-css-transform.svg:6
> +        <path style="-webkit-transform: scale(.5)" d="M 0 0 l 200 0 l 0 200 l -200 0 Z"/>

It would be great if you could add the same test again, just like with transformed <clipPaths> but using two another primitive, like <rect>/<circle>/etc.. to trigger the image buffer code path.

> LayoutTests/svg/custom/pointer-events-image-css-transform.svg:1
> +<?xml version="1.0"?>

While I don't like that this is copy&paste, it's okay as-is for now, if you leave a comment that this is 1:1 a copy of an existing test, but using webkit-transform as only difference, which is what I assume you did :-)

> LayoutTests/svg/custom/pointer-events-text-css-transform.svg:1
> +<?xml version="1.0"?>

Ditto.

> LayoutTests/svg/dom/css-transforms.xhtml:41
> +

Also dump the SVGTransformList here of the rect, and make sure it doesn't interfere with the -webkit-transform at the moment.
(There are existing tests, I think, in svg/dynamic-updates, that dump SVGTransformLists, you can check for examples here).
All methods from SVGLocatable interface should be tested and documented here for both the circle and the rect: getScreenCTM/getTransformToElement/getCTM.

Sorry for demanding so much, but I think it's important to document all corner cases right from the beginning, so that we can easily see the influence of future changes in this area.

> Source/WebCore/ChangeLog:13
> +        Tests: svg/clip-path/clip-path-css-transform-1.svg

The test coverage got much better, thanks for tackling this. 
Another area which should be tested is: dynamic updates. I'd suggest to use svg/dynamic-updates/SVGTextElement-dom-transform-attr.html as template for svg/transforms/svg-css-transforms-updates.xhtml. You don't need to use the script-tests framework as it's done in the svg/dynamic-update tests, instead you can concatenate the script-tests/SVGTextElement-dom-transform-attr.js and the html file into one. You also don't need to use any scripting to setup the document, I just wanted to suggest to create a test in the spirit of that svg/dynamic-updates test.

What needs testing:
a) setting an inline style attribute of eg. a rect, to a specific -webkit-transform. Verify repainting worked fine and CSSOM/SVG DOM reflect those changes.
b) setting the style using CSS OM - test the same.
c) setting the -webkit-transform on a <div>, which contains an inline <svg> in a HTML5 compound document. Eg. set a scale to 2, and check that the SVG zoomed correctly.
d) whatever you can also imagine.

This should give basic coverage, if we react to dynamic changes properly.
Sorry again, for demanding all these changes, but I'd like to be certain this is going to work as-is and not break anything existing.

>> Source/WebCore/svg/SVGStyledTransformableElement.cpp:79
>>      if (m_supplementalTransform)
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

Please fix the style issue :-)

> Source/WebCore/svg/SVGTextElement.cpp:125
> +        transform().concatenate(matrix);

Ditto.

[Raul Hudea]

Created attachment 115604 [details]
Address styling issues and add more tests

[WebKit Review Bot]

Comment on attachment 115604 [details]
Address styling issues and add more tests

Attachment 115604 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10511416

New failing tests:
svg/dynamic-updates/SVG-dynamic-css-transform.html
fast/repaint/scroll-fixed-reflected-layer.html
svg/dynamic-updates/SVGClipPathElement-css-transform-influences-hitTesting.html
svg/clip-path/clip-path-css-transform-1.svg
fast/repaint/fixed-table-overflow-zindex.html
svg/dom/css-transforms.xhtml
fast/replaced/width100percent-textarea.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/repaint/fixed-tranformed.html
svg/transforms/svg-css-transforms.xhtml
svg/clip-path/clip-path-css-transform-2.svg
css2.1/20110323/abspos-containing-block-initial-004d.htm
svg/custom/pointer-events-image-css-transform.svg
fast/repaint/fixed-scale.html
svg/transforms/svg-css-transforms-clip-path.xhtml

[Nikolas Zimmermann]

Comment on attachment 115604 [details]
Address styling issues and add more tests

This looks really great, r=me! Thanks for the testcases, they are very nice.

[Dean Jackson]

(In reply to comment #17)
> (From update of attachment 115604 [details])
> This looks really great, r=me! Thanks for the testcases, they are very nice.

Did you forget to set the r flag?

[Dean Jackson]

Comment on attachment 115604 [details]
Address styling issues and add more tests

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

Great tests. Since Nikolas said r=him I'll expect he's ok with this.

> Source/WebCore/ChangeLog:41
> +        (WebCore::RenderSVGModelObject::styleWillChange):
> +   
> +            Set the updateTransform flag on SVG elements whenever a CSS transform is present on the style
> +
> +        * svg/SVGStyledTransformableElement.cpp:
> +        (WebCore::SVGStyledTransformableElement::animatedLocalTransform):
> +
> +            Use the RenderStyle's transform (if it exists) over the SVG's transform
> +
> +        * svg/SVGTextElement.cpp:
> +        (WebCore::SVGTextElement::animatedLocalTransform):
> +
> +            Use the RenderStyle's transform (if it exists) over the SVG's transform

Typically these messages start from the same line as the filename.

[Dean Jackson]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 115604 [details] [details])
> > This looks really great, r=me! Thanks for the testcases, they are very nice.
> 
> Did you forget to set the r flag?

Oh, I see. It was the failing cr-linux test. If you can work that out, r=me still.

[Raul Hudea]

Created attachment 116087 [details]
Add tests to test_expectations.txt

[WebKit Review Bot]

Comment on attachment 116087 [details]
Add tests to test_expectations.txt

Attachment 116087 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10534293

New failing tests:
inspector/extensions/extensions-events.html

[Nikolas Zimmermann]

Comment on attachment 116087 [details]
Add tests to test_expectations.txt

r=me, let's see if it works this time :-)

[WebKit Review Bot]

Comment on attachment 116087 [details]
Add tests to test_expectations.txt

Clearing flags on attachment: 116087

Committed r101062: <http://trac.webkit.org/changeset/101062>

[Csaba Osztrogonác]

The new svg/dom/css-transforms.xhtml test fails on 32 bit GTK and Qt platforms, 
but passes on 64 bit. There is only one rounding problem: 0.0 != -0.0.

--- /ramdisk/qt-linux-release/build/layout-test-results/svg/dom/css-transforms-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/svg/dom/css-transforms-actual.txt 
@@ -23,7 +23,7 @@
 PASS dumpMatrix(rect.getCTM()) is "[0.7 0.7 -0.7 0.7 100.0 0.0]"
 PASS dumpMatrix(rect.getScreenCTM()) is "[0.7 0.7 -0.7 0.7 312.0 8.0]"
 PASS dumpRect(rect.getBBox()) is "[40 40 100 100]"
-PASS dumpMatrix(rect.getTransformToElement(rect)) is "[1.0 0.0 0.0 1.0 0.0 0.0]"
+FAIL dumpMatrix(rect.getTransformToElement(rect)) should be [1.0 0.0 0.0 1.0 0.0 0.0]. Was [1.0 0.0 0.0 1.0 0.0 -0.0].
 
 Test CSSMatrix
 PASS circle.style.webkitTransform is "scale(2, 2) translate(10px, 10px)"

[Philippe Normand]

(In reply to comment #25)
> The new svg/dom/css-transforms.xhtml test fails on 32 bit GTK and Qt platforms, 
> but passes on 64 bit. There is only one rounding problem: 0.0 != -0.0.
> 

I added it to our famous -0.0 skipped tests list for GTK.

[Adam Klein]

Rebaselined Chromium in http://trac.webkit.org/changeset/101106

[Csaba Osztrogonác]

(In reply to comment #25)
> The new svg/dom/css-transforms.xhtml test fails on 32 bit GTK and Qt platforms, 
> but passes on 64 bit. There is only one rounding problem: 0.0 != -0.0.
> 
> --- /ramdisk/qt-linux-release/build/layout-test-results/svg/dom/css-transforms-expected.txt 
> +++ /ramdisk/qt-linux-release/build/layout-test-results/svg/dom/css-transforms-actual.txt 
> @@ -23,7 +23,7 @@
>  PASS dumpMatrix(rect.getCTM()) is "[0.7 0.7 -0.7 0.7 100.0 0.0]"
>  PASS dumpMatrix(rect.getScreenCTM()) is "[0.7 0.7 -0.7 0.7 312.0 8.0]"
>  PASS dumpRect(rect.getBBox()) is "[40 40 100 100]"
> -PASS dumpMatrix(rect.getTransformToElement(rect)) is "[1.0 0.0 0.0 1.0 0.0 0.0]"
> +FAIL dumpMatrix(rect.getTransformToElement(rect)) should be [1.0 0.0 0.0 1.0 0.0 0.0]. Was [1.0 0.0 0.0 1.0 0.0 -0.0].
> 
>  Test CSSMatrix
>  PASS circle.style.webkitTransform is "scale(2, 2) translate(10px, 10px)"

I added it to the Qt skipped list: http://trac.webkit.org/changeset/101124

[Dean Jackson]

Should this be marked as FIXED - do we want to track the test failures here or separately?

[Dirk Schulze]

I think we can mark it as fixed. All following patches should get new bug reports.

[Simon Fraser (smfr)]

This patch added some tests to WebCore/manual-tests. That directory is deprecated. Manual tests should go into the top-level ManualTests directory.