Bug 79068 – SVG should support transform-origin and relative values

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

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.

[Hans Muller]

Created attachment 130180 [details]
Patch

[Simon Fraser (smfr)]

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.

[Hans Muller]

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

[Hans Muller]

Created attachment 130431 [details]
Patch

[WebKit Review Bot]

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.

[Hans Muller]

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

[Simon Fraser (smfr)]

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.

[WebKit Review Bot]

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

[Hans Muller]

Created attachment 130466 [details]
Patch

[Hans Muller]

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

[Hans Muller]

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

[WebKit Review Bot]

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.

[Simon Fraser (smfr)]

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&

[Nikolas Zimmermann]

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.

[Hans Muller]

Created attachment 130861 [details]
Patch

[Hans Muller]

(In reply to comment #15)
Made all of the suggested changes and added a repaint test (svg/transforms/dynamic-transform-origin.svg).

[Build Bot]

Comment on attachment 130861 [details]
Patch

Attachment 130861 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11894040

[WebKit Review Bot]

Comment on attachment 130861 [details]
Patch

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

[Hans Muller]

Created attachment 130882 [details]
Patch

[Hans Muller]

(In reply to comment #19)
Previous version of the patch was based on an obsolete version of Length.h.

[WebKit Review Bot]

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

[Nikolas Zimmermann]

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.

[Hans Muller]

Created attachment 131360 [details]
Patch

[Nikolas Zimmermann]

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.

[Hans Muller]

Created attachment 131435 [details]
Patch

[Dirk Schulze]

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

[Hans Muller]

Created attachment 131451 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131451 [details]
Patch

Clearing flags on attachment: 131451

Committed r110532: <http://trac.webkit.org/changeset/110532>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ádám Kallai]

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

[Ádám Kallai]

I skipped this test.

http://trac.webkit.org/changeset/110558

[Hans Muller]

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

[Hans Muller]

(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

[Zoltan Arvai]

New test introduced in r110532 fails on Qt linux release and 32 bit builds:
svg/transforms/transform-origin-css-property.xhtml

[Csaba Osztrogonác]

(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

[Stephen Chenney]

Committed r114416: <http://trac.webkit.org/changeset/114416>