Bug 26328 – Changing href attribute of svg images does not work when changing display attribute as well

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Volker Gersabeck]

please try the sample page: https://rapidrabb.it/files/svg-image-bug.xhtml

description:
I have an svg image tag with an empty xlink:href attribute and the broken image icon is shown. then i use JavaScript to set the display attribute and the xlink:href at the same time in three functions as follows:
- set a real image url and display to inherit.
- set an empty image url and display to none.
- remove the xlink:href attribute and set display to none.
Either the image should be shown (first case) or nothing should be visible. However, in some cases the broken image icon reappears.

[Volker Gersabeck]

Created attachment 31168 [details]
example of the bug with short description

[Volker Gersabeck]

forgot to mention: this bug seems to be new since Safari 4 Beta.

[Rob Buis]

Created attachment 31244 [details]
First attempt

This fixes the bug :) I tried to emulate the two problems in two separate testcases.
Cheers,

Rob.

Comment on attachment 31244 [details]
First attempt

> +        * svg/SVGElement.cpp:
> +        (WebCore::SVGElement::setXmlbase):

I don’t see SVGElement.cpp diffs in the patch.

[Eric Seidel (no email)]

Comment on attachment 31244 [details]
First attempt

SVGLoad events would be a better solution than using timeouts.

[Rob Buis]

Created attachment 31257 [details]
Fixed testcases

I think Eric meant this with using SVGLoad. Also removed setXmlBase stuff.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 31257 [details]
Fixed testcases

I don't understand this clause:
 95     if (isURIAttribute)
 96         m_imageLoader.updateFromElementIgnoringPreviousError();
 97     else if (!renderer())
 98         return;

Why only return when ! isURIAttribute && !renderer()?  That seems wrong, given all the other code below.

How does HTML handle this case?  This patch doesn't seem right.

[Rob Buis]

Created attachment 31598 [details]
Different approach

I think this approach is clearer, the image loading is part of the general attribute change notification and
does not depend on the renderer being there. If any svg attribute changes still a setNeedsLayout is called, when there is a renderer. HTMLImageElement also loads image on src changes in parseMappedAttribute.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 31598 [details]
Different approach

Does this still work if you set href.baseVal.value= "newurl.png"?

You're removing the m_imageLoader.updateFromElementIgnoringPreviousError(); update from svgAttributeChanged, which makes me think that this will break updates which are driven by JavaScript property changes.

Yes, it sucks that JS doesn't just call parseMappedAttribute.  Eventually we need to fix that.

I think this broke href.baseVal, so r-

[Rob Buis]

Created attachment 31666 [details]
Back to fixing SVGImageElement::svgAttributeChanged

These changes were made:

- I went back to the original fix. Since svgAttributeChanged is there for SVG and parseMappedAttribute is not called for the JS bindings, I tried to fix up SVGImageElement::svgAttributeChanged. Basically the xlink:href handling is singled out first and done independent of renderer() being there or not. The rest is as before, I hope the code is more readable than in the first patch. I guess in the long run svgAttributeChanged should go (is there a bug for that?).
- I added a test for the href.baseValue setting alternative, which does not end up in parseMappedAttribute.
- I updated the ChangeLogs because it also fixes bug 26392.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 31666 [details]
Back to fixing SVGImageElement::svgAttributeChanged

Looks OK.

[Rob Buis]

Landed in r45095.