506856 - SVG: animVal readonly behaviour is incorrect

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

For SVGAnimatedLength, SVG 1.1 describes the animVal attribute as follows:

> readonly SVGLength animVal
>     If the given attribute or property is being animated, contains the current
>     animated value of the attribute or property, and both the object itself and
>     its contents are readonly. If the given attribute or property is not
>     currently being animated, contains the same value as 'baseVal'.

Based on my testing we our implementation fails in the following ways.

Expected behaviour:

a) Setting animVal to another SVGLength should always fail (by throwing a NO_MODIFICATION_ALLOWED_ERR DOMException) because it is a readonly property
b) Setting animVal.value should fail (by throwing a NO_MODIFICATION_ALLOWED_ERR DOMException) only when the length is being animated.

Actual behaviour:

a) Setting animValue never throws an exception.
b) Setting animValue.value always throws an exception even when the length is not animated.

Interoperability:

Opera 10b2, provides the expected behaviour for (a), but for (b) it never throws an exception.
WebKit July 09 trunk does the same as Firefox for (a). (b) hasn't been tested because the test case I've prepared relies on being able to add animation elements via the DOM which WebKit doesn't appear to support at this time.

Regarding the identity of the SVGLength objects returned from animVal, testing Opera and WebKit provides the following:

c) Subsequent requests for the animVal SVGLength object when the length is not animated the same object (i.e. testing with ===)
d) The SVGLength object returned by animVal when the length is not animated is a different object to the SVGLength object returned while the length is being animated. This seems reasonable in light of the prose quoted above.

Comparing this with Firefox, we return a different object for every request for animVal. Therefore we do not match the behaviour for (c) but we do for (d).

I propose we bring our behaviour into line with the spec and other implementations on the following 3 points:

i) Make setting animVal always throw the appropriate exception (this is (a) above)
ii) Make setting the contents of animVal throw the appropriate exception only when it is animated, i.e. (b) -- I haven't looked at how feasible this is to implement yet.
iii) Make subsequent calls to animVal return the same object in keeping with the behaviour of other implementations, i.e. (c)

I'd like to get this right for lengths so we can try to follow the same approach for numbers, etc. Lists, of course, may be quite a different story.

Test case to follow.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Cross-browser test case

Comment 2

[Cameron McCormack (:heycam)]

(In reply to comment #0)
> Expected behaviour:
> 
> a) Setting animVal to another SVGLength should always fail (by throwing a
> NO_MODIFICATION_ALLOWED_ERR DOMException) because it is a readonly property

I think the expected behaviour is to silently fail to set the value of the property.  That is generally how other readonly IDL attributes are treated.

> b) Setting animVal.value should fail (by throwing a NO_MODIFICATION_ALLOWED_ERR
> DOMException) only when the length is being animated.

Agreed with this one.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Updated liveness test case

Thanks Cameron! I've updated the test case. It turns out:

  Opera: throws NO_MODIFICATION_ALLOWED_ERR exception
  Firefox: throws TypeError ("setting a property that has only a getter")
  WebKit: fails silently

Is the behaviour you mentioned defined anywhere?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to comment #3)
> Thanks Cameron! I've updated the test case. It turns out:
> 
>   Opera: throws NO_MODIFICATION_ALLOWED_ERR exception
>   Firefox: throws TypeError ("setting a property that has only a getter")
>   WebKit: fails silently
> 
> Is the behaviour you mentioned defined anywhere?

http://dev.w3.org/2006/webapi/WebIDL/#host-objects says that the property must be ReadOnly (in the ECMAScript sense).  http://dev.w3.org/2006/webapi/WebIDL/#put then (which is invoked when assigning a property) will get to step 8 without doing any property setting.

Note though that Web IDL hasn't had detailed review yet and is still only a draft, so it is possible that these requirements will change.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Liveness test case

Thanks very much once again Cameron! It sounds like the Firefox behaviour is reasonable then for the time being, or at least, it's not something that is specific to animVals but the IDL implementation in general.

I've updated the test case so that it doesn't require an exception to be thrown.

Therefore, this bug only concerns:

* Make setting the contents of animVal throw the appropriate exception only
when it is animated. Currently this is hardwired to always throw the exception:

 http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.h#218

* Where possible, returning the same SVGLength object for subsequent calls to animVal in a manner consistent with other implementations (this is debatable so I'll wait for feedback on this). Also, more testing is required on this point regarding the liveness of such objects *during* animation.

Thanks Cameron!

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: More details readonly test

This is an additional test that focuses on when the animVal attribute is readonly to check that it is readonly when and only when it is animated (meaning when there's an active interval or it's frozen).

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1a

This patch addresses the issue of lengths being readonly. I think the object identity issue deserves a separate bug.

With this patch applied the second test cases passes. The first test case fails on the object identity test.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 392660 [details] [diff] [review]
patch v1a

Putting this up for review now that I've split the object identity issue off into a separate bug (bug 508496).

dholbert: Including you on this review because I'd just like to run my changes to the animation controller by you.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 392660 [details] [diff] [review]
patch v1a

(In reply to comment #8)
> dholbert: Including you on this review because I'd just like to run my changes
> to the animation controller by you.

Cool -- the AnimationController changes look good to me. r=dholbert on those.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Personally I think behaviours b) and c) are really bad. Script should behave consistently whether an attribute is animated or not, especially because animation is often triggered by timers and therefore from the script's point of view it's unpredictable.

c) is especially problematic. If you call .animVal before animation starts, hold onto the result while animation starts, and then query it, you're getting incorrect results. That's just wrong.

Does anyone agree with me? What leeway do we have?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So basically I don't like the part of the spec that this patch implements.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #10)
> Personally I think behaviours b) and c) are really bad. Script should behave
> consistently whether an attribute is animated or not, especially because
> animation is often triggered by timers and therefore from the script's point of
> view it's unpredictable.

Agreed. I cannot imagine a use case for setting animVal since the only times where it can be set, it is the same as baseVal. Except perhaps to catch the exception and thereby determine if the attribute is being animated?

I will query www-svg about this one.

> c) is especially problematic. If you call .animVal before animation starts,
> hold onto the result while animation starts, and then query it, you're getting
> incorrect results. That's just wrong.

I'm not sure about this. I think we're talking about the object identity issue here which I've split off into bug 508496. As far as *our* implementation goes, even though the DOM objects have different identities (!==) they should still return accurate results since they are just tear-offs. It's just that script that tries to compare the objects using === will think they're different objects. That seems problematic to me.

I'm not sure how other implementations work except that before animation animatedLength.animVal === animatedLength.animVal and after animation begins the objects differ in identity (but I haven't tested beyond that).

> Does anyone agree with me? What leeway do we have?

Will post to www-svg.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I misunderstood option c). Behaviour c) is fine, and we should adopt it in bug 508496.

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to comment #10)
> Personally I think behaviours b) and c) are really bad. Script should behave
> consistently whether an attribute is animated or not, especially because
> animation is often triggered by timers and therefore from the script's point of
> view it's unpredictable.

Query posted to www-svg: http://lists.w3.org/Archives/Public/www-svg/2009Aug/0015.html

(In reply to comment #13)
> Sorry, I misunderstood option c). Behaviour c) is fine, and we should adopt it
> in bug 508496.

Ok great, I take that to mean we should always return the same object (===) for animVal (and baseVal for that matter).

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes.

Except that if all references to the animVal/baseVal object are dropped, then it's OK (actually, good) to discard the object and allocate a new one next time animVal/baseVal are called. But that's observably equivalent anyway, since script can't detect the object identity change if it doesn't have a reference to the old object.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1b (testcase only)

It would appear that SVG1.1 SE will adopt the behaviour suggested by roc, i.e. animVal and its contents are ALWAYS readonly.

See updated wording in SVG1.1 SE:
http://dev.w3.org/SVG/profiles/1.1F2/publish/types.html#InterfaceSVGAnimatedLength

And discussion on www-svg:
http://lists.w3.org/Archives/Public/www-svg/2009Aug/0025.html
http://lists.w3.org/Archives/Public/www-svg/2009Aug/0026.html
http://www.w3.org/2009/08/12-svg-minutes.html#item04

Therefore our current behaviour is correct.

As a result I've removed all the code changes from this patch and updated the test case to check that animatedLength.animVal is always readonly. Is it still useful to add this test case?

The previous patch included optimisations to the compositor which are still useful so I'll file a separate bug for that.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 394190 [details] [diff] [review]
patch v1b (testcase only)

doesn't need sr. Doesn't even really need review :-)

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1c (testcase only)

Fix bitrot. Remove use of querying prefs.

Comment 19

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Patch v1c applies on top of the patch for bug 508496 (both modify Makefile.in, that's all).

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: patch v1e

Update code for testing SMIL.

Comment 21

[Jonathan Watt [:jwatt]]

Pushed http://hg.mozilla.org/mozilla-central/rev/5d3fbeea0294

Comment 22

[Jonathan Watt [:jwatt]]

I had to remove the test from the Makefile.in due to test failure or one of the linux boxes. Seems to be a rounding issue.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1251360093.1251365884.23631.gz

Comment 23

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Fix for failing test case

This should fix the test case that was failing on Linux. The tests pass on TryServer but I'm not sure if it's using svg.smil.enabled. Anyway, I managed to repro the problem on my machine by changing the dpi to 101 and this patched fixed it. So it should be good.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checkin-needed?

Comment 25

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yes please!

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/a8e72104670d

Not sure whether that means this bug is fixed now, though.

Comment 27

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yes, this is fixed. In the end we got the spec changed rather than changing our behaviour.

http://dev.w3.org/SVG/profiles/1.1F2/publish/types.html#InterfaceSVGAnimatedLength