Ticket #2985 (closed bug: fixed)

Opened 2 years ago

Last modified 2 years ago

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

Reported by: marcjansen Owned by: ahocevar
Priority: minor Milestone: 2.11 Release
Component: Renderer.SVG Version: 2.10
Keywords: Cc:
State: Complete

Description

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.

Attachments

OpenLayers-2985-001.patch Download (24.5 KB) - added by marcjansen 2 years ago.
OpenLayers-2985-002.patch Download (13.0 KB) - added by marcjansen 2 years ago.

Change History

Changed 2 years ago by marcjansen

Changed 2 years ago by marcjansen

  • state set to Review

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.

Changed 2 years ago by ahocevar

  • state changed from Review to Needs More Work

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!

Changed 2 years ago by marcjansen

Changed 2 years ago by marcjansen

  • state changed from Needs More Work to Review

Hi ahocevar,

the new patch attachment:OpenLayers-2985-002.patch Download (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.

Changed 2 years ago by ahocevar

  • status changed from new to closed
  • state changed from Review to Complete
  • resolution set to fixed

Changed 2 years ago by marcjansen

  • status changed from closed to reopened
  • state changed from Complete to Review
  • resolution fixed deleted

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 Download. Could you please add that one as well, the example is currently broken.

Changed 2 years ago by ahocevar

  • status changed from reopened to closed
  • state changed from Review to Complete
  • resolution set to fixed

Thanks marcjansen - the example is now committed with r10991.

Changed 2 years ago by ahocevar

  • status changed from closed to reopened
  • resolution fixed deleted

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

Changed 2 years ago by ahocevar

  • status changed from reopened to closed
  • resolution set to fixed

fixed with r11018 - thanks cmoullet for spotting this issue.

Changed 2 years ago by tschaub

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

Changed 2 years ago by 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.

Note: See TracTickets for help on using tickets.