Onclick attribute wont change with javascript

Question

3.00/5 (1 vote)

See more:

I have an image people click:

HTML

<img id="himg" src=""  önclick="menu(1);" />

And i have javascript to change the "onclick" attribute:

JavaScript

function change(){
    var y = "menu(0)";
    return y;
}
function change1(){
    var y = "menu(1)";
    return y;
}
function menu(x){
    if(x==1){
    document.getElementById("menu").innerHTML="<h3><ul><li>Home</li><li>About</li><li>Profile</li><li>Coins</li><li>News</li></ul></h3>";
    document.getElementById("himg").onclick = change();
    }
    if(x===0){
        document.getElementById("menu").innerHTML=" ";
        document.getElementById("himg").onclick = change1();
    }
}

I dont know whats happening, but the onclick doesnt change!!!

Posted 21-Mar-14 9:37am

G4mm4R4y

Add a Solution

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

**Zoltán Zörgő** · Answer 1 · 2014-03-21T09:44:00

Solution 1

0) What you do is not cross-browser.
1) Use jquery, forget plain javascript: https://api.jquery.com/category/events/event-handler-attachment/[^], more precisely the bind[^] method.
2) by adding round brackets (= change1()) you don't assign the function, you call the function and assign it's result. But this is not what you want.
3) Simply remove brackets: = change1;

Posted 21-Mar-14 9:44am

Zoltán Zörgő

Updated 21-Mar-14 9:49am

v2

Comments

G4mm4R4y 21-Mar-14 15:48pm

Nothing worked...

Zoltán Zörgő 21-Mar-14 15:50pm

See update: assigning this way is working only in specific browsers. Use jquery instead!

G4mm4R4y 21-Mar-14 15:51pm

it doesnt work on chrome and internet explorer, isnt there any way without jquery?

G4mm4R4y 21-Mar-14 15:50pm

... A "Is not cross browser" is not enough. Does it work on chrome?

Zoltán Zörgő 21-Mar-14 15:59pm

What's wrong with jquery?
Still, it is working in chrome:


<div id="x" onclick="alert(1);">XXXXXXXXXXXXXXXX</div>
<script>
function click2() {alert(2);}

document.getElementById("x").onclick = click2;
</script>

G4mm4R4y 21-Mar-14 16:02pm

the exact code above i posted does not work, neither does yours... Idk whats happening then because i tried it in chrome, firefox, and ie.

G4mm4R4y 21-Mar-14 16:06pm

i got it working with this:

function menu(x){
if(x==1){
x=x-1
document.getElementById("menu").innerHTML="

<ul><li>Home</li><li>About</li><li>Profile</li><li>Coins</li><li>News</li></ul>

";
}else{
x=x+1
document.getElementById("menu").innerHTML=" ";
}
document.getElementById('mimg').innerHTML='<img id="himg" önclick="menu('+ x +');" height="40px" width="40px" src="menu.png" />'

}

Zoltán Zörgő 21-Mar-14 16:21pm

Oh, I re-read you code. It won't work that way. Check this code:


<div id="x" onclick="alert(1);">XXXXXXXXXXXXXXXX</div>

<script>
function callThis(v) {alert(v);}

function change(obj) { obj.onclick = function(){callThis(100);};}

change(document.getElementById("x"));
</script>

But I am pretty sure you could rethink your design...

Sergey Alexandrovich Kryukov 21-Mar-14 16:31pm

Good catch, reasonable suggestions, my 5.
—SA

Sergey Alexandrovich Kryukov 21-Mar-14 16:40pm

I added one more suggestion in Solution 2, please see.
—SA

Sergey Alexandrovich Kryukov · Answer 2 · 2014-03-21T10:40:00

In addition to good suggestions by Zoltán Zörgő in Solution 1, I would suggest one more thing.

I think, in most cases dynamic change of the handler of the element is a bad style of programming. You can do it, but it makes the code purely supportable, especially if you have more options than x==0 and x==1. Moreover, in such approach, this 0 and 1 becomes some "magic numbers" not connected with its semantics which would be clear if you had properly named variables carrying the menu options.

You can get much better, and, more importantly, more maintainable results if you used more traditional method, never changing the handler (or setting a handler to an HTML element only once); and yes, preferably using jQuery. For this purpose, you should use only one handler, where you implement merged behavior from your current change and change1 (one more semantically non-sound naming). Inside the handler, you could have the "if" or "switch" block with branches representing all the menu choices, and implementation of the response to each branch could call different or the same functions.

—SA