Bug 71309

Created attachment 113204 [details] Proposed patch

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.

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

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>

(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>

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

(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.

(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 :)

(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.

(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.

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

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.

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

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.

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

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

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.

(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?

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.

(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.

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

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

Comment on attachment 116087 [details] Add tests to test_expectations.txt r=me, let's see if it works this time :-)

Comment on attachment 116087 [details] Add tests to test_expectations.txt Clearing flags on attachment: 116087 Committed r101062: <http://trac.webkit.org/changeset/101062>

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)"

(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.

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

(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

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

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

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