Created attachment 127901 [details] Preview patch SVG should support the CSS property 'transform-origin' and relative values for 'transform' and 'transform-origin' values. I wrote a patch for it, but it is missing tests and a change log.
Created attachment 130180 [details] Patch
Comment on attachment 130180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130180&action=review r- for the function naming, but the patch seems OK. > Source/WebCore/platform/Length.h:168 > - float calcFloatValue(int maxValue) const > + float calcFloatValue(float maxValue) const It's possible that this could change rounding behavior in other parts of the code. Have you run all the layout tests? > Source/WebCore/rendering/style/RenderStyle.cpp:774 > +inline bool optimizeForTransformOrigin(const TransformOperations& transformOperations, RenderStyle::ApplyTransformOrigin applyOrigin) optimizeForTransformOrigin() doesn't tell me what this function actually does. It should have a name like requireTransformOrigin(). > Source/WebCore/rendering/style/RenderStyle.cpp:828 > + > + Remove one of these blank lines. > LayoutTests/svg/transforms/transform-origin-css-property.xhtml:32 > + You should see 9 green 20x20 rectangles below, each rectangle should be rotated by 45 degrees. Each green rectangle > + is actually one of a set of about 6, each one with a different but equivalent value for the CSS transform-origin > + property. Behind each stack of blue rectangles is a single 20x20 red div that has been specified in in the same way. > + The red divs should be completely obscured. > + </p> Please avoid text in tests where pixel testing is important. Differences between platforms will almost always make it impossible to share pixel results. This text can go into an HTML comment. Which blue rectangles? Can you make the rects bigger to make failures easier to detect? This could also be a ref test, perhaps.
(In reply to comment #2) > (From update of attachment 130180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130180&action=review > > It's possible that this could change rounding behavior in other parts of the code. Have you run all the layout tests? We'd been concerned about this too; will check if the change introduces any additional failures and add an additional Length method (instead of changing an existing one) if it does. > optimizeForTransformOrigin() doesn't tell me what this function actually does. It should have a name like requireTransformOrigin(). That sounds like a better name, since the function returns false if the transform-origin isn't needed. > This could also be a ref test, perhaps. Good point, will make this a ref test. Will also make the rectangles bigger and move the descriptive text to an HTML comment.
Created attachment 130431 [details] Patch
Attachment 130431 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1 Source/WebCore/platform/Length.h:192: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
> Source/WebCore/platform/Length.h:192: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] > Total errors found: 1 in 11 files The switch statement is contained in copy of calcFloatValue(int). I didn't change the existing indentation in calcFloatValue(float)) (the copy) since it's consistent with the entire file.
Comment on attachment 130431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130431&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:798 > + bool applyTransformOrigin = requireTransformOrigin(rareNonInheritedData->m_transform->m_operations, applyOrigin); I think it would be worth having a local const reference to rareNonInheritedData->m_transform->m_operations, to avoid two de-refs every time you use the operations list. > Source/WebCore/rendering/style/RenderStyle.cpp:813 > + bool applyTransformOrigin = requireTransformOrigin(rareNonInheritedData->m_transform->m_operations, applyOrigin); ditto.
Comment on attachment 130431 [details] Patch Attachment 130431 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11837591 New failing tests: svg/transforms/transform-origin-css-property.xhtml
Created attachment 130466 [details] Patch
(In reply to comment #7) > (From update of attachment 130431 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130431&action=review > > > Source/WebCore/rendering/style/RenderStyle.cpp:798 > > + bool applyTransformOrigin = requireTransformOrigin(rareNonInheritedData->m_transform->m_operations, applyOrigin); > > I think it would be worth having a local const reference to rareNonInheritedData->m_transform->m_operations, to avoid two de-refs every time you use the operations list. > > > Source/WebCore/rendering/style/RenderStyle.cpp:813 > > + bool applyTransformOrigin = requireTransformOrigin(rareNonInheritedData->m_transform->m_operations, applyOrigin); > > ditto. I've just uploaded a patch with these changes.
Attachment 130466 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1 Source/WebCore/platform/Length.h:192: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130466&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:798 > + const TransformOperations transformOperations = rareNonInheritedData->m_transform->m_operations; This is a copy. You want a const TransformOperations&
Comment on attachment 130466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130466&action=review Some more comments from me: > Source/WebCore/ChangeLog:8 > + Added SVG support for the CSS 'transform-origin' property and for percentage I would like to see at least one additional testcase, that uses the repaint.js harness / repaintTest() functionality and excercises dynamic changes of the 'transform-origin' property, eg. from 0 0 to 50% 50% for a SVG element. I'd like to verify that dynamic changes are properly covered as well by your patch. Also please amend platform/chromium/test_expectations.txt, to mark your new tests as needing rebaseline (= IMAGE+TEXT IMAGE TEXT PASS). > Source/WebCore/ChangeLog:14 > + * css/svg.css: I think changes like those should really be explained here in the ChangeLog, making it easier to understand what happened later say in 2 years. Please extend the ChangeLog. > Source/WebCore/css/svg.css:68 > +/* transform-origin on SVG elements */ Ideally you'd link a spec here, or leave a comment in the ChangeLog, where these rules are coming from, who defined them, etc. (Dirk should know the spec links) > Source/WebCore/css/svg.css:74 > +html|* > svg { So you're restoring the default behavior here from HTML, any html name-spaced element within a svg tree, has the 50% 50% transform origin. > Source/WebCore/platform/Length.h:168 > + // FIXME - when subpixel layout is supported this copy of calcFloatValue() can be removed. See bug 71143. We usually use // FIXME: When subpixel... >> Source/WebCore/platform/Length.h:192 >> + case Fixed: > > A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] I still think you shouldn't add wrong styled code, even if you copied the function. Make it correct from the beginning, once we switch to the subpixel branch we can then just remove the calcFloatValue(int) variant and be happy. > Source/WebCore/rendering/style/RenderStyle.cpp:774 > +inline bool requireTransformOrigin(const TransformOperations& transformOperations, RenderStyle::ApplyTransformOrigin applyOrigin) Why do you pass const TransformOperations& here, if you want to pass transformOperations.operations() directly. > Source/WebCore/rendering/style/RenderStyle.cpp:799 > + bool applyTransformOrigin = requireTransformOrigin(transformOperations, applyOrigin); Once you fixed Simons comment above, you can then use pass trasnsformOperations.operations() here, to avoid calling this multiple times inside requiresTransformOrigin. > Source/WebCore/rendering/style/RenderStyle.cpp:804 > + unsigned size = transformOperations.operations().size(); ditto. You should introduce a local variable (as reference again!) that points to transformOperations.operations() > Source/WebCore/rendering/style/RenderStyle.cpp:826 > + unsigned size = transformOperations.operations().size(); Same here. > Source/WebCore/svg/SVGStyledTransformableElement.cpp:72 > + // RenderSVGHiddenContainers don't have an objectBoundingBox. Therefore no > + // percentage value can be used there. I don't understand this comment, how does this affect the code? > LayoutTests/ChangeLog:8 > + Separate tests for SVG CSS transform-origin property support and for You rather added new tests, then separated existing ones, that sounds confusing. > LayoutTests/svg/transforms/transform-origin-css-property-expected.xhtml:37 > + function test() Not really needed, make it inline.
Created attachment 130861 [details] Patch
(In reply to comment #15) Made all of the suggested changes and added a repaint test (svg/transforms/dynamic-transform-origin.svg).
Comment on attachment 130861 [details] Patch Attachment 130861 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11894040
Comment on attachment 130861 [details] Patch Attachment 130861 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11898021
Created attachment 130882 [details] Patch
(In reply to comment #19) Previous version of the patch was based on an obsolete version of Length.h.
Comment on attachment 130882 [details] Patch Attachment 130882 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11905007 New failing tests: svg/transforms/transform-origin-css-property.xhtml
Comment on attachment 130882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130882&action=review Nice patch Hans - it's gettin much closer, still some new comments: > Source/WebCore/ChangeLog:28 > + * rendering/style/RenderStyle.cpp: > + (WebCore): > + (WebCore::optimizeForTransformOrigin): > + (WebCore::RenderStyle::applyTransform): The interesting parts are not described :( It's helpful to know here that you've added a second applyTransform() variant, which now takes a FloatSize. You should describe the differences between the borderBoxSize and the boundingRect, and why you have to add the boundingRect.x()/y() offset when applying this. I'm aware of the differences (HTML only requires the borderBoxSize, for SVG we have the objectBoundingBox() only and thus need to calculate percentages relative to that box, which requires the x/y translation to be respected), but I guess not anyone is, so this would be helpful to describe. 896 void applyTransform(TransformationMatrix&, const LayoutSize& borderBoxSize, ApplyTransformOrigin = IncludeTransformOrigin) const; 897 void applyTransform(TransformationMatrix&, const FloatRect& boundingBox, ApplyTransformOrigin = IncludeTransformOrigin) const; > Source/WebCore/css/svg.css:68 > +/* transform-origin on SVG elements */ That's obvious ;-) > Source/WebCore/css/svg.css:74 > +html|* > svg { I'd add here // Restore default transform origin behavior for html name-spaced elements in SVG documents. It's not immediately clear for anyone that 50%, 50% is the HTML default :-) > Source/WebCore/rendering/style/RenderStyle.cpp:796 > +void RenderStyle::applyTransform(TransformationMatrix& transform, const LayoutSize& borderBoxSize, ApplyTransformOrigin applyOrigin) const I woud have made this just call, applyTransform(transform, FloatRect(FloatPoint(), borderBoxSize), applyOrigin) instead of duplicating code and eventually mark both inline. Or do you need to call the non-float calcFloatValue variant here, explicitely? I'd just try it, run all tests, if nothing new is broken, just go ahead and avoid code dup. > LayoutTests/svg/transforms/dynamic-transform-origin.svg:7 > + document.getElementById('rect').style.webkitTransformOrigin = "50% 50%"; I'd like to see a copy of this test that does the same but using the transform-origin attribute. The patch is in that maps the transform-origin attribute to -webkit-transform-origin CSS property, no? So this should work.
Created attachment 131360 [details] Patch
Comment on attachment 131360 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131360&action=review Patch looks great. My suggestion about using the repaint.js harnes was wrong, please use reftests for this, as we discussed in private mails. > Source/WebCore/rendering/style/RenderStyle.h:897 > + void applyTransform(TransformationMatrix&, const FloatRect& boundingBox, ApplyTransformOrigin = IncludeTransformOrigin) const; I would have still named it applyTransformForHTML vs. applyTransformForSVG.
Created attachment 131435 [details] Patch
Comment on attachment 131435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131435&action=review Just snippets. But you don't have commuter rights yet. > Source/WebCore/platform/Length.h:203 > return 0; copyright is missing > Source/WebCore/rendering/style/RenderStyle.cpp:4 > + * Copyright (C) 2011 Adobe Systems Incorporated. All Rights Reserved. s/All Rights Reserved./All rights reserved./ > Source/WebCore/svg/SVGStyledTransformableElement.cpp:71 > + // Note: objectBoundingBox is an emptyRect for elements like pattern or clipPath. You should also reference to the spec text that demands that. Otherwise this would not be a good enough reason to take a note instead of a fix me :)
Created attachment 131451 [details] Patch
Comment on attachment 131451 [details] Patch Clearing flags on attachment: 131451 Committed r110532: <http://trac.webkit.org/changeset/110532>
All reviewed patches have been landed. Closing bug.
This test's expectation has been updated by Philippe Normand at revision #110543 but it fails on Qt. There is no platform specific expectation yet. Any idea would be appreciated. I will skip this test until that. * LayoutTests/inspector/styles/svg-style-expected.txt
I skipped this test. http://trac.webkit.org/changeset/110558
(In reply to comment #30) > This test's expectation has been updated by Philippe Normand at revision #110543 but it fails on Qt. > There is no platform specific expectation yet. Any idea would be appreciated. I will skip this test until that. > > * LayoutTests/inspector/styles/svg-style-expected.txt This test fails on Mac/Lion, with or without the patch for bug 79068. The problem appears to be that the inspector thinks that the SVG rect in inspector/styles/svg-style-expected.txt appears on line 38 when it clearly appears on line 30 (which is what the expected.txt file expects). Since the test was only intended to verify that the the inspector doesn't crash, matching line numbers may be overkill.
(In reply to comment #32) > (In reply to comment #30) > > This test's expectation has been updated by Philippe Normand at revision #110543 but it fails on Qt. > > There is no platform specific expectation yet. Any idea would be appreciated. I will skip this test until that. > > > > * LayoutTests/inspector/styles/svg-style-expected.txt > > This test fails on Mac/Lion, with or without the patch for bug 79068. The problem appears to be that the inspector thinks that the SVG rect in inspector/styles/svg-style-expected.txt appears on line 38 when it clearly appears on line 30 (which is what the expected.txt file expects). Since the test was only intended to verify that the the inspector doesn't crash, matching line numbers may be overkill. It turns out that this is a known bug - https://bugs.webkit.org/show_bug.cgi?id=72434
New test introduced in r110532 fails on Qt linux release and 32 bit builds: svg/transforms/transform-origin-css-property.xhtml
(In reply to comment #34) > New test introduced in r110532 fails on Qt linux release and 32 bit builds: > svg/transforms/transform-origin-css-property.xhtml Skip landed in r111367
Committed r114416: <http://trac.webkit.org/changeset/114416>