Bug 79068

Summary: SVG should support transform-origin and relative values
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, commit-queue, dglazkov, dino, eoconnor, giles_joplin, kadam, macpherson, menard, ossy, pnormand, schenney, simon.fraser, webkit.review.bot, zarvai, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70025    
Attachments:
Description Flags
Preview patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dirk Schulze 2012-02-20 19:49:50 PST
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.
Comment 1 Hans Muller 2012-03-05 12:21:09 PST
Created attachment 130180 [details]
Patch
Comment 2 Simon Fraser (smfr) 2012-03-05 12:48:13 PST
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.
Comment 3 Hans Muller 2012-03-05 20:20:37 PST
(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.
Comment 4 Hans Muller 2012-03-06 13:11:49 PST
Created attachment 130431 [details]
Patch
Comment 5 WebKit Review Bot 2012-03-06 13:15:32 PST
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.
Comment 6 Hans Muller 2012-03-06 13:39:13 PST
> 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 7 Simon Fraser (smfr) 2012-03-06 13:49:10 PST
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 8 WebKit Review Bot 2012-03-06 14:52:35 PST
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
Comment 9 Hans Muller 2012-03-06 16:29:19 PST
Created attachment 130466 [details]
Patch
Comment 10 Hans Muller 2012-03-06 16:47:28 PST
(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.
Comment 11 Hans Muller 2012-03-06 16:47:28 PST
(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.
Comment 12 WebKit Review Bot 2012-03-06 16:48:28 PST
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 13 Simon Fraser (smfr) 2012-03-06 17:24:29 PST
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 14 Nikolas Zimmermann 2012-03-07 05:10:19 PST
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.
Comment 15 Hans Muller 2012-03-08 11:19:14 PST
Created attachment 130861 [details]
Patch
Comment 16 Hans Muller 2012-03-08 11:20:38 PST
(In reply to comment #15)
Made all of the suggested changes and added a repaint test (svg/transforms/dynamic-transform-origin.svg).
Comment 17 Build Bot 2012-03-08 11:50:53 PST
Comment on attachment 130861 [details]
Patch

Attachment 130861 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11894040
Comment 18 WebKit Review Bot 2012-03-08 11:56:18 PST
Comment on attachment 130861 [details]
Patch

Attachment 130861 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11898021
Comment 19 Hans Muller 2012-03-08 12:44:05 PST
Created attachment 130882 [details]
Patch
Comment 20 Hans Muller 2012-03-08 12:44:59 PST
(In reply to comment #19)
Previous version of the patch was based on an obsolete version of Length.h.
Comment 21 WebKit Review Bot 2012-03-08 14:08:00 PST
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 22 Nikolas Zimmermann 2012-03-09 02:22:47 PST
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.
Comment 23 Hans Muller 2012-03-12 11:22:12 PDT
Created attachment 131360 [details]
Patch
Comment 24 Nikolas Zimmermann 2012-03-12 13:15:35 PDT
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.
Comment 25 Hans Muller 2012-03-12 16:05:47 PDT
Created attachment 131435 [details]
Patch
Comment 26 Dirk Schulze 2012-03-12 16:30:04 PDT
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 :)
Comment 27 Hans Muller 2012-03-12 16:44:35 PDT
Created attachment 131451 [details]
Patch
Comment 28 WebKit Review Bot 2012-03-12 19:45:55 PDT
Comment on attachment 131451 [details]
Patch

Clearing flags on attachment: 131451

Committed r110532: <http://trac.webkit.org/changeset/110532>
Comment 29 WebKit Review Bot 2012-03-12 19:46:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Ádám Kallai 2012-03-13 06:45:26 PDT
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
Comment 31 Ádám Kallai 2012-03-13 07:28:05 PDT
I skipped this test.

http://trac.webkit.org/changeset/110558
Comment 32 Hans Muller 2012-03-13 16:34:36 PDT
(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.
Comment 33 Hans Muller 2012-03-14 10:49:35 PDT
(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
Comment 34 Zoltan Arvai 2012-03-19 08:44:56 PDT
New test introduced in r110532 fails on Qt linux release and 32 bit builds:
svg/transforms/transform-origin-css-property.xhtml
Comment 35 Csaba Osztrogonác 2012-03-19 23:49:41 PDT
(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
Comment 36 Stephen Chenney 2012-04-17 12:34:10 PDT
Committed r114416: <http://trac.webkit.org/changeset/114416>