Bug 21371

Summary: Cannot animate "points" attribute for <svg:polygon>
Product: WebKit Reporter: Antoine Quint <ml>
Component: SVGAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.biryukoff, commit-queue, dglazkov, d-r, eric, esprehn+autocc, fmalita, freddy, glenn, graouts, gyuyoung.kim, jeffschiller, koivisto, kondapallykalyan, krit, pdr, rniwa, schenney, s.hollenshead, webkit-bug-importer, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Testcase
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Antoine Quint 2008-10-04 14:32:52 PDT
I've tried animating the points of a polygon but it does not work.
Comment 1 Antoine Quint 2008-10-04 14:35:41 PDT
Created attachment 24093 [details]
Testcase
Comment 2 Dirk Schulze 2010-03-28 05:42:16 PDT
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
Comment 3 Dirk Schulze 2010-03-28 08:26:13 PDT
Created attachment 51861 [details]
Patch
Comment 4 WebKit Review Bot 2010-03-28 23:58:54 PDT
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.
Comment 5 Dirk Schulze 2010-03-29 00:31:10 PDT
(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.
Comment 6 WebKit Review Bot 2010-03-29 00:57:44 PDT
Attachment 51861 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1611047
Comment 7 Dirk Schulze 2010-03-29 01:46:27 PDT
Created attachment 51885 [details]
Patch

with build-fix for chromium
Comment 8 WebKit Review Bot 2010-03-29 01:51:32 PDT
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.
Comment 9 Nikolas Zimmermann 2010-03-30 01:35:03 PDT
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.
Comment 10 Eric Seidel (no email) 2010-04-06 23:46:33 PDT
Attachment 51885 [details] was posted by a committer and has review+, assigning to Dirk Schulze for commit.
Comment 11 Eric Seidel (no email) 2010-05-17 00:42:35 PDT
Unsure of the status of this patch.  It's been in pending-commit for over a month.  Updates?
Comment 12 Dirk Schulze 2010-05-17 01:24:18 PDT
Landed in http://trac.webkit.org/changeset/56795.
Comment 13 Freddy Snijder 2012-08-29 16:34:23 PDT
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?
Comment 14 Dirk Schulze 2012-08-29 17:58:34 PDT
The code base changed a lot since the landing. Niko? Philip? Any ideas?
Comment 15 Alexander Biryukov 2013-03-03 08:16:58 PST
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?
Comment 16 Sawyer Hollenshead 2013-11-19 11:09:17 PST
This is broken once again in latest Chrome (31.0.1650.57) and Safari 7. Works fine in Firefox 25.0.1.
Comment 17 Antoine Quint 2013-11-19 12:05:27 PST
Reopening since it's broken in Safari 7 on Mavericks.
Comment 18 Antoine Quint 2013-11-20 01:38:15 PST
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.
Comment 19 Antoine Quint 2013-11-20 02:44:52 PST
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.
Comment 20 Antoine Quint 2013-11-20 05:16:41 PST
Created attachment 217421 [details]
Patch
Comment 21 Antti Koivisto 2013-11-20 05:38:24 PST
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.
Comment 22 Antoine Quint 2013-11-20 05:45:34 PST
(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.
Comment 23 Antoine Quint 2013-11-20 06:08:27 PST
Created attachment 217422 [details]
Patch for landing
Comment 24 WebKit Commit Bot 2013-11-20 06:53:05 PST
Comment on attachment 217422 [details]
Patch for landing

Clearing flags on attachment: 217422

Committed r159559: <http://trac.webkit.org/changeset/159559>
Comment 25 WebKit Commit Bot 2013-11-20 06:53:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Sawyer Hollenshead 2013-11-20 07:28:33 PST
Awesome, thanks everyone!
Comment 28 Antoine Quint 2013-11-21 09:35:40 PST
(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.
Comment 29 Antoine Quint 2013-11-21 10:08:57 PST
(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.
Comment 30 Antoine Quint 2013-12-20 00:20:44 PST
*** Bug 126050 has been marked as a duplicate of this bug. ***