Including JavaScript in SVG

Question

I am trying to create an interactive SVG code with JavaScript, by embedding the JavaScript in the SVG. I don't know if this is the right way to do this:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg width="100%" height="100%" version="1.1"
xmlns="http://www.w3.org/2000/svg"
onkeypress="move()">
<script type="text/javascript">
    <![CDATA[
    var x;
    var y;
    function move()
    {
        x = new Number(svg.getElementsByTagName("circle")[0].getAttribute("cx"));
        y = new Number (svg.getElementsByTagName("circle")[0].getAttribute("cy"));
        switch (event.keyCode)
        {
            case 119:
            y--;
            y = y.toString();
            svg.getElementsByTagName("circle").setAttribute("cy",y);
            break;
            case 115:
            y++;
            y = y.toString();
            svg.getElementsByTagName("circle").setAttribute("cy",y);
            break;
            case 97:
            x--;
            x = x.toString();
            svg.getElementsByTagName("circle").setAttribute("cx",x);
            break;
            case 100:
            x++;
            x = x.toString();
            svg.getElementsByTagName("circle").setAttribute("cx",x);
            break;
            default:
        }
    }
    ]]>
</script>
<rect x="0" y="0" height="500" width="500" style="stroke-width:1; stroke:black; fill:white"></rect>
<circle cx="250" cy="250" r="50" stroke="red" stroke-width="1" fill="red"></circle>
</svg>

It is supposed to have a ball that moves with wasd, but the ball doesn't move. What am I doing wrong?

Answer 1

Here is a working version as I would write it:

<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN"
  "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
<svg version="1.1" xmlns="http://www.w3.org/2000/svg">
  <circle cx="250" cy="250" r="50" fill="red" />
  <script type="text/javascript"><![CDATA[
    var KEY = { w:87, a:65, s:83, d:68 };
    var moveSpeed = 5;
    var circle = document.getElementsByTagName("circle")[0];
    var x = circle.getAttribute('cx')*1,
        y = circle.getAttribute('cy')*1;
    document.documentElement.addEventListener('keydown',function(evt){
      switch (evt.keyCode){
        case KEY.w:
          circle.setAttribute('cy',y-=moveSpeed);
          // Alternatively:
          // circle.cy.baseVal.value = (y-=moveSpeed);
        break;
        case KEY.s:
          circle.setAttribute('cy',y+=moveSpeed);
        break;
        case KEY.a:
          circle.setAttribute('cx',x-=moveSpeed);
        break;
        case KEY.d:
          circle.setAttribute('cx',x+=moveSpeed);
        break;
      }
    },false);
  ]]></script>
</svg>

Some notes:

1. Don't re-get the reference to the circle again and again. Making your code DRY makes it more robust, less typing, and (in this case) faster to execute.

   Edit: If you cannot figure out how to do this given my code above, post any code that is not working for you.

2. Don't rely on a global event object; that's old IE nonsense. Use the event object passed to your event handler.

   Edit: If you reference event in your code with no parameter or local variable by that name, you are assuming that there will be a global event object set. Instead, see the code I wrote for you, which shows that the event handler is passed an event object. By giving that a name, such as I gave it the name evt, you are receiving an event object specific to your event handler.

3. Since you are modifying the x and y variables, there's no need to re-get the cx and cy attributes each key press.

   Edit: In your original code and the answer you accepted, you have declared var x outside your event handler, and you have x = ... at the start of your event handler, and then x++ in one of the event handlers. You can either re-get the current value of x each time (as you did) and then setAttribute(...,x+1), or (as I did) you can only fetch the value of the attribute once before the event handlers and then assume that this value is correct each time you handle the key event.

4. Don't put your JavaScript event handlers on your elements, attach them programmatically.

   Edit: In your SVG markup you have: <svg ... onkeypress="move()">. Mixing your behavior with your markup is a really bad idea in HTML, and a bad idea in SVG. Instead of using onfoo="..." attributes to describe what should happen when an event occurs on an element, instead use addEventListner() to attach the event handlers via code, without editing your SVG markup.

5. There's no need to coerce the numbers to strings before setting them as attributes.

6. Use keydown and the ASCII event codes I supplied above instead of keypress and the odd numbers you were using if you want it to work in all browsers.

   Edit: You complained in another post that you cannot do this because you want the event handler to be processed repeatedly as the key is held down. Note that your desired behavior is achieved with my sample code in Chrome, Safari, Firefox, and IE (I don't have Opera to test). In other words keydown works as you wanted, despite how you thought it should behave.

Edit 2: If you want to include a script block at the top of your document, before all elements have necessarily been created, you can do something like the following:

<svg ...>
  <script type="text/javascript">
    window.addEventListener('load',function(){
      var circle = ...;
      document.rootElement.addEventListener('keydown',function(evt){
        ...
      },false);
    },false);
  </script>
  ...
</svg>

The outer function will only run once the page has loaded, so you can be sure that the elements exist to reference them.