PHP JavaScript and HTML, seemingly not calling the function.

[Scobbo]

Hey everyone,
This is my firist post, I have been searching for the answer for quite a while and trying many different solutions. I have some java experience but I think I have done something wrong in the code. I'm trying to get the html boxes to update number that is being drawn to the scree when it has been changed. there are 3 boxes that call a function that requires 3 arguments (this is the part I think I'm messing up. Here is the code:

			<div>
				<label for="buy">Buy Price: <input type="text" name="buy" 
						id="buy" onchange="solve(this.id,freight.id,markup.id)" value="<?php htmlout($buy); ?>"/></label>
			</div>
			<div>
				<label for="freight">Freight: <input type="text" name="freight"
						id="freight" onchange="solve(buy.id,this.id,markup.id)" value="<?php htmlout($freight); ?>"/></label>
			</div>
			<div>
				<label for="markup">Markup: <input type="text" name="markup"
						id="markup" onchange="solve(buy.id,freight.id,this.id)" value="20"/></label>%
			</div>
			<div>
        Sell Price: 
			<script type="text/javascript">
			function solve(x,y,z)
			{
	  	var buy=document.getElementById(x).value;
			var freight=document.getElementById(y).value;
			var markup=document.getElementById(z).value;
			var newmarkup;
			var sell;
			var markupamount;
			sell=buy+freight;
			newmarkup= "0."+markup;
			var i = Integer.parseInt(newmarkup.trim());
			markupamount=sell*i;
			sell=sell+markupamount;
			document.write(sell);
			}
			</script>
			</div>

I believe it should work, but as I said I have only done a little with java and I think I'm messing up somewhere with the arguments.

Let me know if you need any more information, thanks heaps for any help you guys can offer!

[paul_wilkins]

Before other complaints come in, Java is a language for creating applications which has its own completely separate Java and JSP forum. JavaScript by comparison is not an application language, it is a scripting language instead. They are completely different and have about as much similarity to each other as a cup has to do with cupboards. Please learn this.

On to your code.

Problem 1: the variable called Integer is not defined. I presume that you're just wanting to use the parseInt function instead. If that's the case, you'll also want to specify the second parameter of 10, which is very important given that people can enter "08" or "09" which parseInt will automatically interpret as being octal values. Force it to work in base 10 and you won't have any potential bugs there.

var i = parseInt(newmarkup.trim(), 10);

Problem 2: Form values are strings, so when you do buy + freight, JavaScript will be concatenating strings, it won't be adding any numbers together. If you expect them to be numbers, make sure that they are received from the form as numbers. If it's not a number in the form then give a default value instead.

var buy = Number(document.getElementById(x).value) || 0;
var freight = Number(document.getElementById(y).value) || 0;
var markup = Number(document.getElementById(z).value) || 0;

Problem 3: document.write is deprecated for a good reason. Once the page has finished loading, any attempt to use document.write will completely wipe the page and start again with nothing.

Instead of that, you need to be able to target a certain part of the existing page, so that you can update it with your new info. When dealing with in-page content the innerHTML function can be useful for that.

Here is an update to the HTML that provides you a place to put the updated info.

Sell Price: <span id="sell"></span>

where you could update the contents of the span with:

document.getElementById('sell').innerHTML = sell;

Now for some improvements.

All of the var statements can be shifted to a single statement at the start. This way you can see the declared intentions right off, and it's easier to rework the code.

var buy, freight, markup, newmarkup, sell, markupamount, i;

These next four lines can be improved quite a lot now, by some refactoring:

sell = buy + freight;
newmarkup= "0." + markup;
i = parseInt(newmarkup.trim());
markupamount=sell*i;
sell = sell + markupamount;

Guess what happens when newmarkup is "0.20". The value of i will be 0, so there's no markup.

The newmarkup calculation is the biggest problem. Strings and numbers should not be added together. Now that buy, freight and markup are actual numbers, the above 5 lines can be correctly reduced to this one line:

sell = (buy + freight) * (1 + markup / 100);

Where a 20% markup means multiplying by 1.2

Another improvement could be to not pass in all of the id values to the function. Currently there are three separate inline events that are required on the inputs. Consider an improvement where a common parent, such as a form element, has just the one onchange event that passes the form as a reference itself. That way the inputs can be referenced from within the script as e.g., form.elements.buy

This allows the HTML code to be much cleaner. Because you wrap the label around the input, there is already an implicit association with the label, so the for attribute doesn't need to be specified either:

<form id="sales">
    <div>
        <label>Buy Price:
            <input type="text" name="buy" value="13"/>
        </label>
    </div>
    <div>
        <label>Freight:
            <input type="text" name="freight" value="7"/>
        </label>
    </div>
    <div>
        <label>Markup:
            <input type="text" name="markup" value="20"/>
        </label>%
    </div>
    <div>
        Sell Price: <span id="sell"></span>
    </div>
</form>
<script type="text/javascript">
...
</script>

And a simple script at the end of the body then attaches an onchange event to the form.

Here's the final script after all of these updates:

document.getElementById('sales').onchange = function() {
    solve(this);
};
 
function solve(form) {
    var buy, freight, markup, sell;
    buy = Number(form.elements.buy.value) || 0;
    freight = Number(form.elements.freight.value) || 0;
    markup = Number(form.elements.markup.value) || 0;
 
    sell = (buy + freight) * (1 + markup / 100);
 
    document.getElementById('sell').innerHTML = sell;
}

Edit:

IE fails to bubble the onchange event. See below for update

[Scobbo]

Wow, thanks heaps for all of that, that was much more help than I expected. As you can see, I'm not really good at Java or JSP. I'm still learning. I only really needed it for this bit of my site because I don't want to reload the page everytime. I'll get to work on it now and I'll let you know how everything went. Thanks again!

[paul_wilkins]

Wow, thanks heaps for all of that, that was much more help than I expected. As you can see, I'm not really good at Java or JSP.

I don't know the first thing about Java or JSP either, because neither of them are JavaScript. The language being handled here is JavaScript.

[Scobbo]

Hey, It isn't working still. I see how it should, but do I need to rename anything? I copied and pasted the code, and when I update any of those 3 boxes it doesn't do anything, I type the number I hit tab to the next line but it sales stays blank. Here is my code.

			<form id="sales">
    <div>
        <label>Buy Price:
            <input type="text" name="buy" value="<?php htmlout($buy); ?>"/>
        </label>
    </div>
    <div>
        <label>Freight:
            <input type="text" name="freight" value="<?php htmlout($freight); ?>"/>
        </label>
    </div>
    <div>
        <label>Markup:
            <input type="text" name="markup" value="20"/>
        </label>%
    </div>
    <div>
        Sell Price: <span id="sell"></span>
    </div>
</form>
			<div> 
			<script type="text/javascript">
      document.getElementById('sales').onchange = function() {
      solve(this);
      };
 
      function solve(form) {
        var buy, freight, markup, sell;
        buy = Number(form.elements.buy.value) || 0;
        freight = Number(form.elements.freight.value) || 0;
        markup = Number(form.elements.markup.value) || 0;
        sell = (buy + freight) * (1 + markup / 100);
        document.getElementById('sell').innerHTML = sell;
      }
			</script>

[felgall]

The way you have attached the onchange means it will only be triggered if the form is changed and your visitors can't do that.

Try moving it to a field or fields within the form that your visitors can change.

[Scobbo]

The way you have attached the onchange means it will only be triggered if the form is changed and your visitors can't do that.

Try moving it to a field or fields within the form that your visitors can change.

Anyone who can see this form can change it. Thats the way I want it.

I have changed the code to add in the change handler but it still doesn't do anything. I'm pretty sure I put the code in the right spot

        <label>Buy Price:
            <input type="text" name="buy" value="<?php htmlout($buy); ?>"/>
        </label>
    </div>
    <div>
        <label>Freight:
            <input type="text" name="freight" value="<?php htmlout($freight); ?>"/>
        </label>
    </div>
    <div>
        <label>Markup:
            <input type="text" name="markup" value="20"/>
        </label>%
    </div>
    <div>
        Sell Price: <span id="sell"></span>
    </div>
</form>
			<div>
			<script type="text/javascript"> 
			var form = document.getElementById('sales');
      form.elements.buy.onchange = onchangeHandler;
      form.elements.freight.onchange = onchangeHandler;
      form.elements.markup.onchange = onchangeHandler;
 
      function onchangeHandler() {
      solve(this.form);
      };
 
      function solve(form) {
        var buy, freight, markup, sell;
        buy = Number(form.elements.buy.value) || 0;
        freight = Number(form.elements.freight.value) || 0;
        markup = Number(form.elements.markup.value) || 0;
        sell = (buy + freight) * (1 + markup / 100);
        document.getElementById('sell').innerHTML = sell;
      }
			</script>
			</div>

[paul_wilkins]

It seems that Internet Explorer is pretty much alone in not bubbling up onchange events from forms. All modern web browsers are capable of doing that, as well as many not-so-modern web browsers. However, we must ensure that the code is capable of running on as wide a cross-section of web browsers.

We should assign the change event to the individual form elements instead, which keeps IE happy and allows things to work as intended.

var form = document.getElementById('sales');
form.elements.buy.onchange = onchangeHandler;
form.elements.freight.onchange = onchangeHandler;
form.elements.markup.onchange = onchangeHandler;
 
function onchangeHandler() {
    solve(this.form);
};

The solve function can remain the same as it was before.

[paul_wilkins]

Where's the form tag? It should have an id of "sales" to allow the script to target the elements within that form.

[Scobbo]

Opps sorry, I missed 2 lines at the top of the code, it has

			<form id="sales">
    <div>

above it, I'll add it in now to the previous post.

:EDIT:
Seems I can only edit this post, so here is the code.

			<form id="sales">
    <div>
        <label>Buy Price:
            <input type="text" name="buy" value="<?php htmlout($buy); ?>"/>
        </label>
    </div>
    <div>
        <label>Freight:
            <input type="text" name="freight" value="<?php htmlout($freight); ?>"/>
        </label>
    </div>
    <div>
        <label>Markup:
            <input type="text" name="markup" value="20"/>
        </label>%
    </div>
    <div>
        Sell Price: <span id="sell"></span>
    </div>
</form>
			<div>
			<script type="text/javascript"> 
			var form = document.getElementById('sales');
      form.elements.buy.onchange = onchangeHandler;
      form.elements.freight.onchange = onchangeHandler;
      form.elements.markup.onchange = onchangeHandler;
 
      function onchangeHandler() {
      solve(this.form);
      };
 
      function solve(form) {
        var buy, freight, markup, sell;
        buy = Number(form.elements.buy.value) || 0;
        freight = Number(form.elements.freight.value) || 0;
        markup = Number(form.elements.markup.value) || 0;
        sell = (buy + freight) * (1 + markup / 100);
        document.getElementById('sell').innerHTML = sell;
      }
			</script>
			</div>

[paul_wilkins]

Okay, that seems to work for me.

What browser are you using, and can you give more details concerning "doesn't do anything"

A test page would be good, so that we can browse there in order to diagnose anything further that may be interfering with it.

[Scobbo]

Okay, that seems to work for me.

What browser are you using, and can you give more details concerning "doesn't do anything"

A test page would be good, so that we can browse there in order to diagnose anything further that may be interfering with it.

Ok Here is a test page:

robcol.freehostia.com/task

You will need to log in with Username: sitepoint Password: sitepoint

Then (because the server I have this on is odd and doesn't work the same as the server it will be on and the dev server) you will need to add "parts" after the final / so the URL will be http://robcol.freehostia.com/task/parts then hit add new part and you can see what I'm getting.

Thanks heaps!

P.S. There may be issues with the add button, this project is far from complete but you shouldn't need to press it to see the issue.

[paul_wilkins]

Look at the form tag.

In the above code it is:

<form id="sales">

It is that id attribute of "sales" that allows the script to access the form.

Look at the form tag on your page:

<form action="?addform" method="post">

When you add the id attribute to the form on your page, it should look like this:

<form id="sales" action="?addform" method="post">

[Scobbo]

Ha Ha! Beautiful! Such a silly mistake. Thanks heaps! You have been so much help! Thanks!