Ctrl-Click or Shift-Click on a SVG-feature with named graphic opens new window because of <use>/<def> and the href-attribute

[marcjansen]

See this thread on the OpenLayers users mailinglist:

 http://osgeo-org.1803224.n2.nabble.com/Select-Control-Ctrl-click-on-Feature-with-a-graphicName-opens-new-browser-window-tc5846039.html

As suggested by ahocevar I'll provide a patch soon that'll remove the property supportUse.

[marcjansen]

The attached patch removes the attribute supportUse from OpenLayers.Renderer.SVG as suggested by ahocevar.

The example graphic-name.html still looks good in Opera 8.0.552.224 and Firefox 3.6.13 (both on Ubuntu 10.10). The example has been changed to also include a rotation-property in the default-style (all other changes were merely cosmetic). I changed that as Firebug gave a notice when hovering out of a rendered feature. This might have been introduced by this patch.

It also removes a now obsolete test from SVG.html that only makes sense when supportUse exists.

It does not include new tests to check the changes, I would be glad if someone would share advice about how to test the specific rendering.

Thanks for any review or comments.

[ahocevar]

marcjansen: the notice you are seeing in firebug comes from a typo in SVG.js (style.rotation instead of rotation), and there is another block in there that can be removed. See this diff which fixes both:

Index: lib/OpenLayers/Renderer/SVG.js
===================================================================
--- lib/OpenLayers/Renderer/SVG.js	(revision 10975)
+++ lib/OpenLayers/Renderer/SVG.js	(working copy)
@@ -337,16 +331,10 @@
             if ((rotation !== undefined || node._rotation !== undefined) && pos) {
                 node._rotation = rotation;
                 rotation |= 0;
-                if(node.nodeName !== "svg") {
-                    node.setAttributeNS(null, "transform",
-                        "rotate(" + rotation + " " + pos.x + " " +
-                        pos.y + ")");
-                } else {
-                     var metrics = this.symbolMetrics[id];
-                     node.firstChild.setAttributeNS(null, "transform",
-                     "rotate(" + style.rotation + " " + metrics[1] +
-                         " " +  metrics[2] + ")");
-                }
+                var metrics = this.symbolMetrics[id];
+                node.firstChild.setAttributeNS(null, "transform",
+                "rotate(" + rotation + " " + metrics[1] +
+                    " " +  metrics[2] + ")");
             }
         }
         

Regarding the examples: you added anchor and mining symbols, but commented them out with a note about incompatibility with Chrome 8. What is the issue here? And after fixing the above typo, it won't be necessary to add a rotation property to the default style.

For the tests: we don't really require unit tests here, but what you could do if you want to add tests is find the dom element that represents a graphicName point and compare its coordinates and position with your expected ones. The best test for this part of the rendering engine is an acceptance test using the graphic-name.html example.

Another minor remark: you can remove the no longer needed tests from the test case entirely, instead of commenting them out. And instead of the comment around line 300 ("workaround for webkit..."), you could just state that the more appropriate way to implement this would be use/defs, but due to various issues in several browsers, it is safer to copy the symbols instead of referencing them.

If you can fix the rotation part and my remarks, and explain what the issue with the anchor symbol is, then this patch is good to be committed. Again, thanks for your efforts!

[marcjansen]

Hi ahocevar,

the new patch attachment:OpenLayers-2985-002.patch (a full replacement for the first patch)...

• ...removes "rotation" from the default-style in the example
• ...includes your changes stated in comment:2
• ...therefore the firebug notice disappears
• ...removes the previously added "anchor" and "mining" named-symbols, as these were buggy in Chrome. They didn't render with the old use/defs-renderer either, so I suppose that this is not an issue introduced by this patch
• ...removes one test from tests/Renderer/SVG.html as this one is now obsolete

The file examples/graphic-name.html renders fine in FF 3.6.13, Opera 10.62 and Chrome 8.0.552.224 all on Ubuntu 10.10.

Thanks again for commenting on this and reviewing the patch. Merry Christmas y'all.

[ahocevar]

r10976

[marcjansen]

Hi ahocevar,

Thanks for reviewing, but revision r10976 is missing the file "examples/graphic-name.js" that as is included in the attachment:OpenLayers-2985-002.patch . Could you please add that one as well, the example is currently broken.

[ahocevar]

Thanks marcjansen - the example is now committed with r10991.

[ahocevar]

The advice I gave in 2 was a bad one. The code for nodeName != "svg" is still needed.

[ahocevar]

fixed with r11018 - thanks cmoullet for spotting this issue.

[tschaub]

I put the v3.2 bit back in the examples in r11019. Please clarify if this was indeed supposed to be removed.

[ahocevar]

Thanks tschaub for your sharp eye. In fact there were even more changes committed with r11018 that were part of #2984. I reverted the remaining ones with r11021.