Why does my button need to be clicked twice for the event handler to work the first time, but thereafter just once?

Question

I want the visitor to be able to expand/collapse some sections, and am using:

<input onclick="return toggleDiv('xx')" 
       type="button" 
       class="button" 
       value="click here to expand/collapse"/>

and in the I have the function:

function toggleDiv(a){
  var e=document.getElementById(a);
  if(!e)return true;
  if(e.style.display=="none"){
    e.style.display="block"
  } else {
    e.style.display="none"
  }
  return true;
}

The first time a button is clicked it doesn't work, subsequent clicks (on any of the buttons) work OK.

There is related conversation here: Button needs to be clicked twice to trigger function

but I don't understand the answer (too technical;-), could someone help explain it please?

Answer 1

The initial style on your 'xx' div may be causing some trouble...

Explanation

Say you have a stylesheet rule configured to make your divs initially hidden. Something like:

div { display: none }

(...where of course the selector (div) will probably be a little bit less broad)

This would appear to work correctly, in that the page will load with all of your div elements hidden ("collapsed"). However, there's no actual value for the style.display property on the elements themselves - they're merely inheriting the value from the stylesheet. So in your code, the test you're using to check if the element is hidden:

if(e.style.display=="none"){

...will fail, incorrectly identifying the target as being visible, and causing the style.display property to be set to "none" (which has no visible effect, since the element had already been hidden by the stylesheet). Then the next time the button is clicked, the value you set the last time around will cause the text to succeed, and your routine will set style.display to "block".

The easy way to correct for this is to simply reverse your test and check for "block":

  if(e.style.display=="block"){
    e.style.display="none"
  } else {
    e.style.display="block"
  }

...however, this will fall apart if you have some elements configured to be initially visible: you'll just run into the same problem in reverse, with the first button click failing to have any visible effect. For a more robust behavior, you'll need to test the style that's actually active on the element:

function getStyle(el, name)
{
  // the way of the DOM
  if ( document.defaultView && document.defaultView.getComputedStyle )
  {
    var style = document.defaultView.getComputedStyle(el, null);
    if ( style )
      return style[name];
  }
  // IE-specific
  else if ( el.currentStyle )
    return el.currentStyle[name];
  
  return null;
}

function toggleDiv(a){
  var e=document.getElementById(a);
  if(!e)return true;
  if(getStyle(e, "display") == "none"){
    e.style.display="block"
  } else {
    e.style.display="none"
  }
  return true;
}

Here we use a helper function, getStyle(), to retrieve the active value in a cross-platform manner. See also: getComputedStyle(), currentStyle

Answer 2

more easy implémentation some how

var a=1;
function toggleDiv(xx){
  var e=document.getElementById(xx);

  if(a==1){
    e.style.display="block";a=0;
  } else {
    e.style.display="none";
  a=1;
  }

}

Answer 3

To save all the extra Javascript code to get the style etc, a simple fix for this would be to add the hide CSS to the element itself.

<div class="yourclass" style="display:none;">content</div>

edit

I decided to explain my answer a little better. I'm assuming you didn't hide the div you wanted to toggle by default, so it was visible, however when calling e.style.display the result would not be none neither would it be block because the style hasn't been set yet.

Instead, you were retrieving an empty string which means the first time you clicked it, your else statement was firing; the div display was being set to none on first click, that way the next click would retrieve the value as none so of course would then change it to block.

Answer 4

The simple answer you're looking for is to add the display:none; to the inline style. This way, when you look at

if (x.style.display == 'none')

it will return true!

Answer 5

I know this is an old thread, but for the sake of being somewhat useful (for a change), you could always call the current display within the function. alternatively you could define on-load, but given the button might not be clicked, I would prefer to do via a addEventListener().

document.getElementById("someID1").addEventListener("click", hideOpen);

function hideOpen() {
  var x = document.getElementById("someID2");
  x.style.display = window.getComputedStyle(document.getElementById("someID2")).display;
  if (x.style.display === "none") {
    x.style.display = "block";
  } else {
    x.style.display = "none";
  }
}

Effectively you load the JS, then when an element is clicked (someID1), the function will define varible X, assign the current computed style.display and then test the if it's block or none.

Answer 6

Something like this might work for you. But put it in the head of the document. the window onload function is important because the DOM needs to be loaded before you can mess around with the input field

<input id="divOpener" rel="xx" type="button" class="button" value="click here to expand/collapse"/>

and this

window.onload = function(){
            //  get input field from html
            var myInput = document.getElementById('divOpener');
            // add onclick handler
            myInput.onclick = function(){
               // if you really need to say which div in the document body, you can
               // add it to the rel or ref etc. attributes then get the value with
               // get attribute
               var myDiv = document.getElementById(this.getAttribute('rel'));
               if(!myDiv) return;
               if(myDiv.style.display=="none"){
                  myDiv.style.display="block";
               } else {
                 myDiv.style.display="none";
               }
            }
        }

Answer 7

Shog9's answer saved me from an evening's frustration. Thanks.

However there's a much simpler solution than the one he presents. The secret is simply to switch around the order of the conditional. i.e.

if(e.style.display == "block"){
    e.style.display="none"
} else {
    e.style.display="block"
}

The insight here is simply that the condition can fail for two different reasons: if the value is unobtainable it will do the right thing anyway.