Last Comment Bug 652832 - Dynamic changes to <use> width/height aren't honored, for special-cased <svg>/<symbol> targets
: Dynamic changes to <use> width/height aren't honored, for special-cased <svg>...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Robert Longson
:
Mentors:
Depends on:
Blocks: 608161 648875 667064 682685
  Show dependency treegraph
 
Reported: 2011-04-26 09:41 PDT by Dr. Olaf Hoffmann
Modified: 2011-09-20 05:00 PDT (History)
6 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
SVG with animated use width and height referencing an svg (1.08 KB, image/svg+xml)
2011-04-26 09:42 PDT, Dr. Olaf Hoffmann
no flags Details
SVG with animated use width and height referencing a symbol (1.08 KB, image/svg+xml)
2011-04-26 09:43 PDT, Dr. Olaf Hoffmann
no flags Details
SVG with animated use width and height referencing a g [should not animate] (1.21 KB, image/svg+xml)
2011-04-26 09:44 PDT, Dr. Olaf Hoffmann
no flags Details
scripted testcase (1.30 KB, image/svg+xml)
2011-04-26 11:27 PDT, Daniel Holbert [:dholbert]
no flags Details
patch (15.22 KB, patch)
2011-06-12 03:30 PDT, Robert Longson
jwatt: review-
Details | Diff | Review
address review comments (17.38 KB, patch)
2011-06-17 08:09 PDT, Robert Longson
jwatt: review-
Details | Diff | Review
updated patch (17.94 KB, patch)
2011-06-17 12:48 PDT, Robert Longson
jwatt: review+
Details | Diff | Review
hg changeset patch with final review comments addressed (13.65 KB, patch)
2011-06-20 13:18 PDT, Robert Longson
no flags Details | Diff | Review

Description Dr. Olaf Hoffmann 2011-04-26 09:41:17 PDT
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110426 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110426 Firefox/6.0a1

The attributes width and height of the use element do not animate.

Samples for referenced svg, symbol and g will be attached.

See bug 648875 as well.

Reproducible: Always

Steps to Reproduce:
1. Try provided samples
2. Compare with source code, recommendations
3. Compare with behaviour of other viewers like Opera, Batik/Squiggle, adobe plugin ...

Actual Results:  
No animation.

Expected Results:  
Animation of width and height of use in case this applies for the referenced
element.
Comment 1 Dr. Olaf Hoffmann 2011-04-26 09:42:28 PDT
Created attachment 528329 [details]
SVG with animated use width and height referencing an svg
Comment 2 Dr. Olaf Hoffmann 2011-04-26 09:43:46 PDT
Created attachment 528330 [details]
SVG with animated use width and height referencing a symbol
Comment 3 Dr. Olaf Hoffmann 2011-04-26 09:44:43 PDT
Created attachment 528331 [details]
SVG with animated use width and height referencing a g [should not animate]
Comment 4 Daniel Holbert [:dholbert] 2011-04-26 11:02:08 PDT
Comment on attachment 528331 [details]
SVG with animated use width and height referencing a g [should not animate]

just to be clear - <symbol> and <svg> are special-cases for <use>, as described here:  http://www.w3.org/TR/SVG11/struct.html#UseElement

In those cases, the <use>'s width & height attributes are supposed to be transferred through to the clone of the target element.

For all other target elements (including <g>), width & height aren't passed through like that.  So our rendering of the "<g>" testcase here (with no visible animation) is correct, and matches what Opera does.
Comment 5 Daniel Holbert [:dholbert] 2011-04-26 11:13:21 PDT
(In reply to comment #4)
> just to be clear - <symbol> and <svg> are special-cases for <use>, as described
> here:  http://www.w3.org/TR/SVG11/struct.html#UseElement

It looks like we honor this special-casing for the initial rendering, but dynamic changes after that aren't honored (whether scripted or animated).  See upcoming testcase.
Comment 6 Daniel Holbert [:dholbert] 2011-04-26 11:27:26 PDT
Created attachment 528365 [details]
scripted testcase

Here's a testcase with just script (no animation).  It has one <use> element targeting an <svg>, and another <use> targeting a <symbol>.

The testcase tweaks the <use> elements' widths (stretching what would otherwise be perfect circles) both in the onload handler and on any subsequent clicks.

My Firefox nightly honors the onload tweak (shows stretched circles) about 80% of the time -- whereas in the other 20%, it shows 2 perfect circles, presumably because they finished rendering before we got to the onload handler).

Firefox's rendering does *not* honor any onclick tweaks, though the attribute is being set, as can be seen by opening Tools | Web Console and typing
> document.getElementsByTagName("use")[0].getAttribute("width")

In contrast, Opera seems to honor the onload tweak as well as all click tweaks.
Comment 7 Robert Longson 2011-06-12 03:30:55 PDT
Created attachment 538742 [details] [diff] [review]
patch

Also fixes bug 648875
Comment 8 Robert Longson 2011-06-12 03:42:06 PDT
BTW all those comments in the various SetBaseValueString implementations assume that DidChangeXXX won't need to be overridden. There are other overrides in nsSVGMarkerElement that may not work properly in the face of setAttribute changes but that's another bug.
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-12 17:07:51 PDT
Comment on attachment 538742 [details] [diff] [review]
patch

Bad dimensions are only one way in which the element could be invalid, so can you change 'IsValid' to 'HasValidDimensions' or something?

I think CopyLength would be better named 'SetLength'. Also the implementation of this method should NS_ABORT_IF_FALSE that the length is found.

Can you remove the DEBUG_tor code from nsSVGUseElement::CreateAnonymousContent while you're there?

>   if (symbol || svg) {
>-    if (HasAttr(kNameSpaceID_None, nsGkAtoms::width)) {
>-      nsAutoString width;
>-      GetAttr(kNameSpaceID_None, nsGkAtoms::width, width);
>-      newcontent->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, PR_FALSE);
>-    }
>+    nsSVGElement *newElement = static_cast<nsSVGElement*>(newcontent.get());
> 
>-    if (HasAttr(kNameSpaceID_None, nsGkAtoms::height)) {
>-      nsAutoString height;
>-      GetAttr(kNameSpaceID_None, nsGkAtoms::height, height);
>-      newcontent->SetAttr(kNameSpaceID_None, nsGkAtoms::height, height, PR_FALSE);
>-    }
>+    newElement->CopyLength(nsGkAtoms::width, mLengthAttributes[WIDTH]);
>+    newElement->CopyLength(nsGkAtoms::height, mLengthAttributes[HEIGHT]);

This should only be happening if IsExplicitlySet() is true for the respective lengths, right?

It would seem best not to have the test depend on correct style inheritance - that should be a separate test. Can you just set the fill on the <rect>s that are being used?

Regarding use-01.svg, can you change the <rect>s to be 100 x 100 since 400 seems unnecessarily large and I'm concerned about all the relevant parts of the test being onscreen on small devices.

I think the dynamic-use tests should wait for MozReftestInvalidate - that may be unnecessary to test invalidation in our current implementation, but the implementation may change in future, and it may not be true of other implementations.

Actually, I don't understand how dynamic-use-04.svg works. How does setting the width to 1 fill the viewport?

In anim-use-length-01.svg I'm not sure that the first <use> is useful, since if the second behaves correctly then the first is covered and redundant and isn't actually testing anything. Maybe just remove that and make the background red?

The last closing </use> in anim-use-length-01.svg seems to be indented one space too many. :)
Comment 10 Robert Longson 2011-06-17 08:09:43 PDT
Created attachment 540054 [details] [diff] [review]
address review comments
Comment 11 Robert Longson 2011-06-17 08:12:41 PDT
> 
> Actually, I don't understand how dynamic-use-04.svg works. How does setting
> the width to 1 fill the viewport?

Anything non-zero is rendered the same as width and height are only used if the use points to a symbol or an <svg> element. Anything non-zero isn't rendered.

> 
> In anim-use-length-01.svg I'm not sure that the first <use> is useful, since
> if the second behaves correctly then the first is covered and redundant and
> isn't actually testing anything. Maybe just remove that and make the
> background red?
> 

I've moved the tests so they aren't co-located.
Comment 12 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-17 09:19:38 PDT
Comment on attachment 540054 [details] [diff] [review]
address review comments

You need to add class="reftest-wait" to the tests now using MozReftestInvalidate. Since you need to go back to those two files anyway, can you add a blank line before the </script>?

anim-use-length-01.svg seems busted. The rects in both symbols are lime, and in fact sym2 isn't even used. I also don't see what having two <use>s animating the width in two different directions gives you over animating just one. What failure are you expecting to catch by doing that?

+ <use x="0" y="200" fill="lime" xlink:href="#lime"/>

You can remove the 'fill' attribute on this element in use-01.svg. Also I'm not sure why the 'x' and 'y's are set to 200 rather than 100. Not a big deal, but maybe change that while you're there for the 'fill' attribute?

(In reply to comment #11)
> Anything non-zero is rendered the same as width and height are only used if
> the use points to a symbol or an <svg> element. Anything non-zero isn't
> rendered.

Ah, right. Can you add an opening comment to doTest something like this:

        // Since the <use> does not reference an <svg> or <symbol>, the value
        // of its 'width' attribute is ignored except to disable/enable its
        // rendering by setting it to zero/non-zero. Setting it to a non-zero
        // value here should show the entire referenced <rect>. See
        // http://www.w3.org/TR/SVG11/struct.html#UseElement
Comment 13 Robert Longson 2011-06-17 12:48:32 PDT
Created attachment 540107 [details] [diff] [review]
updated patch

Sorry about that. I thought I'd finished the updates to the patch but I clearly hadn't.

Includes a code change to reclone when the width/height goes away so that RemoveAttribute works properly (existing reftest that failed once some of the previous review comments were implemented)
Comment 14 Robert Longson 2011-06-18 02:08:53 PDT
> anim-use-length-01.svg seems busted. The rects in both symbols are lime, and
> in fact sym2 isn't even used. I also don't see what having two <use>s
> animating the width in two different directions gives you over animating
> just one. What failure are you expecting to catch by doing that?

Goldilocks, not too big and not too small.

> You can remove the 'fill' attribute on this element in use-01.svg. Also I'm
> not sure why the 'x' and 'y's are set to 200 rather than 100. Not a big
> deal, but maybe change that while you're there for the 'fill' attribute?

I split the difference and went for 150. If it fails I want it to be clear whether or not there are two separate failures.
Comment 15 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-20 03:16:07 PDT
Comment on attachment 540107 [details] [diff] [review]
updated patch

Regarding the <![CDATA[ addition, that should go around the entire script. For HTML compatibility, in the SVG 2.0 timeframe the SVG WG is going to seriously consider changing the content of the <script> tag from PCDATA to CDATA with a special rule to ignore <![CDATA[ at the start and ]]> at the end. The special rule would not catch <![CDATA[ in the middle of the script like you have it (something which I've never seen anyone actually do before BTW).

>+      } else {
>+        // Reclone so that we go back to the default value
>+        TriggerReclone();
>       }

"default" threw me here. I think this comment needs some expansion. Something like:

        // Our width/height attribute is now no longer explicitly set, so we
        // need to revert the clone's width/height to the width/height of the
        // content that's being cloned.

(In reply to comment #14)
> Goldilocks, not too big and not too small.

Okay. One other thing about this test is that I find the rather randomly chosen x/y/width/height values to be a bit of a distraction and make the test harder to figure grok what the test is doing. Seems like the x/y offsets could be omitted except on the second test. Making the width/height 100/50 or something nice and round like that would make it easier to read too. Up to you though.
Comment 16 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-20 03:18:24 PDT
> except on the second test

By which I mean the second sub-test in anim-use-length-01.svg
Comment 17 Robert Longson 2011-06-20 13:18:38 PDT
Created attachment 540570 [details] [diff] [review]
hg changeset patch with final review comments addressed
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-21 05:17:56 PDT
Comment on attachment 540570 [details] [diff] [review]
hg changeset patch with final review comments addressed

I'll land this a bit later after changing the tests to link to:

http://creativecommons.org/publicdomain/zero/1.0/
Comment 19 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-06-21 05:44:21 PDT
Pushed to Try: http://tbpl.mozilla.org/?tree=Try&rev=9cbab3ca911c
Comment 20 :Ehsan Akhgari (out sick) 2011-06-21 15:06:54 PDT
http://hg.mozilla.org/mozilla-central/rev/30c1b4519b2a
Comment 21 Vlad [QA] 2011-08-24 07:53:34 PDT
I have run the tests from the attachment but I get no animation in 
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 1
Comment 22 Robert Longson 2011-08-24 15:02:19 PDT
(In reply to Vlad from comment #21)
> I have run the tests from the attachment but I get no animation in 
> Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 1

How long did you wait? The animations last around 25 seconds and only do things every 10 seconds or so.
Comment 23 Daniel Holbert [:dholbert] 2011-08-24 15:07:23 PDT
BTW: Comment 6 is still true (that testcase is still broken) in latest nightly, FWIW... Should that have been fixed by the patch here, or should I file a new bug on that?
Comment 24 Dr. Olaf Hoffmann 2011-08-25 02:07:10 PDT
Robert Longson - note that these are two independent and repeated and continuous animations for width and height, therefore all the time something should change, sometimes only the ellipses vanish for about 2s, because width or height is zero. Just for the g-test-case this is everything, what happens. For the other two testcase the distortion of the ellipses changes all the time.

I checked this with 9a1 from yesterday - well, the result is not very convincing
compared for example with the current Opera version, which manages the problem
under similar conditions on the same computer ... 
But it might be a performance problem, but than Gecko would be very bad compared
to Opera, Batik/Squiggle, Adobe plugin or WebKit.

I think, the original tests from my test suite including a comparison with
animateTransform still fail completely due to another unfixed bug in Gecko, therefore currently it is difficult to provide a precision test for Gecko to indicate clearly and visually, what should happen ;o)
Comment 25 Robert Longson 2011-08-25 02:25:28 PDT
(In reply to Daniel Holbert [:dholbert] from comment #23)
> BTW: Comment 6 is still true (that testcase is still broken) in latest
> nightly, FWIW... Should that have been fixed by the patch here, or should I
> file a new bug on that?

I think scripted animation is still broken (except for transitions to and from <=0). As far as I can tell SMIL is OK now. We need a new bug as this one is done.
Comment 26 Vlad [QA] 2011-08-29 00:17:48 PDT
I have now tested this on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2 and I get no animation, only still pictures of ellipsis.
I have waited several seconds and the images are changing but i still don't have no animation.
Comment 27 Robert Longson 2011-08-29 02:30:39 PDT
(In reply to Vlad from comment #26)
> I have now tested this on Mozilla/5.0 (Windows NT 6.1; rv:7.0)
> Gecko/20100101 Firefox/7.0 beta 2 and I get no animation, only still
> pictures of ellipsis.
> I have waited several seconds and the images are changing but i still don't
> have no animation.

bug 663288 finally fixes everything to work properly.
Comment 28 Vlad [QA] 2011-09-20 04:34:27 PDT
Hi guys.
I have tried this once again on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 6 

and I still get no animation.
If the target milestone is Mozilla7, doesn't this suppose to work in Firefox 7 beta?
Thanks
Comment 29 Robert Longson 2011-09-20 05:00:42 PDT
Only the use case in bug 648875 is fixed for Mozilla 7. Everything else requires 9.

Note You need to log in before you can comment on or make changes to this bug.