Weird Javascript Bug (jQuery) in simple Testpage

Question

I'm in the process of writing a Mediawiki extension. I'm actually at a very very early stage ;). You can find the code here (okay, i can only submit one link, so imagine a github url) /eugenkiss/discussion-extension

I've got a weird jQuery problem that I just can't resolve even by utilising firebug and trying to debug my code. I've uploaded the current code and an example here: http://jsfiddle.net/NMpU5/

Try to open a discussion and click on at least two "reply"-links. Then click the cancel button of the first form that appeared. I don't know why, but when you click the cancel button another form is closed instead of the desired one.

You can also vary this. For instance, open two forms and close the last openend one. At first it seems to work. But when you try to close the other form (by clicking cancel) it does not disappear. However, the event is triggered as shown by firebug. Sometimes, when I click another reply anchor after that, there will be opened as much forms as I clicked the seemingly not working cancel button of the other form.

Well, for my desired extension I could of course limit the existence of opened forms to one - why else would you need two or more opened? But I just want to find the damn bug since I already invested a lot of time in finding it! It's a precious bug for me, you know ;)

BTW, I'm using jQuery 1.4.2

javascript


$(document).ready(function() {
    // Hide the discussion bodys per default
    $(".discussion").addClass("closed")
        .children(".discussion-body").hide();

    // Register two custom events for the individual discussion divs      
    // "open" & "close" in order to make the discussion bodys
    // collapsable and be able to toggle the events by clicking
    // the "discussion-header-button" anchor
    $(".discussion")
    .bind("open", function(e) {
       if(!$(this).hasClass("opened")) {
           $(this).children(".discussion-body").slideDown();
           $(this).find(".discussion-header-button").html("[-]");
           $(this).addClass("opened");
           $(this).removeClass("closed");
       }
    })
    .bind("close", function(e) {
       if(!$(this).hasClass("closed")) {
           $(this).children(".discussion-body").slideUp();
           $(this).find(".discussion-header-button").html("[+]");
           $(this).addClass("closed");
           $(this).removeClass("opened");
       }
    })
    .find(".discussion-header-button").click(function(){
        relatedDiscussion = $(this).parents(".discussion");
        if(relatedDiscussion.hasClass("closed")) {
            relatedDiscussion.trigger("open");
        }
        else if(relatedDiscussion.hasClass("opened")) {
            relatedDiscussion.trigger("close");
        }
    });

    // Register custom "showForm" & "destroyForm" events on posts       
    // in order to make the "Reply" button work
    // TODO: Maybe add "preview" & "submit"
    $(".discussion-body .post")
    .bind({
        showForm: function(){
            post = $(this);
            postBody = post.find(".post-body").first();
            postHeader = post.find(".post-header").first();

            postBody.append(postCommentFormHtml);
            replyLink = postHeader.find(".reply");
            replyLink.unbind();

            form = postBody.find(".post-comment-form");
            form.slideDown();

            // create actions for the form buttons
            form.find(".cancel").click(function(){
                post.triggerHandler("destroyForm");
            });
            form.find(".preview").click(function(){
                // Hier muss mit Ajax und der Datenbank gespielt
                // werden um ein preview erstellen zu können
            });
            form.find(".submit").click(function(){
                // Hier muss mit Ajax und der Datenbank gespielt
                // werden um den Post abschicken zu können
            });
        },
        destroyForm: function(){
            post = $(this);
            postBody = post.find(".post-body").first();
            postHeader = post.find(".post-header").first();

            replyLink = postHeader.find(".reply");
            replyLink.click(replyAction);

            form = postBody.find(".post-comment-form");
            form.slideUp(function(){
                    $(this).remove();
            });
        }
    });
    //$(".discussion-post-comment").click(createPostCommentForm);
    $(".discussion .reply").click(replyAction);

    function replyAction(event){
        // Note: It is important to use triggerHandler instead of trigger
        // otherwise the "showForm" event of all parents is triggered
        // recursively (bubbling) and this is not desired
        event.preventDefault();
        relatedPost = $(this).parents(".post").first();
        relatedPost.triggerHandler("showForm");
    }
});
postCommentFormHtml = "\
    <div class='post-comment-form' style='display:none;'><br>\
    <form action='textarea.htm'>\
        <textarea name='post' cols='50' rows='8'></textarea>\
        <br>\
        <input class='submit' type='submit' value=' Post '>\
        <input class='preview' type='submit' value=' Preview '>\
        <input class='cancel'type='reset' value=' Cancel '>\
    </form>\
    </div>";

Html


<div class="discussion">
<div class="discussion-header">
    <a class="discussion-header-button">[+]</a>
    Diskussion: 3 Kommentar(e)
    <a class="discussion-post-comment">Post Comment</a>
</div>
<div class="discussion-body">
<div class="post">
    <div class="post-header">
        <span class="post-header-name">Eugen</span>
        <span class="post-header-date">2010-02-25 12:32:30</span>
        <a class="reply" href="#">Reply</a>
        <a class="edit" href="#">Edit</a>
        <a class="delete" href="#">Delete</a>
    </div>
    <div class="post-body">
        Ich denke das sollte anders sein!
    </div>
    <div class="post">
        <div class="post-header">
            <span class="post-header-name">Markus</span>
            <span class="post-header-date">2010-02-25 12:32:31</span>
            <a class="reply" href="#">Reply</a>
         <a class="edit" href="#">Edit</a>
         <a class="delete" href="#">Delete</a>
        </div>
        <div class="post-body">
            Ich denke nicht
        </div>
    </div>
</div>
<div class="post"> 
    <div class="post-header">
        <span class="post-header-name">Jan</span> 
        <span class="post-header-date">2010-03-25 12:32:30</span>
        <a class="reply" href="#">Reply</a>
        <a class="edit" href="#">Edit</a>
        <a class="delete" href="#">Delete</a>
    </div>
    <div class="post-body">
        Mal was ganz anderes: Denkt ihr das selbe was ich denke?
    </div>
</div>
</div>
</div>

Edit: I want to add that changing the id's to classes didn't help. Plus, if that helps you: I discovered (using Firebug) that the "post" (and therefore "postbody") variable (in the "destroyForm" event) is actually pointing at the wrong post and therefore the wrong form is deleted. But it is beyond me why the post variable is pointing at the wrong post in the first place

Edit2: changed alls ids to classes http://jsfiddle.net/NMpU5/1/

You have to change the "id" values so that they are unique anyway, so do that, and then maybe people can help. As it is, it's so wrong that it's not worth debugging. — Pointy
– Pointy, Commented Mar 1, 2010 at 13:42
I did that jsfiddle.net/NMpU5/1 but the bug is still there. Shall I update the code here in stackoverflow with the new version with classes instead of ids? — Eugen
– Eugen, Commented Mar 1, 2010 at 13:50

Pointy · Accepted Answer · 2010-03-01 14:50:27Z

3

It looks to me as if many of the variables in your event handler functions (notably, "post" and "relatedDiscussion") are not declared with "var" in each function. I've tried to figure out exactly what that might do, but I got confused. Nevertheless, when you do not declare your local variables, they're global variables. That means that each function that sets "post" equal to some new value is changing the value used by "post" in all the other functions that may be active.

Change it to

 var post = $(this);

etc.

edited Mar 1, 2010 at 14:50

answered Mar 1, 2010 at 14:41

Pointy

415k62 gold badges600 silver badges631 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

Eugen Over a year ago

Oh thank you, that's been the bug! That's what I get for not remebering javascript basics! I actually wondered that firebug showed me that the (not so) local variables had already a value before assigning s.th. to them. But I assumed that it's been just an inconsistency in firebug. you can see the updated version here jsfiddle.net/NMpU5/4 Again thanks ;)

user241244 · Accepted Answer · 2010-03-01 13:32:21Z

3

First thing that strikes my eye: IDs should be unique. Change your IDs for A tags into classes, and see if that clears it up.

answered Mar 1, 2010 at 13:32

user241244

2 Comments

Eugen Over a year ago

yes i tried that (in a git branch) but It did not solve my problem, so i reverted

Pointy Over a year ago

Try it again - you cannot share "id" values between multiple elements. Every "id" must be perfectly unique on any page.

Mottie · Accepted Answer · 2010-03-01 14:52:31Z

1

You should use jQuery's live events to bind to the form instead of binding a click to each button after creation.

I posted an update to your script

I basically pulled out the form click functions and converted them to live functions

$(".cancel").live("click", function(){
    $(this).closest(".post-comment-form").slideUp('',function(){ $(this).remove(); });
});
$(".preview").live("click", function(){
    // Hier muss mit Ajax und der Datenbank gespielt
    // werden um ein preview erstellen zu können
});
$(".submit").live("click", function(){
    // Hier muss mit Ajax und der Datenbank gespielt
    // werden um den Post abschicken zu können
});

I didn't use your post.triggerHandler("destroyForm"); function because frankly I've never used it before and I couldn't get it to work LOL.

edited Mar 1, 2010 at 14:52

answered Mar 1, 2010 at 14:47

Mottie

86.7k30 gold badges132 silver badges248 bronze badges

2 Comments

Mottie Over a year ago

You might also want to look into changing .parents() into .closest(). The code gets very confusing when I'm trying to figure out where in the post and how far into the replies you are. I'd say remove your .bind() functions entirely and replace them with .live()

Eugen Over a year ago

thanks for the remarks but the probelm is already solved. The destroyForm is a custom event I created with bind(). I'll look into closest().

Pointy · Accepted Answer · 2010-03-01 13:39:35Z

0

When you use jQuery selectors like '#something', the library uses "document.getElementById()" to find the elements. You cannot expect things to work if you're using the same "id" value for multiple elements; in fact you'll get results exactly as you describe. The fact that you're using "find()" to look for elements by shared "id" does not matter.

That problem aside, you need to be using "name" attributes for your form input fields anyway.

answered Mar 1, 2010 at 13:39

Pointy

415k62 gold badges600 silver badges631 bronze badges

1 Comment

Eugen Over a year ago

Okay I changed all ids to classes but the bug is still there jsfiddle.net/NMpU5/1 Thanks for the remark about the form fields

Ju Nogueira · Accepted Answer · 2010-03-01 13:58:14Z

-1

you don't have to change de IDs to classes. you just need unique ids to each element, so you can get the right one.

answered Mar 1, 2010 at 13:58

Ju Nogueira

8,4692 gold badges31 silver badges33 bronze badges

1 Comment

Eugen Over a year ago

hmm okay i think that would work somehow. But anyway, the jQuery selectors should find find the right form, because I nested it that way, that find() should find the right form

Collectives™ on Stack Overflow

Weird Javascript Bug (jQuery) in simple Testpage

5 Answers 5

1 Comment

2 Comments

2 Comments

1 Comment

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

1 Comment

2 Comments

2 Comments

1 Comment

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related