285

I am trying to use an HTML button to call a JavaScript function.

Here's the code:

<input type="button" value="Capacity Chart" onclick="CapacityChart();">

It doesn't seem to work correctly though. Is there a better way to do this?

Here is the link:http://projectpath.ideapeoplesite.com/bendel/toolscalculators.html click on the capacity tab in the bottom left section. The button should generate an alert if the values are not changed and should produce a chart if you enter values.

Brett DeWoody
  • 59,771
  • 29
  • 135
  • 184
forrest
  • 10,570
  • 25
  • 70
  • 132
  • 8
    please define "doesn't seem to work correctly". what error(s) are you getting when you click it? – just somebody Dec 22 '09 at 15:59
  • The button works in IE8, but not FF 3.5 due to a JavaScript error (see Jeff's answer) – Chris Shouts Dec 22 '09 at 16:46
  • 1
    Yeah, turns out document.all is a nonstandard IE thing; updated my answer with a suggested alternative. – Jeff Dec 22 '09 at 16:50
  • @paper1337, @Jeff: The function still doesn't achieve the desired effect in IE8, so even swapping document.all for document.getElementById won't fix it. – Andy E Dec 22 '09 at 19:48

8 Answers8

447

There are a few ways to handle events with HTML/DOM. There's no real right or wrong way but different ways are useful in different situations.

1: There's defining it in the HTML:

<input id="clickMe" type="button" value="clickme" onclick="doFunction();" />

2: There's adding it to the DOM property for the event in Javascript:

//- Using a function pointer:
document.getElementById("clickMe").onclick = doFunction;

//- Using an anonymous function:
document.getElementById("clickMe").onclick = function () { alert('hello!'); };

3: And there's attaching a function to the event handler using Javascript:

var el = document.getElementById("clickMe");
if (el.addEventListener)
    el.addEventListener("click", doFunction, false);
else if (el.attachEvent)
    el.attachEvent('onclick', doFunction);

Both the second and third methods allow for inline/anonymous functions and both must be declared after the element has been parsed from the document. The first method isn't valid XHTML because the onclick attribute isn't in the XHTML specification.

The 1st and 2nd methods are mutually exclusive, meaning using one (the 2nd) will override the other (the 1st). The 3rd method will allow you to attach as many functions as you like to the same event handler, even if the 1st or 2nd method has been used too.

Most likely, the problem lies somewhere in your CapacityChart() function. After visiting your link and running your script, the CapacityChart() function runs and the two popups are opened (one is closed as per the script). Where you have the following line:

CapacityWindow.document.write(s);

Try the following instead:

CapacityWindow.document.open("text/html");
CapacityWindow.document.write(s);
CapacityWindow.document.close();

EDIT
When I saw your code I thought you were writing it specifically for IE. As others have mentioned you will need to replace references to document.all with document.getElementById. However, you will still have the task of fixing the script after this so I would recommend getting it working in at least IE first as any mistakes you make changing the code to work cross browser could cause even more confusion. Once it's working in IE it will be easier to tell if it's working in other browsers whilst you're updating the code.

jpmc26
  • 28,463
  • 14
  • 94
  • 146
Andy E
  • 338,112
  • 86
  • 474
  • 445
  • 11
    Using jquery, there is also: `$("#clickMe").bind('click', function () { alert('hello!'); };)` – Roman May 22 '14 at 13:28
42

I would say it would be better to add the javascript in an un-obtrusive manner...

if using jQuery you could do something like:

<script>
  $(document).ready(function(){
    $('#MyButton').click(function(){
       CapacityChart();
    });
  });
</script>

<input type="button" value="Capacity Chart" id="MyButton" >
Paul
  • 5,514
  • 2
  • 29
  • 38
  • 5
    Agreed. In many cases, jQuery and other frameworks are sheer overkill for small amounts of javascript and not all javascript code needs to be cross-browser. – Andy E Dec 22 '09 at 16:11
  • 2
    Fair enough.. the point I was trying to make was "a better way to do this" is unobtrusively... Without jQuery, something like: var btn = document.getElementById('MyButton'); btn.onclick = function(){CapacityChart();} – Paul Dec 22 '09 at 16:21
  • 1
    These people using jQuery seem to be set on using it for everything web. How about using flat JavaScript for one? Goodness, jQuery is really getting on my nerves now. LOL! Ill call it lazy man's way of creating web, for I guess that's all it truly is. – DoctorLouie Dec 22 '09 at 22:16
  • @DrLouie: I have nothing at all against jQuery, it is a good solution to many problems, but when it's not tagged or asked by the OP any mention of jQuery should be assisted by a non-jQuery method too, otherwise they run the risk of a) confusing the asker - he might not know what jQuery is or b) annoying the asker by telling him to use jQuery for a few lines of javascript code. @Paul - I must say that wasn't a great read... – Andy E Dec 23 '09 at 10:25
  • 1
    apologies... wrong link posted - meant to post a stackoverflow one. http://stackoverflow.com/questions/1588374/to-be-a-lazy-developer-or-not-to-be-a-lazy-developer – Paul Dec 23 '09 at 16:32
9

Your HTML and the way you call the function from the button look correct.

The problem appears to be in the CapacityCount function. I'm getting this error in my console on Firefox 3.5: "document.all is undefined" on line 759 of bendelcorp.js.

Edit:

Looks like document.all is an IE-only thing and is a nonstandard way of accessing the DOM. If you use document.getElementById(), it should probably work. Example: document.getElementById("RUnits").value instead of document.all.Capacity.RUnits.value

Jeff
  • 21,744
  • 6
  • 51
  • 55
  • That's not specifically the problem with the function, which doesn't throw any errors in IE – Andy E Dec 22 '09 at 19:45
4

This looks correct. I guess you defined your function either with a different name or in a context which isn't visible to the button. Please add some code

jitter
  • 53,475
  • 11
  • 111
  • 124
3

Just so you know, the semicolon(;) is not supposed to be there in the button when you call the function.

So it should just look like this: onclick="CapacityChart()"

then it all should work :)

MashukKhan
  • 1,946
  • 1
  • 27
  • 46
jacki
  • 41
  • 4
2

One major problem you have is that you're using browser sniffing for no good reason:

if(navigator.appName == 'Netscape')
    {
      vesdiameter  = document.forms['Volume'].elements['VesDiameter'].value;
      // more stuff snipped
    }
    else
    {
      vesdiameter  = eval(document.all.Volume.VesDiameter.value);
      // more stuff snipped
    }

I'm on Chrome, so navigator.appName won't be Netscape. Does Chrome support document.all? Maybe, but then again maybe not. And what about other browsers?

The version of the code on the Netscape branch should work on any browser right the way back to Netscape Navigator 2 from 1996, so you should probably just stick with that... except that it won't work (or isn't guaranteed to work) because you haven't specified a name attribute on the input elements, so they won't be added to the form's elements array as named elements:

<input type="text" id="VesDiameter" value="0" size="10" onKeyUp="CalcVolume();">

Either give them a name and use the elements array, or (better) use

var vesdiameter = document.getElementById("VesDiameter").value;

which will work on all modern browsers - no branching necessary. Just to be on the safe side, replace that sniffing for a browser version greater than or equal to 4 with a check for getElementById support:

if (document.getElementById) { // NB: no brackets; we're testing for existence of the method, not executing it
    // do stuff...
}

You probably want to validate your input as well; something like

var vesdiameter = parseFloat(document.getElementById("VesDiameter").value);
if (isNaN(vesdiameter)) {
    alert("Diameter should be numeric");
    return;
}

would help.

NickFitz
  • 34,537
  • 8
  • 43
  • 40
  • Hi NickFitz, one of my challenges here is that this script was originally written in 1993 - hence the references to Netscape 4. I had some help getting it updated and it worked until I got it inserted in its designate page. – forrest Dec 22 '09 at 16:49
  • I doubt it was written in 1993, as Brendan Eich didn't invent JS until 1995 http://en.wikipedia.org/wiki/Javascript#History ;-) I would strongly recommend using `document.getElementById` and stripping out the other stuff. – NickFitz Dec 22 '09 at 17:05
  • I think it would be prudent to at least get it working in IE before then working on browser compatibility, otherwise he won't know if his browser compatibility issues will be fixed because it still won't work. – Andy E Dec 22 '09 at 19:47
  • Better to get it working using standard DOM techniques, then there won't be any browser compatibility problems to speak of :-) – NickFitz Dec 23 '09 at 12:56
2

Your code is failing on this line:

var RUnits = Math.abs(document.all.Capacity.RUnits.value);

i tried stepping though it with firebug and it fails there. that should help you figure out the problem.

you have jquery referenced. you might as well use it in all these functions. it'll clean up your code significantly.

Patricia
  • 7,752
  • 4
  • 37
  • 70
  • It isn't failing in IE on that line, though. – Andy E Dec 22 '09 at 19:45
  • because document.all is an IE thing. he should just use $('#RUints').val() jquery notation b/c he is including jquery already. this takes care of any browswer differences and cleans up the code. – Patricia Dec 23 '09 at 19:34
2

I have an intelligent function-call-backing button code:

<br>
<p id="demo"></p><h2>Intelligent Button:</h2><i>Note: Try pressing a key after clicking.</i><br>
<button id="button" shiftKey="getElementById('button').innerHTML=('You're pressing shift, aren't you?')" onscroll="getElementById('button').innerHTML=('Don't Leave me!')" onkeydown="getElementById('button').innerHTML=('Why are you pressing keys?')" onmouseout="getElementById('button').innerHTML=('Whatever, it is gone.. maybe')" onmouseover="getElementById('button').innerHTML=('Something Is Hovering Over Me.. again')" onclick="getElementById('button').innerHTML=('I was clicked, I think')">Ahhhh</button>