OnClick button needs clicking twice to work?

Question

Very new to JS so bear with me - I'm trying to make a show/hide section on a page. It's working - but it needs to be clicked twice to work. I've looked at event handling and tried a couple examples I found on here but it's not working.

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

#topsection {
      width: 100%;
      padding: 50px 0;
      text-align: center;
      background-color: lightblue;
      margin-top: 20px;
      display: none;
}

<div id="topsection">
        This is a DIV element.
        </div>

<div id="section3">
    <button class="togglebutton" onclick="myFunction()">
        <img src="image.png" alt="buttonpng";>
    </button>
</div>

Any help would be appreciated!

I;ve placed your code into a runnable snippet, seems to be working just fine. — 0stone0, May 26 '23 at 11:12
@0stone0 - apologies, I forgot to add the CSS in, in the CSS I have the 'topsection' as display: none - and I think this is what's causing the issue, but I want the section to be hidden until the button is clicked. — poppythewitch, May 26 '23 at 11:14
Please [edit] your question to add the CSS to mimic a [mre]. — 0stone0, May 26 '23 at 11:15
Does this answer your question? [myDiv.style.display returns blank when set in master stylesheet](https://stackoverflow.com/questions/16748813/mydiv-style-display-returns-blank-when-set-in-master-stylesheet) — 0stone0, May 26 '23 at 11:18
@0stone0 updated, and the issue is now present in the snippet too. I've gone through the page you sent and it doesn't seem to be working either. — poppythewitch, May 26 '23 at 11:21
Well this is an exact duplicate, your solution is in the first answer of that linked question. — 0stone0, May 26 '23 at 11:25
@0stone0 - Added 'display:none' into the div itself and it worked. Cheers! — poppythewitch, May 26 '23 at 11:36
Glad it worked, you could also do `if ((x?.style?.display || "none") === "none")` or use `getComputedStyle` with a fallback. Please mark this as a duplicate. — 0stone0, May 26 '23 at 11:39

score 2 · Answer 1 · answered May 26 '23 at 11:13

In the code, style.display property returns an empty string if the inline style is not set. in your code, when you first click x.style.display is an empty string. To fix it, you can use window.getComputedStyle instead of style.display.

function myFunction() {
  var x = document.getElementById("topsection");
  var computedStyle = window.getComputedStyle(x);

  if (computedStyle.display === "none") {
    x.style.display = "block";
  } else {
    x.style.display = "none";
  }
}

<div id="topsection">
  This is a DIV element.
</div>

<div id="section3">
  <button class="togglebutton" onclick="myFunction()">
    <img src="image.png" alt="buttonpng" />
  </button>
</div>

Gass · Answer 2 · 2023-05-26T11:44:45.157

0

The problem is in this line if (x.style.display === "none") {

x.style.display is undefined the first time you run the code not a string --> "none"

And so, if you change that conditional statement for if(!x.style.display) or if(x.style.display == undefined) it will work with just one click.

Why is it working with two clicks?

It is because the first time you run the code the if statement is not met and so, the else {} is executed defining that specific style to "none".

And so you get what happends the second time you click. Right?

Second issue

Replace your else { x.style.display = "none"} for else {x.style.display = null} so that you undefine the style prop

You can also achieve the same thing by using: x.style.removeProperty('display')

edited May 26 '23 at 11:44

answered May 26 '23 at 11:23

Gass

7,536
3
37
41

1

YAY sorted! Thank you!!!!!! – poppythewitch May 26 '23 at 11:25
Then it wont work the second time – 0stone0 May 26 '23 at 11:25
@0stone0 - I just noticed that - it is working the first time but it won't 'show' again after it's been hidden. – poppythewitch May 26 '23 at 11:30
It would be a lot easier to just use a fallback for the empty string – 0stone0 May 26 '23 at 11:32
@poppythewitch updated. it will work now, all the time ;) – Gass May 26 '23 at 11:36
null is not a valid css value..... use unset – 0stone0 May 26 '23 at 11:38

score -1 · Answer 3 · answered May 26 '23 at 11:39

Mitesh Dudhat has the correct answer to fix your immediate problem but there is another solution.

If you swap out your id for a class and use a separate class set to display: block you can use classList to toggle that class on/off.

The reason for swapping out the id for a class is due to ids ranking higher in specificity in the CSS cascade - using a class means that you wouldn't need to add an !important property to the new class rule.

Note: in this example I've removed the inline JavaScript and used querySelector to cache the DOM elements, and addEventListener to add a listener to the button.

const section = document.querySelector('.topsection');
const button = document.querySelector('.togglebutton');

button.addEventListener('click', handleClick);

function handleClick() {
  section.classList.toggle('show');
}

.topsection {
  width: 100%;
  padding: 50px 0;
  text-align: center;
  background-color: lightblue;
  margin-top: 20px;
  display: none;
}

.show {
  display: block !important;
}

<div class="topsection">
  This is a DIV element.
</div>

<div id="section3">
  <button class="togglebutton">
    Toggle section    
  </button>
</div>

there are many different ways of solving a problem. There is nothing like just one solution. You are making a major change on the code of the OP rather than giving hints to improve his errors and answer his question. — Gass, May 26 '23 at 11:46

OnClick button needs clicking twice to work?

3 Answers3