Unable to indirectly animate visibility of SVGUseElement

• Component: SVG
• Status: RESOLVED FIXED

When viewing the SVG below, the red rectangle should disappear at 3s.

The red rectangle is referenced by a <use>.
The <use> is contained inside a <g>. 
The visibility of the <g> is set to hidden at 3s.

<svg width="100%" height="100%" viewBox="0 0 1000 800"
  xmlns="http://www.w3.org/2000/svg"
  xmlns:xlink="http://www.w3.org/1999/xlink">
  <defs>
    <rect id="r" x="100" y="100" width="800" height="600" fill="red" stroke="none"/>
  </defs>
  <g>
    <use xlink:href="#r"/>
    <set attributeType="XML" attributeName="visibility" to="hidden" begin="3s"/>
  </g>
</svg>

Works with Safari 4.0.5 (531.22.7), Windows XP
Doesn't work with
  Chrome 7.0.517.44, Windows XP
  Safari 5.0.3 (6533.19.4), Mac OS X 10.6

A subsequent browser Zoom causes the red rectangle to disappear.

The <set> animation does work when directly targeting the <use>

  <use xlink:href="#r">
    <set attributeType="XML" attributeName="visibility" to="hidden" begin="3s"/>
  </use>

[marek]

Yes, I noticed the same buggy behavior in my game http://svg.kvalitne.cz/submarine/index.htm, which makes it unplayable at all.. :-(
I'm enclosing similar testcase extracted from my game...

[marek]

Created attachment 75627 [details]
Broken behavior testcase, behaves the same bad way also on Windows platform...

[Leo Yang]

Created attachment 78781 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 78781 [details]
Patch

Excellent catch! Code r+, Test r->
It's a pity that we need platform dependant results, maybe you could come up with a testcase that avoids font-size changes?
Can't you trigger the bug with parent fill='..' changes, or sth. else like stroke-width?

[Leo Yang]

Created attachment 79120 [details]
Revised patch version 2

Using fill = "..." is a good idea. Maybe we don't need platform dependent test case because dumRenderTree can produce filling color in text.

[Nikolas Zimmermann]

Comment on attachment 79120 [details]
Revised patch version 2

View in context: https://bugs.webkit.org/attachment.cgi?id=79120&action=review

Excellent, almost r+. Our common style is to indidicate a "green rect" as success in the expected.png/txt files. Yours is red, please change the transition from black -> red, to red -> green, then I'll r+/cq+ it. Thanks for your patience :-)

> LayoutTests/svg/custom/use-inherit-style.svg:8
> +    <rect id="rect" x="0" y="0" width="100" height="60" />

Here you should use fill="red" as initial value...

> LayoutTests/svg/custom/use-inherit-style.svg:17
> +        document.getElementById("g").setAttribute("fill", "red");

... and change it to fill="green" here.

[Leo Yang]

Created attachment 79235 [details]
Revised patch version 3

We can't set initial fill value for the rectangle because otherwise the rectangle will not inherit fill property of <g> element. Using "green" causes 0x008000 color value which isn't consistent with the existing truly green png, so 0x00FF00 is used explicitly.

[Nikolas Zimmermann]

(In reply to comment #7)
> Created an attachment (id=79235) [details]
> Revised patch version 3
> 
> We can't set initial fill value for the rectangle because otherwise the rectangle will not inherit fill property of <g> element. Using "green" causes 0x008000 color value which isn't consistent with the existing truly green png, so 0x00FF00 is used explicitly.

I'm sorry, with 'truly green' I meant the "green" color value, 0x008000. Using "green" is perfectly fine.

[Nikolas Zimmermann]

Comment on attachment 79235 [details]
Revised patch version 3

I don't care though whether the rect is 00FF00 or just 00FF00, either is fine, so r=me.

[WebKit Commit Bot]

Comment on attachment 79235 [details]
Revised patch version 3

Clearing flags on attachment: 79235

Committed r76027: <http://trac.webkit.org/changeset/76027>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.