606399 - SVG SMIL: Animating rect rx or ry only should animate corresponding attribute

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Attachment: Test case

Regarding <rect> rounding, SVG says:

  If a properly specified value is provided for ‘rx’ but not for ‘ry’, then the user agent processes the ‘rect’ element with the effective value for ‘ry’ as equal to ‘rx’. If a properly specified value is provided for ‘ry’ but not for ‘rx’, then the user agent processes the ‘rect’ element with the effective value for ‘rx’ as equal to ‘ry’.
http://www.w3.org/TR/SVG/shapes.html#RectElement

We seem to do that too, but not when we animate. If one specifies neither attribute, but animates, say, 'rx' you'd expect the rectangle's rounding to be animated as per the behaviour described above. However, it's not. It seems you need to animate BOTH 'rx' and 'ry' to get the desired effect. Perhaps ry isn't taking on rx's animated value and is assuming the value of 0 thus suppressing any visual effect?

See the attached test case. There's no animation.

I came across this when testing: http://upload.wikimedia.org/wikibooks/de/7/79/SVG-Animation_Beispiel01.svg (but DON'T load that URL, it'll currently crash until bug 602880 is fixed.)

CC'ing a few folk here in case this is a dupe.

Comment 1

[Robert Longson [:longsonr]]

We really need to remove most HasAttr calls from layout and content except for those few that are on non-animated attributes.

Comment 2

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

Nominating this as a blocker.

The German wikibooks entry describing SVG animation presents one main example of SVG animation:

http://de.wikibooks.org/wiki/SVG/_Animation
http://de.wikibooks.org/wiki/Media:SVG-Animation_Beispiel01.svg

This file fails pretty badly in our current builds due to:
* Bug 602880 -- it crashes -- blocking-final and hopefully fixed soon
* Bug 594198 -- CSS zero values without units treated differently -- blocking-final
* Bug 436296 -- we don't support animateColor. Nothing likely to change here.
* This bug

From what I understand this bug mostly just requires changing the way layout fetches values from content. Judging from Robert's comment, comment 1, there are probably a few other places this same sort of bug occurs.

Comment 3

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

Assigning to me for now.

Filed bug 608161 to track all other instances of this category of bug (i.e.
using HasAttr on animated attributes inside content & layout).

Comment 4

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

Attachment: Patch v1a

Proposed patch to fix this.

One issue of behaviour, which may be worth clarifying with www-svg, is that SVG says:

If a properly specified value is provided for ‘rx’ but not for ‘ry’, then the user agent processes the ‘rect’ element with the effective value for ‘ry’ as equal to ‘rx’.

i.e. it talks about values being "specified". I've understood this include setting a value via the DOM using the SVGLength DOM interface. So if I set the rx length using the DOM but not ry, ry takes on the rx value. Opera and WebKit agree this far.

One issue though is that once specified in this way, it's not really possible to 'unspecify' the value. With attribute setting, you can just remove the attribute but with the DOM interface there's no 'reset' so far as I can tell. Attempting to set the attribute to something bogus or calling removeAttribute will probably do the trick but this does seem a bit hacky.

On this point Opera and WebKit differ. If you set the value to 0 in Opera it will treat it as if it hasn't been set, whereas WebKit treats it as still being set. I've matched WebKit's behaviour here since that makes more sense to me.

Comment 5

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

Me too. I think your analysis is correct. Absence of 'reset' in the DOM API is probably not a big deal.

Comment 6

[Robert Longson [:longsonr]]

Shouldn't removeAttribute set mIsBaseSet to false though?

Comment 7

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

Comment on attachment 489372 [details] [diff] [review]
Patch v1a

   PRPackedBool mIsAnimated;
+  PRPackedBool mIsBaseSet;

Make these a :1 bitfield so the object doesn't grow by 4 bytes.

Comment 8

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

Not blocking on this, but we'll definitely take patches like this.

Comment 9

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

Oh, Robert's right in comment #6 of course! We'll need that too.

Comment 10

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

(In reply to comment #6)
> Shouldn't removeAttribute set mIsBaseSet to false though?

Yes, it does that. (Since UnsetAttr calls LengthAttributesInfo::Reset, which calls nsSVGLength2::Init where we set mIsBaseSet to false). The test case element id="b" tests this.

My point is just that it's not particularly intuitive for a content author because there doesn't appear to be any way to achieve this effect using the SVGLength interface. You have to think a bit laterally. But like roc said, it's probably not a big deal.

My bad, I said "DOM interface" meaning the SVG-specific DOM interfaces as opposed to the generic XML DOM interfaces.

Comment 11

[Robert Longson [:longsonr]]

(In reply to comment #10)
> (In reply to comment #6)
> > Shouldn't removeAttribute set mIsBaseSet to false though?
> 
> Yes, it does that. (Since UnsetAttr calls LengthAttributesInfo::Reset, which
> calls nsSVGLength2::Init where we set mIsBaseSet to false). The test case
> element id="b" tests this.

I just realised that ;-)

Comment 12

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

Attachment: Patch v1b, r=roc, a=roc

Address review feedback (comment 7).

Comment 13

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

Not landing yet, got two test failures on Windows reftest on Try. There seem to be a few pixels out on a couple of the "circles" even when the markup is identical between the test and reference file.

Comment 14

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

Attachment: Patch v1c; r=roc; a=roc

Updated patch to fix reftest failures on Windows.

The problem seems to be that the presence of animation was causing some minor differences in the anti-aliasing between the stroke and the file. This was happening ever for un-animated content where the markup was identical between test and reference.

As discussed with roc on IRC, it may be a layers issue where some of the content was getting promoted to a layer due to the nearby animation (or something of that nature anyway, I don't know that part of the code). Simply pausing and seeking the timeline as we do for most tests however seems to put us in a stable state and mitigates this issue.

Running the updated patch through Try once more and unless anyone objects this is the version I'll land.

Carrying forward r,a=roc

Comment 15

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

Landed: http://hg.mozilla.org/mozilla-central/rev/b60a960a76be

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> Not landing yet, got two test failures on Windows reftest on Try. There seem to
> be a few pixels out on a couple of the "circles" even when the markup is
> identical between the test and reference file.

I don't think this ended up being fixed in the final patch here -- it may have just looked like it was fixed on tryserver, due to sporadic-ness.  Bug 616516 seems to be about the exact same problem described in comment 13 / comment 14 here.  (See in particular Bug 616516 comment 1, which sounds like the exact same type of reftest mismatch that birtles describes in comment 13 here.)

Comment 17

[Daniel Holbert [:dholbert]]

(also, the test in question -- anim-rect-rxry-1.svg -- doesn't actually need its reftest-wait magic, since in its current state,  it doesn't actually do anything in an onload handler)

Comment 18

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

(In reply to comment #16)
> I don't think this ended up being fixed in the final patch here -- it may have
> just looked like it was fixed on tryserver, due to sporadic-ness.  Bug 616516
> seems to be about the exact same problem described in comment 13 / comment 14
> here.  (See in particular Bug 616516 comment 1, which sounds like the exact
> same type of reftest mismatch that birtles describes in comment 13 here.)

Ok. Let's follow it up in bug 616516 then. The changes here to force a sample and wait at least make the test fail less frequently. Previously it used to always fail on Try for windows builds but never on my local windows machine.