Bug 21371 – Cannot animate "points" attribute for <svg:polygon>

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

[Antoine Quint]

I've tried animating the points of a polygon but it does not work.

[Antoine Quint]

Created attachment 24093 [details]
Testcase

[Dirk Schulze]

We have to differentiate between two issues on your example. The first one is the wrong handling on missing 'from' attributes. The second one is the animation itself. I take this bug to handle the animation problem. See also W3C test: http://www.w3.org/Graphics/SVG/Test/20061213/svggen/animate-elem-34-t.svg

[Dirk Schulze]

Created attachment 51861 [details]
Patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGAnimateElement.cpp:149:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


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

[Dirk Schulze]

(In reply to comment #4)
> Attachment 51861 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/svg/SVGAnimateElement.cpp:149:  Tests for true/false, null/non-null,
> and zero/non-zero should all be done without equality comparisons. 
> [readability/comparison_to_zero] [5]
> Total errors found: 1 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

The value can be negative, so ... == 0 check is ok here.

[WebKit Review Bot]

Attachment 51861 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1611047

[Dirk Schulze]

Created attachment 51885 [details]
Patch

with build-fix for chromium

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGAnimateElement.cpp:149:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files


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

[Nikolas Zimmermann]

Comment on attachment 51885 [details]
Patch

Nice patch, some small comments before landing:

> Index: WebCore/svg/SVGPathSegList.h
> ===================================================================
> --- WebCore/svg/SVGPathSegList.h	(revision 56693)
> +++ WebCore/svg/SVGPathSegList.h	(working copy)
> @@ -45,6 +45,8 @@ namespace WebCore {
>          SVGPathSegList(const QualifiedName&);
>      };
>  
> +    float blendFunc(float from, float to, float progress);

Can you rename this function to something more descriptive?



> Index: WebCore/svg/SVGPointList.cpp

> +        const FloatPoint from = fromList->getItem(n, ec);
> +        const FloatPoint to = toList->getItem(n, ec);
Can't you omit const here?

Excellent testcase, r=me.

[Eric Seidel (no email)]

Attachment 51885 [details] was posted by a committer and has review+, assigning to Dirk Schulze for commit.

[Eric Seidel (no email)]

Unsure of the status of this patch.  It's been in pending-commit for over a month.  Updates?

[Dirk Schulze]

Landed in http://trac.webkit.org/changeset/56795.

[Freddy Snijder]

The test case for this bug doesn't work in Chrome Version 21.0.1180.82 or Safari Version 6.0 (8536.25). Isn't the fix incorporated in the latest Safari & Chrome releases?

[Dirk Schulze]

The code base changed a lot since the landing. Niko? Philip? Any ideas?

[Alexander Biryukov]

Hi guys. Do you have any updates about the last two comments?
I tried it in Chrome 25.0.1364.99 (Mac OS) and Safari 6.0.2 (Mac OS). 
May be better to reopen the bug?

[Sawyer Hollenshead]

This is broken once again in latest Chrome (31.0.1650.57) and Safari 7. Works fine in Firefox 25.0.1.

[Antoine Quint]

Reopening since it's broken in Safari 7 on Mavericks.

[Antoine Quint]

I did a little poking around yesterday and found that the SVGPointsList gets animated correctly. It seems the issue is at the painting level, either the dirty rect is not correct or the animated path is not obtained.

[Antoine Quint]

It doesn't seem like the right list of points is retrieved when updatePathFromPolygonElement() is called. We always get the base list instead of the animated list.

[Antoine Quint]

Created attachment 217421 [details]
Patch

[Antti Koivisto]

Comment on attachment 217421 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGPathData.cpp:77
>  static void updatePathFromPolygonElement(SVGElement* element, Path& path)

You could modernize this to take SVGElement& since you are here (the argument can't be null).

> Source/WebCore/rendering/svg/SVGPathData.cpp:92
>  static void updatePathFromPolylineElement(SVGElement* element, Path& path)

Here too.

> LayoutTests/svg/animations/polyline-set-expected.txt:6
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderSVGRoot {svg} at (10,0) size 210x220
> +    RenderSVGPath {polyline} at (10,0) size 210x220 [stroke={[type=SOLID] [color=#FF0000] [stroke width=20.00]}] [points="10 10 210 10 210 210 10 210"]
> +    RenderSVGPath {polyline} at (10,0) size 210x220 [stroke={[type=SOLID] [color=#008000] [stroke width=20.00]}] [points="10 10 110 10 110 110 10 110"]

Could you make a reftest instead? Render tree dumps are fragile.

[Antoine Quint]

(In reply to comment #21)
> (From update of attachment 217421 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217421&action=review
> 
> > Source/WebCore/rendering/svg/SVGPathData.cpp:77
> >  static void updatePathFromPolygonElement(SVGElement* element, Path& path)
> 
> You could modernize this to take SVGElement& since you are here (the argument can't be null).
> 
> > Source/WebCore/rendering/svg/SVGPathData.cpp:92
> >  static void updatePathFromPolylineElement(SVGElement* element, Path& path)
> 
> Here too.
> 
> > LayoutTests/svg/animations/polyline-set-expected.txt:6
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x600
> > +  RenderSVGRoot {svg} at (10,0) size 210x220
> > +    RenderSVGPath {polyline} at (10,0) size 210x220 [stroke={[type=SOLID] [color=#FF0000] [stroke width=20.00]}] [points="10 10 210 10 210 210 10 210"]
> > +    RenderSVGPath {polyline} at (10,0) size 210x220 [stroke={[type=SOLID] [color=#008000] [stroke width=20.00]}] [points="10 10 110 10 110 110 10 110"]
> 
> Could you make a reftest instead? Render tree dumps are fragile.

No problem, I'll make those changes as I land, thanks for the review.

[Antoine Quint]

Created attachment 217422 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 217422 [details]
Patch for landing

Clearing flags on attachment: 217422

Committed r159559: <http://trac.webkit.org/changeset/159559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sawyer Hollenshead]

Awesome, thanks everyone!

[Eric Seidel (no email)]

FYI, this new test is super-flaky for Blink.  We merged your change in:
https://codereview.chromium.org/79063004/

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20Blink&showAllRuns=true&tests=fast%2Fjs%2Fglobal-constructors.html%2Cfast%2Fpreloader%2Fdocument-write-noscript.html%2Csvg%2Fanimations%2Fpolygon-set.svg%2Cwebaudio%2Foscillator-sine.html

I suspect it may be flaky for WebKit as well.

[Antoine Quint]

(In reply to comment #27)
> FYI, this new test is super-flaky for Blink.  We merged your change in:
> https://codereview.chromium.org/79063004/
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20Blink&showAllRuns=true&tests=fast%2Fjs%2Fglobal-constructors.html%2Cfast%2Fpreloader%2Fdocument-write-noscript.html%2Csvg%2Fanimations%2Fpolygon-set.svg%2Cwebaudio%2Foscillator-sine.html
> 
> I suspect it may be flaky for WebKit as well.

Eric, you already had a fix for a similar bug with https://codereview.chromium.org/55893002/ (fix by Erik Dahlström). This just changed the code a little bit but should have the same results. In fact now I think you have dead code since pointsCurrentValue is not longer in use.

I'm not sure if this test is flaky with WebKit, but I'll look into it.

[Antoine Quint]

(In reply to comment #28)
> I'm not sure if this test is flaky with WebKit, but I'll look into it.

I'm told these two tests have not caused flakiness since they landed.

[Antoine Quint]

*** Bug 126050 has been marked as a duplicate of this bug. ***