JavaScript for文からbreakでうまく抜けられていない事案を解決したいです

Question

JavaScript / for文からbreakでうまく抜けられていない事案を解決したい

※※1回目の回答をいただいたあと、kei344さんに作成いただいたサンプルを元に質問を編集させていただきました。
最初の質問では必要な範囲が記載しきれていませんでした。

---

Javascript初級者です。
Javascriptでモーダルウィンドウを実装しています。（#iconOpenをクリックすると#worksDetailWebを表示します）

「work-web」というクラスが付与されたdivをdisplay:noneで非表示にし、
「current」というクラスがついたdivのみdisplay:blockで表示しています。
「#iconRight」ボタンを押すとひとつ次のdivに「current」クラスを付与して内容を切り替える動きを実装しようとしたところ、for文が意図どおりに動作しませんでした。

該当のソースコード

<div id="iconOpen">iconOpen</div>

<div id="worksDetailWeb" class="dispnone">
 <div id="iconClose">iconClose</div>
 <div id="iconRight">iconRight</div>

 <div id="wrapper">
  <div class="work-web current" id="div01">div01</div>
  <div class="work-web" id="div02">div02</div>
  <div class="work-web" id="div03">div03</div>
  <div class="work-web" id="div04">div04</div>
  <div class="work-web" id="div05">div05</div>
 </div>
</div>

#worksDetailWeb {
  border: 1px solid #000;
}
.current {
  color: red;
}
.dispnone {
  display: none;
}

const worksDetailWeb = document.getElementById('worksDetailWeb');
const eachWebWorks = document.getElementsByClassName('work-web');
const iconRight = document.getElementById('iconRight');
const iconClose = document.getElementById('iconClose');
const iconOpen = document.getElementById('iconOpen');

function worksDetail(){
  //#iconRightをクリック→currentクラスを削除して1つ次のdivにcurrentクラスを付与する。
  iconRight.addEventListener('click', function (e) {
    for (i = 0; i < eachWebWorks.length-1; i++) {
      let self = eachWebWorks[i];
      if (self.classList.contains('current')) {
       self.classList.remove('current');
       eachWebWorks[i + 1].classList.add('current');
       break;
    }
   }
  });

  //#iconCloseをクリック→currentクラスを#div01に付与し直す。
  iconClose.addEventListener('click', function (e) {
    for (let eachWebWork of eachWebWorks) {
       if (eachWebWork.classList.contains('current')) {
          eachWebWork.classList.remove('current');
          eachWebWorks[0].classList.add('current');
          break;
       }
    }
     worksDetailWeb.classList.add('dispnone');
  });
}

//#iconOpenを開いたらモーダル部分を表示する
iconOpen.addEventListener('click', function (e) {
  e.preventDefault();
  if (worksDetailWeb.classList.contains('dispnone')) {
    worksDetailWeb.classList.remove('dispnone');
    worksDetail();
  }
})

発生している問題・エラーメッセージ

1回目は#div01→#div02→#div03→#div04→#div05
と期待した通り動いたのですが、一度#iconCloseボタンでモーダルを閉じ、再度開くと

2回目は#div01→#div03→#div05
3回目は#div01→#div04→#div05
4回目以降は#div01→#div05

という動きになってしまいました。

breakの前の行にconsole.logを入れて確認したところ、
表示されなかったdivの部分はbreakせずに次の処理に進んでしまっていました。

補足情報（FW/ツールのバージョンなど）

初めて質問させていただきます。質問の仕方に不備などありましたら申し訳ありません。
何卒よろしくお願いします。

Comments

[x_x]

モーダルウィンドウというのはどの部分のことを指していますか？

[miyabi_pudding]

質問に関係ない、コードのツッコミですが、
JavaScriptには、ASIがありますが、明示的に自分でセミコロン記述するようにした方がいいですよ。
（他の言語では全く通用しない）
さすがに全行のステートメントにセミコロンついてないのはちょっと・・・。Swiftじゃないんだから。

[pecoro0321]

恐れ入ります、ご指摘を受けて質問を編集させていただきました。
また、セミコロンについても「省略可能」というふんわりした認識だったのですが「ASI」という言葉を検索して記事など拝読しまして、必ずつけるようにしたほうがいいと認識いたしました。ご指摘ありがとうございました。

Answer 1

動くサンプル：https://jsfiddle.net/f9c2vw48/

---

特に問題が無く動いているようなので、他のコードがあってそれが邪魔しているのではないでしょうか。

Comments

[pecoro0321]

サンプルを作っていただきありがとうございます！！質問に書き出した部分は動いているのですね。。質問のためのコードの切り出し範囲に問題がありそうなので、コードを見直しつつ質問を編集するか検討したいと思います。

コードが動くかどうかの確認にどうサンプル化すればいいかわからなかったので勉強になりました。ありがとうございました！

[kei344]

（追記に対して）moredeepさんの回答にあるように、addEventListenerが複数回実行されています。
addEventListenerは同じイベントに複数の関数を割り当てられるため、開閉するたび同じ関数が複数回実行されるのでそのような問題が起きています。

表示を消すたびに関数の登録を解除するか、クリックイベントの中でクリックイベントを登録するような設計をやめましょう。

https://jsfiddle.net/rwj6dh91/

[pecoro0321]

早速のご対応ありがとうございます！！
クリックイベントの中にクリックイベントがあるのがおかしかったのですね。。。
深く理解できていない部分をなんとなく書いてしまっていることを痛感いたしました。
今後気をつけたいと思います。ありがとうございました。

※コメントでいただいているworksDetail()の関数化については、「works-graphic」というdivをもつモーダルが別でありまして、引数を加えてあとで使いまわそうかと思っていました。そのような場合に関数化するという考えは合っていますでしょうか…？

[kei344]

> そのような場合に関数化するという考えは合っていますでしょうか…？
現在提示されている関数/変数の構成では、関数化する必要がありませんね。そもそも複数のものが別々にあるのに[getElementsByClassName]で一気に取得するのでしょうか。
あとから読み込むデータがあり、読み込むたびにイベントをつける等の処理のために関数化することは設計として考えられますが、その場合要素の取得も含めて関数の中に入れることになると思います。

[pecoro0321]

ご返答ありがとうございます！
ご指摘を元に他の部分も実装を進めてみたいと思います。
大変お世話になりました。ありがとうございました。

Answer 2

addEventListenerが複数回実行されているだけではないでしょうか？

Comments

[pecoro0321]

回答ありがとうございます！ご指摘部分を意識しつつ周りのコードも読んでみようと思います。

Answer 3

こんな感じでは？

<style>
.work-web{display:none;}
.work-web.current{display:block;}
</style>
<script>
window.addEventListener('DOMContentLoaded', function(e){
  var w=document.querySelectorAll('.work-web');
  var l=w.length;
  ['#iconRight','#iconClose'].forEach((x,y)=>{
    document.querySelector(x).addEventListener('click',()=>{
      var c=[].indexOf.call(w,document.querySelector('.work-web.current'));
      var n=(y==1||c==(l-1))?0:c+1;
      w[c].classList.remove('current');
      w[n].classList.add('current');
    });
  });
});
</script>
<div id="worksDetailWeb">
 <div id="iconClose">close</div>
 <div id="iconRight">right</div>
 <div id="wrapper">
  <div class="work-web current" id="div01">1</div>
  <div class="work-web" id="div02">2</div>
  <div class="work-web" id="div03">3</div>
  <div class="work-web" id="div04">4</div>
  <div class="work-web" id="div05">5</div>
 </div>
</div>

Comments

[yambejp]

つまりforで回す意味がないということです

[pecoro0321]

ご回答ありがとうございます！初級者のため時間がかかってしまうのですが、いただいたコードを読んでまずは理解してみようと思います。

[pecoro0321]

拝読しました。
配列の中で特定の要素が何番目にあるかというのを
var index = Array.indexOf.call(配列, 特定の要素);
という形で取得できるのですね。。勉強になりました。ありがとうございました！