[Webkit-unassigned] [Bug 71309] Allow SVG elements to be transformed using webkit-transform

Tue Nov 15 02:00:42 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=71309

Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #114992|review?                     |review-
               Flag|                            |

--- Comment #14 from Nikolas Zimmermann <zimmermann at kde.org>  2011-11-15 02:00:38 PST ---
(From update of attachment 114992)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.