Bug 137942 – Fix animation of orient attribute on marker element

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

[Nikos Andronikos]

There are two issues with the animation of the orient attribute:

See http://jsfiddle.net/dodgeyhack/fajLbp48/8/ for a test

1. Animation logic from 'auto' to an angle is missing.
2. The animated value of orientType is not used within the renderer so although the animation is running and is reflected in the DOM, it is not rendered.
This affects animations that have a differing type for the initial value and the animated from/to values.
Current tests (listed below) just query the DOM via JS so have not picked this up.

Current tests:
LayoutTests/svg/animations/animate-marker-orient-from-angle-to-angle.html
LayoutTests/svg/animations/animate-marker-orient-from-angle-to-auto.html
LayoutTests/svg/animations/animate-marker-orient-to-angle.html

[Nikos Andronikos]

Created attachment 240242 [details]
Patch

[Nikos Andronikos]

Currently in the process of trying to upload a new patch but webkit-patch is throwing an exception. Watch this space...

[Nikos Andronikos]

Created attachment 240585 [details]
Patch

[WebKit Commit Bot]

Attachment 240585 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikos Andronikos]

Created attachment 240609 [details]
Patch

[Dirk Schulze]

Comment on attachment 240609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240609&action=review

Patch looks great in general. Just some snippets.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:99
> +        // Discrete animation - no linear interpolation possible between values (e.g. auto to angle)

Period at the end.

> Source/WebCore/svg/SVGAnimatedAngle.cpp:106
> +        } else {

add a return here and don't check for else.

> LayoutTests/svg/animations/animate-marker-orienttype-2.html:21
> +<script>
> +if (testRunner) {
> +    testRunner.waitUntilDone();
> +    window.setTimeout(function() {testRunner.notifyDone()}, 1500);
> +}
> +</script>
> +<svg xmlns="http://www.w3.org/2000/svg" viewbox="0 0 150 150" width="500" height="500">
> +    <defs>
> +        <marker id="arrow" orient="auto" markerWidth="10" markerHeight="10" style="overflow:visible">
> +            <path d="M5,0 L 0,-5 L0,5 z" fill="green" opacity="0.5" />
> +            <animate attributeName="orient" from="auto" to="80" begin="0s" dur="1s" fill="freeze"/>
> +        </marker>
> +    </defs>
> +    <path d="M 20,20 L 80,80" marker-start="url(#arrow)" stroke="black"/>
> +</svg>

We use a different kind of tests for animations. Please take a look at the other tests in the same folder. Especially, take a look at the script-test folder and create a js file there. Then create a boiler plate file in the svg/animation folder. The animation tests test different values during the animation (so if the SVG DOM returns the right values) as well as rendering and repainting.

[Nikos Andronikos]

Created attachment 240766 [details]
Patch

[Nikos Andronikos]

As discussed with Dirk: The pixel test portion of repaint tests has been deprecated. Therefore these tests aren't appropriate here (repaint tests wouldn't show orientation of the marker). We're not looking to verify the animation at various time points - that is handled by the js script animation tests which exist. These tests are verifying that the animated value is used when rendering.

[Dirk Schulze]

Comment on attachment 240766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240766&action=review

r=me

> Source/WebCore/svg/SVGAnimatedAngle.cpp:113
> +        if (toAngleAndEnumeration.second == SVGMarkerOrientAngle)
> +            animatedAngleAndEnumeration.first = toAngleAndEnumeration.first;
> +        else
> +            animatedAngleAndEnumeration.first.setValue(0);
> +        return;

We use early returns if there is a return right after the else. Can be fixed later though.

[WebKit Commit Bot]

Comment on attachment 240766 [details]
Patch

Clearing flags on attachment: 240766

Committed r175525: <http://trac.webkit.org/changeset/175525>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.