Closed Bug 606399 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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.
We really need to remove most HasAttr calls from layout and content except for those few that are on non-animated attributes.
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.
blocking2.0: --- → ?
Blocks: 608161
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).
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Attached patch Patch v1a (obsolete) — Splinter Review
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.
Attachment #489372 - Flags: review?(dholbert)
Me too. I think your analysis is correct. Absence of 'reset' in the DOM API is probably not a big deal.
Shouldn't removeAttribute set mIsBaseSet to false though?
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.
Attachment #489372 - Flags: review?(dholbert)
Attachment #489372 - Flags: review+
Attachment #489372 - Flags: approval2.0+
Not blocking on this, but we'll definitely take patches like this.
blocking2.0: ? → -
Oh, Robert's right in comment #6 of course! We'll need that too.
(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.
(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 ;-)
blocking2.0: - → ?
Attached patch Patch v1b, r=roc, a=roc (obsolete) — Splinter Review
Address review feedback (comment 7).
Attachment #489372 - Attachment is obsolete: true
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.
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
Attachment #489379 - Attachment is obsolete: true
Attachment #490854 - Flags: review+
Landed: http://hg.mozilla.org/mozilla-central/rev/b60a960a76be
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
(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.)
(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)
(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.
You need to log in before you can comment on or make changes to this bug.