Why does adding a second dropdown mess up the JS to close menu when user clicks outside it?

Question

I'm not super familiar with JS. I used the W3Schools tutorial for creating an on-click dropdown menu as a reference and added a second menu. However, only the second dropdown menu listed in the javascript maintains the functionality of closing when the user clicks outside the dropdown. (I can switch the order of the functions listed in the JS, and changing nothing else, that switches which menu has that close-when-click-outside functionality.)

Can anyone help me understand why that is? How to fix it would be a bonus but mostly I just don't get why it works for one menu and not the other.

/* When the user clicks on the button, 
toggle between hiding and showing the dropdown content */
function drop1() {
  document.getElementById("drop1").classList.toggle("show");
}

// Close the dropdown if the user clicks outside of it
window.onclick = function(e) {
  if (!e.target.matches('.dropbtn1')) {
    var drop1 = document.getElementById("drop1");
    if (drop1.classList.contains('show')) {
      drop1.classList.remove('show');
    }
  }
}

/* When the user clicks on the button, 
toggle between hiding and showing the dropdown content */
function drop2() {
  document.getElementById("drop2").classList.toggle("show");
}

// Close the dropdown if the user clicks outside of it
window.onclick = function(e) {
  if (!e.target.matches('.dropbtn2')) {
    var drop2 = document.getElementById("drop2");
    if (drop2.classList.contains('show')) {
      drop2.classList.remove('show');
    }
  }
}

.navbar {
  overflow: hidden;
  background-color: #333;
  font-family: Arial, Helvetica, sans-serif;
}

.navbar a {
  float: left;
  font-size: 16px;
  color: white;
  text-align: center;
  padding: 14px 16px;
  text-decoration: none;
}

.dropdown {
  float: left;
  overflow: hidden;
}

.dropbtn1,
.dropbtn2 {
  cursor: pointer;
  font-size: 16px;
  border: none;
  outline: none;
  color: white;
  padding: 14px 16px;
  background-color: inherit;
  font-family: inherit;
  margin: 0;
}

.navbar a:hover,
.dropdown:hover .dropbtn,
.dropbtn:focus {
  background-color: red;
}

.dropdown-content {
  display: none;
  position: absolute;
  background-color: #f9f9f9;
  min-width: 160px;
  box-shadow: 0px 8px 16px 0px rgba(0, 0, 0, 0.2);
  z-index: 1;
}

.dropdown-content a {
  float: none;
  color: black;
  padding: 12px 16px;
  text-decoration: none;
  display: block;
  text-align: left;
}

.dropdown-content a:hover {
  background-color: #ddd;
}

.show {
  display: block;
}

<div class="navbar">
  <a href="#home">Home</a>
  <a href="#news">News</a>
  <div class="dropdown">
    <button class="dropbtn1" onclick="drop1()">Dropdown
    &nbsp; +
  </button>
    <div class="dropdown-content" id="drop1">
      <a href="#">Link 1</a>
      <a href="#">Link 2</a>
      <a href="#">Link 3</a>
    </div>
  </div>
  <div class="dropdown">
    <button class="dropbtn2" onclick="drop2()">Dropdown 2
    &nbsp; +
  </button>
    <div class="dropdown-content" id="drop2">
      <a href="#">Link 4</a>
      <a href="#">Link 5</a>
      <a href="#">Link 6</a>
    </div>
  </div>
</div>

<h3>Dropdown Menu inside a Navigation Bar</h3>
<p>Click on the "Dropdown" link to see the dropdown menu.</p>

Thank you!

mplungjan · Answer 1 · 2021-12-12T07:45:39.657

You are only allowed to have one onclick. The second will overwrite the first

Instead use eventListener and delegation

Notice I removed the inline click and I now only have one class instead of a class per button

window.addEventListener("load", function() {
  // click the dropdown if the user clicks outside it unless that is a button
  document.addEventListener("click", function(e) {
    const tgt1 = e.target.closest('.dropdown-content');
    const tgt2 = e.target.closest('.dropbtn');
    if (!tgt1 && !tgt2) {
      document.querySelectorAll('.dropdown-content').forEach(div => div.classList.remove('show'));
    }
  })
  document.querySelector(".navbar").addEventListener("click", function(e) {
    const tgt = e.target.closest("button");
    if (tgt && tgt.matches('.dropbtn')) {
      document.querySelectorAll('.dropdown-content').forEach(div => div.classList.remove('show'));
      document.getElementById(tgt.dataset.id).classList.add('show');
    }
  })
})

.navbar {
  overflow: hidden;
  background-color: #333;
  font-family: Arial, Helvetica, sans-serif;
}

.navbar a {
  float: left;
  font-size: 16px;
  color: white;
  text-align: center;
  padding: 14px 16px;
  text-decoration: none;
}

.dropdown {
  float: left;
  overflow: hidden;
}

.dropbtn {
  cursor: pointer;
  font-size: 16px;
  border: none;
  outline: none;
  color: white;
  padding: 14px 16px;
  background-color: inherit;
  font-family: inherit;
  margin: 0;
}

.navbar a:hover,
.dropdown:hover .dropbtn,
.dropbtn:focus {
  background-color: red;
}

.dropdown-content {
  display: none;
  position: absolute;
  background-color: #f9f9f9;
  min-width: 160px;
  box-shadow: 0px 8px 16px 0px rgba(0, 0, 0, 0.2);
  z-index: 1;
}

.dropdown-content a {
  float: none;
  color: black;
  padding: 12px 16px;
  text-decoration: none;
  display: block;
  text-align: left;
}

.dropdown-content a:hover {
  background-color: #ddd;
}

.show {
  display: block;
}

<div class="navbar">
  <a href="#home">Home</a>
  <a href="#news">News</a>
  <div class="dropdown">
    <button class="dropbtn" data-id="drop1">Dropdown
    &nbsp; +
  </button>
    <div class="dropdown-content" id="drop1">
      <a href="#">Link 1</a>
      <a href="#">Link 2</a>
      <a href="#">Link 3</a>
    </div>
  </div>
  <div class="dropdown">
    <button class="dropbtn" data-id="drop2">Dropdown 2
    &nbsp; +
  </button>
    <div class="dropdown-content" id="drop2">
      <a href="#">Link 4</a>
      <a href="#">Link 5</a>
      <a href="#">Link 6</a>
    </div>
  </div>
</div>

<h3>Dropdown Menu inside a Navigation Bar</h3>
<p>Click on the "Dropdown" link to see the dropdown menu.</p>

This may or may not be a better solution, but it is important to answer the actual question, which is very specific. If questions are required to be specific and not "please write this code for me" and "what is the best design?", answers should follow similar guidelines ;) This isn't a show-off platform ;) Answering the actual question is even more important for an asker who is learning. — Inigo, Dec 12 '21 at 06:25
`document.querySelectorAll('.dropdown-content').forEach(div => div.classList.remove('show')); document.getElementById(tgt.dataset.id).classList.add('show'); }` can be simplified to `document.querySelectorAll('.dropdown-content').forEach(div => div.classList.toggle('show', div === document.getElementById(tgt.dataset.id))); }` — connexo, Dec 12 '21 at 06:45
Again, @connexo, this is not a design question. Read the Question in the title as well as in the last paragraph of her question. — Inigo, Dec 12 '21 at 06:47
@Inigo Code questions are rarely ever design questions. Are you suggesting we should not advise with best practice approaches and instead make OPs think they picked a proper approach, by just doing the minimal required changes to make their code work? That is now how I understand the purpose of SO, at all. — connexo, Dec 12 '21 at 06:55
@connexo I have been and am a teacher. And a parent. (was an SV dev and later manager, but quit the industry to do good). Teaching is incremental. It's not about showing off or dazzing the learner when that's not what they are looking for. It's important to answer the actual question unless the question is fundamentally flawed. In this case it wasn't and makes total sense. It's fine to add a part two to the answer. But read the guidelines about asking questions. Not opinion based. Be specific and focused. The same rules apply to answers. In any case, the asker is getting it all now — Inigo, Dec 12 '21 at 07:05
@Inigo I have answered hundreds of thousands of js questions on the three biggest internet technologies websites since 1996. I do not answer to show off. OP is better off getting answers using recommended methods than using minimal changes to perpetuate the poor code given by w3schools - which used to be so bad, a site called w3fools was created to explain better code. — mplungjan, Dec 12 '21 at 07:21
Thank you for a more elegant solution. Between your answer and Inigo's I'm able to learn better how to be more efficient with JS. I'm better able to understand Inigo's answer because of my current understanding level of JS, but yours gives me something to pick through and try to figure out to advance my understanding. — kherezae, Dec 12 '21 at 07:43
So I give a better solution and get voted down. Very interesting. Curious as to why... — mplungjan, Dec 12 '21 at 12:28

Inigo · Accepted Answer · 2021-12-12T07:14:11.627

Can anyone help me understand why that is? How to fix it would be a bonus but mostly I just don't get why it works for one menu and not the other.

✨ I'm going to make the smallest possible change to your code to make it work, so that you can best learn what happened. I'm not going to redesign your approach.

window.onclick is a variable, and you are assigning a value to it twice. The second function you assign it, for Dropdown 2, overwrites the first, which was for Dropdown 1.

The problem is easily solved by combining the logic into one function assigned to window.onclick as below.

Another simple, and probably better fix, is to use window.addEventListener("click", function(event) { }) rather than window.onclick.

/* When the user clicks on the button, 
toggle between hiding and showing the dropdown content */
function drop1() {
  document.getElementById("drop1").classList.toggle("show");
}

/* When the user clicks on the button, 
toggle between hiding and showing the dropdown content */
function drop2() {
  document.getElementById("drop2").classList.toggle("show");
}

// Close the dropdown if the user clicks outside of it
window.onclick = function(e) {
  if (!e.target.matches('.dropbtn1')) {
    var drop1 = document.getElementById("drop1");
    if (drop1.classList.contains('show')) {
      drop1.classList.remove('show');
    }
  }
  if (!e.target.matches('.dropbtn2')) {
    var drop2 = document.getElementById("drop2");
    if (drop2.classList.contains('show')) {
      drop2.classList.remove('show');
    }
  }
}

.navbar {
  overflow: hidden;
  background-color: #333;
  font-family: Arial, Helvetica, sans-serif;
}

.navbar a {
  float: left;
  font-size: 16px;
  color: white;
  text-align: center;
  padding: 14px 16px;
  text-decoration: none;
}

.dropdown {
  float: left;
  overflow: hidden;
}

.dropbtn1, .dropbtn2 {
  cursor: pointer;
  font-size: 16px;  
  border: none;
  outline: none;
  color: white;
  padding: 14px 16px;
  background-color: inherit;
  font-family: inherit;
  margin: 0;
}

.navbar a:hover, .dropdown:hover .dropbtn, .dropbtn:focus {
  background-color: red;
}

.dropdown-content {
  display: none;
  position: absolute;
  background-color: #f9f9f9;
  min-width: 160px;
  box-shadow: 0px 8px 16px 0px rgba(0,0,0,0.2);
  z-index: 1;
}

.dropdown-content a {
  float: none;
  color: black;
  padding: 12px 16px;
  text-decoration: none;
  display: block;
  text-align: left;
}

.dropdown-content a:hover {
  background-color: #ddd;
}

.show {
  display: block;
}

<div class="navbar">
  <a href="#home">Home</a>
  <a href="#news">News</a>
  <div class="dropdown">
  <button class="dropbtn1" onclick="drop1()">Dropdown
    &nbsp; +
  </button>
  <div class="dropdown-content" id="drop1">
    <a href="#">Link 1</a>
    <a href="#">Link 2</a>
    <a href="#">Link 3</a>
  </div>
  </div> 
  <div class="dropdown">
  <button class="dropbtn2" onclick="drop2()">Dropdown 2
    &nbsp; +
  </button>
  <div class="dropdown-content" id="drop2">
    <a href="#">Link 4</a>
    <a href="#">Link 5</a>
    <a href="#">Link 6</a>
  </div>
  </div>
</div>

<h3>Dropdown Menu inside a Navigation Bar</h3>
<p>Click on the "Dropdown" link to see the dropdown menu.</p>

Sure, and I also could have suggested using `window.addEventListener("click", function(event) { });` But I'm answering the actual question, while you're answering a different one (a design question). SO is also a learning site, and it is important to answer the actual question. — Inigo, Dec 12 '21 at 06:23
@Inigo I don't agree with you answering the question in a better way. The design decision to go with `onclick` is exactly the problem here, and to teach OP to use `addEventListener` instead to overcome the limitation of `onclick` which is biting them, is a way better help and answer. — connexo, Dec 12 '21 at 06:49
*Why does adding a second dropdown mess up the JS to close menu when user clicks outside it?* Because they used `onclick` which can only meaningfully be assigned once. Use `addEventListener` instead. — connexo, Dec 12 '21 at 06:52
@connexo, sure that works too... But even then I'd make the minimal change, making two calls to `window.addEventListener("click"...`, rather than refactor the code and obfuscate the lesson. Feel free to update my answer as long as it stays true to the preface ;) — Inigo, Dec 12 '21 at 06:55
Thank you, that's very helpful. I caught some of the functions that needed to have changed names but did not realize onclick could only be used once; I see how you combined the functions under the one variable. I appreciate the explanation! — kherezae, Dec 12 '21 at 07:41

Why does adding a second dropdown mess up the JS to close menu when user clicks outside it?

2 Answers2