From c538d0c1b8a3380148030403127d9c5e4f049ec9 Mon Sep 17 00:00:00 2001
From: GeoSot <geo.sotis@gmail.com>
Date: Thu, 9 Sep 2021 02:52:05 +0300
Subject: Revert Navs activation use `for ... of`

---
 .bundlewatch.config.json        |  4 +-
 js/src/scrollspy.js             | 56 ++++++++++++++++----------
 js/tests/unit/scrollspy.spec.js | 87 +++++++++++++++++++----------------------
 3 files changed, 79 insertions(+), 68 deletions(-)

diff --git a/.bundlewatch.config.json b/.bundlewatch.config.json
index ded4e7dc3..19f813bc4 100644
--- a/.bundlewatch.config.json
+++ b/.bundlewatch.config.json
@@ -38,7 +38,7 @@
     },
     {
       "path": "./dist/js/bootstrap.bundle.min.js",
-      "maxSize": "22 kB"
+      "maxSize": "22.25 kB"
     },
     {
       "path": "./dist/js/bootstrap.esm.js",
@@ -46,7 +46,7 @@
     },
     {
       "path": "./dist/js/bootstrap.esm.min.js",
-      "maxSize": "18 kB"
+      "maxSize": "18.25 kB"
     },
     {
       "path": "./dist/js/bootstrap.js",
diff --git a/js/src/scrollspy.js b/js/src/scrollspy.js
index 9809c28cd..173557622 100644
--- a/js/src/scrollspy.js
+++ b/js/src/scrollspy.js
@@ -43,7 +43,7 @@ const CLASS_NAME_ACTIVE = 'active'
 const SELECTOR_DATA_SPY = '[data-bs-spy="scroll"]'
 const SELECTOR_NAV_LIST_GROUP = '.nav, .list-group'
 const SELECTOR_NAV_LINKS = '.nav-link'
-// const SELECTOR_NAV_ITEMS = '.nav-item'
+const SELECTOR_NAV_ITEMS = '.nav-item'
 const SELECTOR_LIST_ITEMS = '.list-group-item'
 const SELECTOR_DROPDOWN = '.dropdown'
 const SELECTOR_DROPDOWN_TOGGLE = '.dropdown-toggle'
@@ -95,7 +95,9 @@ class ScrollSpy extends BaseComponent {
       this._observer = this._getNewObserver()
     }
 
-    this._observableSections.forEach(section => this._observer.observe(section))
+    for (const section of this._observableSections) {
+      this._observer.observe(section)
+    }
   }
 
   dispose() {
@@ -136,13 +138,21 @@ class ScrollSpy extends BaseComponent {
     if (target.classList.contains(CLASS_NAME_DROPDOWN_ITEM)) { // Activate dropdown parents
       SelectorEngine.findOne(SELECTOR_DROPDOWN_TOGGLE, target.closest(SELECTOR_DROPDOWN))
         .classList.add(CLASS_NAME_ACTIVE)
-    }
+    } else {
+      for (const listGroup of SelectorEngine.parents(target, SELECTOR_NAV_LIST_GROUP)) {
+        // Set triggered links parents as active
+        // With both <ul> and <nav> markup a parent is the previous sibling of any nav ancestor
+        for (const item of SelectorEngine.prev(listGroup, `${SELECTOR_NAV_LINKS}, ${SELECTOR_LIST_ITEMS}`)) {
+          item.classList.add(CLASS_NAME_ACTIVE)
+        }
 
-    for (const listGroup of SelectorEngine.parents(target, SELECTOR_NAV_LIST_GROUP)) {
-      // Set triggered links parents as active
-      // With both <ul> and <nav> markup a parent is the previous sibling of any nav ancestor
-      SelectorEngine.prev(listGroup, `${SELECTOR_NAV_LINKS}, ${SELECTOR_LIST_ITEMS}`)
-        .forEach(item => item.classList.add(CLASS_NAME_ACTIVE))
+        // Handle special case when .nav-link is inside .nav-item
+        for (const navItem of SelectorEngine.prev(listGroup, SELECTOR_NAV_ITEMS)) {
+          for (const item of SelectorEngine.children(navItem, SELECTOR_NAV_LINKS)) {
+            item.classList.add(CLASS_NAME_ACTIVE)
+          }
+        }
+      }
     }
 
     EventHandler.trigger(this._element, EVENT_ACTIVATE, {
@@ -155,42 +165,47 @@ class ScrollSpy extends BaseComponent {
       parent.classList.remove(CLASS_NAME_ACTIVE)
     }
 
-    SelectorEngine.find(`.${CLASS_NAME_ACTIVE}`, parent)
-      .forEach(node => node.classList.remove(CLASS_NAME_ACTIVE))
+    for (const node of SelectorEngine.find(`.${CLASS_NAME_ACTIVE}`, parent)) {
+      node.classList.remove(CLASS_NAME_ACTIVE)
+    }
   }
 
   _getNewObserver() {
     let previousVisibleEntryTop = 0
     let previousParentScrollTop = 0
 
+    const getTargetLink = entry => this._targetLinks.find(el => el.hash === `#${entry.target.id}`)
+
     const activate = entry => {
       previousVisibleEntryTop = entry.target.offsetTop
-      const targetToActivate = this._targetLinks.find(el => el.hash === `#${entry.target.id}`)
+      const targetToActivate = getTargetLink(entry)
       this._process(targetToActivate)
     }
 
     const callback = entries => {
       const parentScrollTop = this._element.scrollTop
-      entries.forEach(entry => {
-        if (entry.isIntersecting) {
+      let previousIntersectionRatio = 0
+      for (const entry of entries) {
+        if (entry.isIntersecting && previousIntersectionRatio < entry.intersectionRatio) {
           const { offsetTop } = entry.target
+          previousIntersectionRatio = entry.intersectionRatio
           const userScrollsDown = parentScrollTop >= previousParentScrollTop
 
           if (userScrollsDown && offsetTop >= previousVisibleEntryTop) { // if we are scrolling down, pick the bigger offsetTop
             activate(entry)
-            return
+            continue
           }
 
-          if (!userScrollsDown && offsetTop < previousVisibleEntryTop) {// if we are scrolling up, pick the smallest offsetTop
+          if (!userScrollsDown && offsetTop < previousVisibleEntryTop) { // if we are scrolling up, pick the smallest offsetTop
             activate(entry)
           }
 
-          return
+          continue
         }
 
-        const notVisibleElement = this._targetLinks.find(el => el.hash === `#${entry.target.id}`)
+        const notVisibleElement = getTargetLink(entry)
         this._clearActiveClass(notVisibleElement)
-      })
+      }
 
       previousParentScrollTop = this._element.scrollTop
     }
@@ -236,8 +251,9 @@ class ScrollSpy extends BaseComponent {
  */
 
 EventHandler.on(window, EVENT_LOAD_DATA_API, () => {
-  SelectorEngine.find(SELECTOR_DATA_SPY)
-    .forEach(spy => new ScrollSpy(spy))
+  for (const spy of SelectorEngine.find(SELECTOR_DATA_SPY)) {
+    new ScrollSpy(spy) // eslint-disable-line no-new
+  }
 })
 
 /**
diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js
index 248639e17..3412efaa1 100644
--- a/js/tests/unit/scrollspy.spec.js
+++ b/js/tests/unit/scrollspy.spec.js
@@ -7,6 +7,11 @@ import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fi
 describe('ScrollSpy', () => {
   let fixtureEl
 
+  const scrollTo = (el, height) => {
+    // el.scrollTo({ top: height })
+    el.scrollTop = height
+  }
+
   const onScrollStop = (callback, element, timeout = 30) => {
     let handle = null
     const onScroll = function () {
@@ -36,25 +41,29 @@ describe('ScrollSpy', () => {
     ].join('')
   }
 
-  const testElementIsActiveAfterScroll = ({ elementSelector, targetSelector, contentEl, scrollSpy, spy, cb }) => {
+  const testElementIsActiveAfterScroll = ({ elementSelector, targetSelector, contentEl, scrollSpy, cb }) => {
     const element = fixtureEl.querySelector(elementSelector)
     const target = fixtureEl.querySelector(targetSelector)
     // add top padding to fix Chrome on Android failures
-    const paddingTop = 5
+    const paddingTop = 0
     const parentOffset = getComputedStyle(contentEl).getPropertyValue('position') === 'relative' ? 0 : contentEl.offsetTop
     const scrollHeight = (target.offsetTop - parentOffset) + paddingTop
 
-    function listener() {
+    contentEl.addEventListener('activate.bs.scrollspy', event => {
+
+      if (scrollSpy._activeTarget !== element) {
+        return
+      }
+
       expect(element.classList.contains('active')).toEqual(true)
       expect(scrollSpy._activeTarget).toEqual(element)
-      spy.calls.reset()
+      expect(event.relatedTarget).toEqual(element)
       cb()
-    }
+    })
 
     setTimeout(() => { // in case we scroll something before the test
-      onScrollStop(listener, contentEl)
-      contentEl.scrollTo({ top: scrollHeight })
-    }, 50)
+      scrollTo(contentEl, scrollHeight)
+    }, 100)
   }
 
   beforeAll(() => {
@@ -151,7 +160,7 @@ describe('ScrollSpy', () => {
         done()
       }, scrollSpyEl)
 
-      scrollSpyEl.scrollTo({ top: 350 })
+      scrollTo(scrollSpyEl, 350)
     })
 
     it('should only switch "active" class on current target specified w element', done => {
@@ -189,7 +198,7 @@ describe('ScrollSpy', () => {
         done()
       }, scrollSpyEl)
 
-      scrollSpyEl.scrollTo({ top: 350 })
+      scrollTo(scrollSpyEl, 350)
     })
 
     it('should correctly select middle navigation option when large offset is used', done => {
@@ -225,7 +234,7 @@ describe('ScrollSpy', () => {
         done()
       }, contentEl)
 
-      contentEl.scrollTo({ top: 550 })
+      scrollTo(contentEl, 550)
     })
 
     it('should add the active class to the correct element', done => {
@@ -247,22 +256,19 @@ describe('ScrollSpy', () => {
         offset: 0,
         target: '.navbar'
       })
-      const spy = spyOn(scrollSpy, '_process').and.callThrough()
 
       testElementIsActiveAfterScroll({
         elementSelector: '#a-1',
         targetSelector: '#div-1',
         contentEl,
         scrollSpy,
-        spy,
         cb: () => {
           testElementIsActiveAfterScroll({
             elementSelector: '#a-2',
             targetSelector: '#div-2',
             contentEl,
             scrollSpy,
-            spy,
-            cb: () => done()
+            cb: done
           })
         }
       })
@@ -287,22 +293,19 @@ describe('ScrollSpy', () => {
         offset: 0,
         target: '.navbar'
       })
-      const spy = spyOn(scrollSpy, '_process').and.callThrough()
 
       testElementIsActiveAfterScroll({
         elementSelector: '#a-1',
         targetSelector: '#div-1',
         contentEl,
         scrollSpy,
-        spy,
         cb: () => {
           testElementIsActiveAfterScroll({
             elementSelector: '#a-2',
             targetSelector: '#div-2',
             contentEl,
             scrollSpy,
-            spy,
-            cb: () => done()
+            cb: done
           })
         }
       })
@@ -327,22 +330,19 @@ describe('ScrollSpy', () => {
         offset: 0,
         target: '.navbar'
       })
-      const spy = spyOn(scrollSpy, '_process').and.callThrough()
 
       testElementIsActiveAfterScroll({
         elementSelector: '#a-1',
         targetSelector: '#div-1',
         contentEl,
         scrollSpy,
-        spy,
         cb: () => {
           testElementIsActiveAfterScroll({
             elementSelector: '#a-2',
             targetSelector: '#div-2',
             contentEl,
             scrollSpy,
-            spy,
-            cb: () => done()
+            cb: done
           })
         }
       })
@@ -382,10 +382,10 @@ describe('ScrollSpy', () => {
           expect(active()).toBeNull()
           done()
         }, contentEl)
-        contentEl.scrollTo({ top: 0 })
+        scrollTo(contentEl, 0)
       }, contentEl)
 
-      contentEl.scrollTo({ top: 201 })
+      scrollTo(contentEl, 201)
     })
 
     it('should not clear selection if above the first section and first section is at the top', done => {
@@ -428,10 +428,10 @@ describe('ScrollSpy', () => {
           done()
         }, contentEl)
 
-        contentEl.scrollTo({ top: 0 })
+        scrollTo(contentEl, 0)
       }, contentEl)
 
-      contentEl.scrollTo({ top: startOfSectionTwo })
+      scrollTo(contentEl, startOfSectionTwo)
     })
 
     it('should correctly select navigation element on backward scrolling when each target section height is 100%', done => {
@@ -459,46 +459,41 @@ describe('ScrollSpy', () => {
         offset: 0,
         target: '.navbar'
       })
-      const spy = spyOn(scrollSpy, '_process').and.callThrough()
 
+      scrollTo(contentEl, 0)
       testElementIsActiveAfterScroll({
         elementSelector: '#li-100-5',
         targetSelector: '#div-100-5',
-        scrollSpy,
-        spy,
         contentEl,
-        cb() {
-          contentEl.scrollTo({ top: 0 })
+        scrollSpy,
+        cb: () => {
+          scrollTo(contentEl, 0)
           testElementIsActiveAfterScroll({
             elementSelector: '#li-100-4',
             targetSelector: '#div-100-4',
-            scrollSpy,
-            spy,
             contentEl,
-            cb() {
-              contentEl.scrollTo({ top: 0 })
+            scrollSpy,
+            cb: () => {
+              scrollTo(contentEl, 0)
               testElementIsActiveAfterScroll({
                 elementSelector: '#li-100-3',
                 targetSelector: '#div-100-3',
-                scrollSpy,
-                spy,
                 contentEl,
-                cb() {
-                  contentEl.scrollTo({ top: 0 })
+                scrollSpy,
+                cb: () => {
+                  scrollTo(contentEl, 0)
                   testElementIsActiveAfterScroll({
                     elementSelector: '#li-100-2',
                     targetSelector: '#div-100-2',
-                    scrollSpy,
-                    spy,
                     contentEl,
-                    cb() {
-                      contentEl.scrollTo({ top: 0 })
+                    scrollSpy,
+                    cb: () => {
+                      scrollTo(contentEl, 0)
                       testElementIsActiveAfterScroll({
                         elementSelector: '#li-100-1',
                         targetSelector: '#div-100-1',
-                        scrollSpy,
-                        spy,
                         contentEl,
+                        scrollSpy,
                         cb: done
                       })
                     }
-- 
cgit v1.2.3