Modal not closing when I click X or outside

Question

The modals I have set up for my website open but do not close when I click the X button or outside.

When I run my code in a browser I can click the button and it works. But then it will not close. I am not sure what the error is or why this does not work.

var modal = document.getElementById("WhiteSedan1")
var btn1 = document.getElementById("BtnWhiteSedan")
var span = document.getElementsByClassName("close")[0];

btn1.onclick = function() {
  modal.style.display = "block";
}
span.onclick = function() {
  modal.style.display = "none";
}
window.onclick = function(event) {
  if (event.target == modal) {
    modal.style.display = "none";
  }
}

.modal {
  display: none;
  /* Hidden by default */
  position: fixed;
  /* Stay in place */
  z-index: 1;
  /* Sit on top */
  padding-top: 50x;
  /* Location of the box */
  left: 0;
  top: 0;
  width: 50%;
  /* Full width */
  height: 100%;
  /* Full height */
  overflow: auto;
  /* Enable scroll if needed */
  background-color: rgb(0, 0, 0);
  /* Fallback color */
  background-color: rgba(0, 0, 0, 0);
  /* Black w/ opacity */
}


/* Modal Content */

.modal-content {
  background-color: #fefefe;
  margin: auto;
  padding: 20px;
  border: 1px solid #888;
  width: 80%;
}


/* The Close Button */

.close {
  color: #aaaaaa;
  float: right;
  font-size: 28px;
  font-weight: bold;
}

.close:hover,
.close:focus {
  color: #000;
  text-decoration: none;
  cursor: pointer;
}

<div class="desc">
  <a href="#" class="btn btn-outline-primary" id="BtnWhiteSedan">White Sedan</a>
</div>

<div id="WhiteSedan1" class="modal">
  <div class="modal-content">
    <span class="close">&times;</span>
    <p class="text-center">
      Model: Toyota<br> Mileage: 28,000 km <br> Transmission: Auto<br> Cost: $10,000
    </p>
  </div>

</div>

My goal is when I click the X or outside the modal, the modal closes.

Answer 1

I've fixed some problems:

• height of the modal, taking the 100% of the space so influencing the click
• put a listener on the document, which will trigger the modal if the status is block or if it's the button.

var modal = document.getElementById("WhiteSedan1")
var btn1 = document.getElementById("BtnWhiteSedan")
var span = document.getElementsByClassName("close")[0];

window.onclick = function(event) {
  if(btn1.contains(event.target)){
    modal.style.display = "block";
  }else{
    if (!modal.contains(event.target) && modal.style.display === "block" ||  span.contains(event.target)) {
      modal.style.display = "none";
    }
  }
}

.modal {
  display: none;
  /* Hidden by default */
  position: fixed;
  /* Stay in place */
  z-index: 1;
  /* Sit on top */
  padding-top: 50x;
  /* Location of the box */
  left: 0;
  top: 0;
  width: 50%;
  overflow: auto;
  height: auto;
  z-index: 1px;
  /* Enable scroll if needed */
  background-color: rgb(0, 0, 0);
  /* Fallback color */
  background-color: rgba(0, 0, 0, 0);
  /* Black w/ opacity */
}


/* Modal Content */

.modal-content {
  background-color: #fefefe;
  margin: auto;
  padding: 20px;
  border: 1px solid #888;
  width: 80%;
}


/* The Close Button */

.close {
  color: #aaaaaa;
  float: right;
  font-size: 28px;
  font-weight: bold;
}

.close:hover,
.close:focus {
  color: #000;
  text-decoration: none;
  cursor: pointer;
}

<div class="desc">
  <a href="#" class="btn btn-outline-primary" id="BtnWhiteSedan">White Sedan</a>
</div>

<div id="WhiteSedan1" class="modal">
  <div class="modal-content">
    <span class="close">&times;</span>
    <p class="text-center">
      Model: Toyota<br> Mileage: 28,000 km <br> Transmission: Auto<br> Cost: $10,000
    </p>
  </div>

</div>

Answer 2

I’d suggest combining all opening / closing modal event listeners into one, otherwise multiple event listeners run consecutively, but you only want one action to happen.

One way of achieving this behavior is to check for event.target: if the BtnWhiteSedan element is clicked, open the modal; otherwise, if neither the modal nor anything inside the modal is clicked, with the exception of the × button, close the modal. See Node.prototype.contains.

Since event is only used for event.target, use destructuring to get the target property directly.

const modal = document.getElementById("WhiteSedan1"),
  openButton = document.getElementById("BtnWhiteSedan"),
  [
    closeButton
  ] = document.getElementsByClassName("close");

addEventListener("click", ({target}) => {
  if (target === openButton) {
    modal.hidden = false;
  }
  else if(target !== modal && !modal.contains(target) || target === closeButton){
    modal.hidden = true;
  }
});

.modal {
  position: fixed;
  z-index: 1;
  padding-top: 50px;
  left: 0;
  top: 0;
  width: 50%;
  height: 100%;
  overflow: auto;
  background-color: rgb(0, 0, 0);
  background-color: rgba(0, 0, 0, 0);
}
.modal-content {
  background-color: #fefefe;
  margin: auto;
  padding: 20px;
  border: 1px solid #888;
  width: 80%;
}
.close {
  color: #aaaaaa;
  float: right;
  font-size: 28px;
  font-weight: bold;
}
.close:hover, .close:focus {
  color: #000;
  text-decoration: none;
  cursor: pointer;
}

<div class="desc">
  <a id="BtnWhiteSedan" class="btn btn-outline-primary" href="#">White Sedan</a>
</div>
<div id="WhiteSedan1" class="modal" hidden>
  <div class="modal-content">
    <span class="close">&times;</span>
    <p class="text-center">Model: Toyota<br> Mileage: 28,000 km<br>Transmission: Auto<br> Cost: $10,000</p>
  </div>
</div>

It’s not always robust to check for CSS properties. Use the hidden attribute instead, or use a class name and modal.classList.has("hidden"), modal.classList.add("hidden"), modal.classList.remove("hidden"), with .hidden { display: none; } in your CSS. See Element.prototpye.classList. If you do use the hidden attribute, remove the CSS default, and simply add a hidden attribute to your modal as I’ve done in the code above.

I’ve also replaced var by const, onclick by addEventListener, and abstract equality by strict equality. I’ve also used more semantic variable names (e.g. closeButton rather than span).

There was also a typo in your CSS: padding-top: 50x; instead of padding-top: 50px;.