Bug 137942 - Fix animation of orient attribute on marker element
Summary: Fix animation of orient attribute on marker element
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified All
: P2 Normal
Assignee: Nikos Andronikos
Depends on: 138416
Blocks: 138456
  Show dependency treegraph
Reported: 2014-10-21 20:02 PDT by Nikos Andronikos
Modified: 2014-11-06 00:02 PST (History)
10 users (show)

See Also:

Patch (13.83 KB, patch)
2014-10-21 20:47 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (13.80 KB, patch)
2014-10-28 18:03 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2014-10-29 10:17 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2014-10-31 15:49 PDT, Nikos Andronikos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikos Andronikos 2014-10-21 20:02:30 PDT
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:
Comment 1 Nikos Andronikos 2014-10-21 20:47:54 PDT
Created attachment 240242 [details]
Comment 2 Nikos Andronikos 2014-10-23 18:56:26 PDT
Currently in the process of trying to upload a new patch but webkit-patch is throwing an exception. Watch this space...
Comment 3 Nikos Andronikos 2014-10-28 18:03:13 PDT
Created attachment 240585 [details]
Comment 4 WebKit Commit Bot 2014-10-28 18:06:36 PDT
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.
Comment 5 Nikos Andronikos 2014-10-29 10:17:19 PDT
Created attachment 240609 [details]
Comment 6 Dirk Schulze 2014-10-29 10:53:05 PDT
Comment on attachment 240609 [details]

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.
Comment 7 Nikos Andronikos 2014-10-31 15:49:44 PDT
Created attachment 240766 [details]
Comment 8 Nikos Andronikos 2014-10-31 15:53:32 PDT
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.
Comment 9 Dirk Schulze 2014-11-04 00:40:58 PST
Comment on attachment 240766 [details]

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


> 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.
Comment 10 WebKit Commit Bot 2014-11-04 01:20:47 PST
Comment on attachment 240766 [details]

Clearing flags on attachment: 240766

Committed r175525: <http://trac.webkit.org/changeset/175525>
Comment 11 WebKit Commit Bot 2014-11-04 01:20:53 PST
All reviewed patches have been landed.  Closing bug.