Bug 26328 - Changing href attribute of svg images does not work when changing display attribute as well
Summary: Changing href attribute of svg images does not work when changing display att...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL: https://rapidrabb.it/files/svg-image-...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-11 11:20 PDT by Volker Gersabeck
Modified: 2009-06-24 23:30 PDT (History)
0 users

See Also:


Attachments
example of the bug with short description (2.02 KB, application/xhtml+xml)
2009-06-11 11:22 PDT, Volker Gersabeck
no flags Details
First attempt (8.53 KB, patch)
2009-06-13 12:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Fixed testcases (8.37 KB, patch)
2009-06-14 04:48 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Different approach (8.59 KB, patch)
2009-06-20 12:44 PDT, Rob Buis
eric: review-
Details | Formatted Diff | Diff
Back to fixing SVGImageElement::svgAttributeChanged (11.23 KB, patch)
2009-06-22 12:34 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Volker Gersabeck 2009-06-11 11:20:44 PDT
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.
Comment 1 Volker Gersabeck 2009-06-11 11:22:04 PDT
Created attachment 31168 [details]
example of the bug with short description
Comment 2 Volker Gersabeck 2009-06-11 11:23:42 PDT
forgot to mention: this bug seems to be new since Safari 4 Beta.
Comment 3 Rob Buis 2009-06-13 12:43:27 PDT
Created attachment 31244 [details]
First attempt

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

Rob.
Comment 4 mitz 2009-06-13 12:52:33 PDT
Comment on attachment 31244 [details]
First attempt

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

I don’t see SVGElement.cpp diffs in the patch.
Comment 5 Eric Seidel (no email) 2009-06-14 02:23:30 PDT
Comment on attachment 31244 [details]
First attempt

SVGLoad events would be a better solution than using timeouts.
Comment 6 Rob Buis 2009-06-14 04:48:15 PDT
Created attachment 31257 [details]
Fixed testcases

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

Rob.
Comment 7 Eric Seidel (no email) 2009-06-18 18:18:33 PDT
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.
Comment 8 Rob Buis 2009-06-20 12:44:58 PDT
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.
Comment 9 Eric Seidel (no email) 2009-06-22 00:00:44 PDT
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-
Comment 10 Rob Buis 2009-06-22 12:34:23 PDT
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.
Comment 11 Eric Seidel (no email) 2009-06-24 00:39:06 PDT
Comment on attachment 31666 [details]
Back to fixing SVGImageElement::svgAttributeChanged

Looks OK.
Comment 12 Rob Buis 2009-06-24 23:30:41 PDT
Landed in r45095.