From 7cc78de7070b158f4fe0233deddf0468d81ab054 Mon Sep 17 00:00:00 2001 From: Guilherme Soares Date: Fri, 24 Jan 2025 14:27:47 +0000 Subject: [PATCH 1/3] fix: scroll to the correct element when smooth scroll is disabled --- js/src/scrollspy.js | 6 ++-- js/tests/unit/scrollspy.spec.js | 51 ++++++++++++++++++++++----------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/js/src/scrollspy.js b/js/src/scrollspy.js index 368092de45..c86c8b0555 100644 --- a/js/src/scrollspy.js +++ b/js/src/scrollspy.js @@ -125,9 +125,7 @@ class ScrollSpy extends BaseComponent { } _maybeEnableSmoothScroll() { - if (!this._config.smoothScroll) { - return - } + const scrollBehavior = this._config.smoothScroll ? 'smooth' : 'auto' // unregister any previous listeners EventHandler.off(this._config.target, EVENT_CLICK) @@ -139,7 +137,7 @@ class ScrollSpy extends BaseComponent { const root = this._rootElement || window const height = observableSection.offsetTop - this._element.offsetTop if (root.scrollTo) { - root.scrollTo({ top: height, behavior: 'smooth' }) + root.scrollTo({ top: height, behavior: scrollBehavior }) return } diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js index fc44471c42..e2af97221e 100644 --- a/js/tests/unit/scrollspy.spec.js +++ b/js/tests/unit/scrollspy.spec.js @@ -842,37 +842,54 @@ describe('ScrollSpy', () => { }) describe('SmoothScroll', () => { - it('should not enable smoothScroll', () => { + it('should not enable smoothScroll', done => { fixtureEl.innerHTML = getDummyFixture() - const offSpy = spyOn(EventHandler, 'off').and.callThrough() - const onSpy = spyOn(EventHandler, 'on').and.callThrough() const div = fixtureEl.querySelector('.content') - const target = fixtureEl.querySelector('#navBar') - // eslint-disable-next-line no-new - new ScrollSpy(div, { - offset: 1 + const link = fixtureEl.querySelector('[href="#div-jsm-1"]') + const observable = fixtureEl.querySelector('#div-jsm-1') + const clickSpy = getElementScrollSpy(div) + + const scrollSpy = new ScrollSpy(div, { + offset: 1, + smoothScroll: false }) - expect(offSpy).not.toHaveBeenCalledWith(target, 'click.bs.scrollspy') - expect(onSpy).not.toHaveBeenCalledWith(target, 'click.bs.scrollspy') + setTimeout(() => { + if (div.scrollTo) { + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'auto' }) + } else { + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + } + + done() + }, 100) + link.click() }) - it('should enable smoothScroll', () => { + it('should enable smoothScroll', done => { fixtureEl.innerHTML = getDummyFixture() - const offSpy = spyOn(EventHandler, 'off').and.callThrough() - const onSpy = spyOn(EventHandler, 'on').and.callThrough() const div = fixtureEl.querySelector('.content') - const target = fixtureEl.querySelector('#navBar') - // eslint-disable-next-line no-new - new ScrollSpy(div, { + const link = fixtureEl.querySelector('[href="#div-jsm-1"]') + const observable = fixtureEl.querySelector('#div-jsm-1') + const clickSpy = getElementScrollSpy(div) + + const scrollSpy = new ScrollSpy(div, { offset: 1, smoothScroll: true }) - expect(offSpy).toHaveBeenCalledWith(target, 'click.bs.scrollspy') - expect(onSpy).toHaveBeenCalledWith(target, 'click.bs.scrollspy', '[href]', jasmine.any(Function)) + setTimeout(() => { + if (div.scrollTo) { + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'smooth' }) + } else { + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + } + + done() + }, 100) + link.click() }) it('should not smoothScroll to element if it not handles a scrollspy section', () => { From 0b1556ffd5d26438324d1a10688e78668bdf841f Mon Sep 17 00:00:00 2001 From: Guilherme Soares Date: Fri, 24 Jan 2025 14:58:31 +0000 Subject: [PATCH 2/3] Scrollspy: improve active behaviour --- js/src/scrollspy.js | 53 ++++++++++----------------------- js/tests/unit/scrollspy.spec.js | 7 +++-- 2 files changed, 20 insertions(+), 40 deletions(-) diff --git a/js/src/scrollspy.js b/js/src/scrollspy.js index c86c8b0555..f4a8cb30b2 100644 --- a/js/src/scrollspy.js +++ b/js/src/scrollspy.js @@ -40,10 +40,10 @@ const SELECTOR_DROPDOWN_TOGGLE = '.dropdown-toggle' const Default = { offset: null, // TODO: v6 @deprecated, keep it for backwards compatibility reasons - rootMargin: '0px 0px -25%', + rootMargin: '0px', smoothScroll: false, target: null, - threshold: [0.1, 0.5, 1] + threshold: [0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1] } const DefaultType = { @@ -65,13 +65,10 @@ class ScrollSpy extends BaseComponent { // this._element is the observablesContainer and config.target the menu links wrapper this._targetLinks = new Map() this._observableSections = new Map() + this._intersectionRatio = new Map() this._rootElement = getComputedStyle(this._element).overflowY === 'visible' ? null : this._element this._activeTarget = null this._observer = null - this._previousScrollData = { - visibleEntryTop: 0, - parentScrollTop: 0 - } this.refresh() // initialize } @@ -115,7 +112,7 @@ class ScrollSpy extends BaseComponent { config.target = getElement(config.target) || document.body // TODO: v6 Only for backwards compatibility reasons. Use rootMargin only - config.rootMargin = config.offset ? `${config.offset}px 0px -30%` : config.rootMargin + config.rootMargin = config.offset ? `${config.offset}px 0px 0px` : config.rootMargin if (typeof config.threshold === 'string') { config.threshold = config.threshold.split(',').map(value => Number.parseFloat(value)) @@ -159,40 +156,21 @@ class ScrollSpy extends BaseComponent { // The logic of selection _observerCallback(entries) { - const targetElement = entry => this._targetLinks.get(`#${entry.target.id}`) - const activate = entry => { - this._previousScrollData.visibleEntryTop = entry.target.offsetTop - this._process(targetElement(entry)) + for (const entry of entries) { + this._intersectionRatio.set(`#${entry.target.id}`, entry.intersectionRatio) } - const parentScrollTop = (this._rootElement || document.documentElement).scrollTop - const userScrollsDown = parentScrollTop >= this._previousScrollData.parentScrollTop - this._previousScrollData.parentScrollTop = parentScrollTop - - for (const entry of entries) { - if (!entry.isIntersecting) { - this._activeTarget = null - this._clearActiveClass(targetElement(entry)) - - continue + let maxIntersectionRatio = 0 + let element = null + for (const [key, val] of this._intersectionRatio.entries()) { + if (val > maxIntersectionRatio) { + element = this._targetLinks.get(key) + maxIntersectionRatio = val } + } - const entryIsLowerThanPrevious = entry.target.offsetTop >= this._previousScrollData.visibleEntryTop - // if we are scrolling down, pick the bigger offsetTop - if (userScrollsDown && entryIsLowerThanPrevious) { - activate(entry) - // if parent isn't scrolled, let's keep the first visible item, breaking the iteration - if (!parentScrollTop) { - return - } - - continue - } - - // if we are scrolling up, pick the smallest offsetTop - if (!userScrollsDown && !entryIsLowerThanPrevious) { - activate(entry) - } + if (element !== null) { + this._process(element) } } @@ -214,6 +192,7 @@ class ScrollSpy extends BaseComponent { if (isVisible(observableSection)) { this._targetLinks.set(decodeURI(anchor.hash), anchor) this._observableSections.set(anchor.hash, observableSection) + this._intersectionRatio.set(anchor.hash, 0) } } } diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js index e2af97221e..7dc9f44cc8 100644 --- a/js/tests/unit/scrollspy.spec.js +++ b/js/tests/unit/scrollspy.spec.js @@ -451,7 +451,7 @@ describe('ScrollSpy', () => { }) }) - it('should clear selection if above the first section', () => { + it('should remember previous selection', () => { return new Promise(resolve => { fixtureEl.innerHTML = [ '', @@ -483,9 +483,10 @@ describe('ScrollSpy', () => { expect(spy).toHaveBeenCalled() expect(fixtureEl.querySelectorAll('.active')).toHaveSize(1) - expect(active().getAttribute('id')).toEqual('two-link') + expect(active().getAttribute('id')).toEqual('one-link') onScrollStop(() => { - expect(active()).toBeNull() + expect(fixtureEl.querySelectorAll('.active')).toHaveSize(1) + expect(active().getAttribute('id')).toEqual('one-link') resolve() }, contentEl) scrollTo(contentEl, 0) From cef7e253e4c64a4428de90d1a51d16f7dc98349b Mon Sep 17 00:00:00 2001 From: Guilherme Soares Date: Fri, 24 Jan 2025 15:36:58 +0000 Subject: [PATCH 3/3] fix: set element to active after a smooth scroll --- js/src/scrollspy.js | 6 +++++- js/tests/unit/scrollspy.spec.js | 24 ++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/js/src/scrollspy.js b/js/src/scrollspy.js index f4a8cb30b2..86879fda27 100644 --- a/js/src/scrollspy.js +++ b/js/src/scrollspy.js @@ -69,6 +69,9 @@ class ScrollSpy extends BaseComponent { this._rootElement = getComputedStyle(this._element).overflowY === 'visible' ? null : this._element this._activeTarget = null this._observer = null + // Sometimes scrolling to the section isn't enough to trigger a observation callback. + // Scroll to up to 2px to make sure it triggers. + this._scrollOffset = 2 this.refresh() // initialize } @@ -132,7 +135,8 @@ class ScrollSpy extends BaseComponent { if (observableSection) { event.preventDefault() const root = this._rootElement || window - const height = observableSection.offsetTop - this._element.offsetTop + + const height = observableSection.offsetTop - this._element.offsetTop - this._scrollOffset if (root.scrollTo) { root.scrollTo({ top: height, behavior: scrollBehavior }) return diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js index 7dc9f44cc8..7947c0ef6f 100644 --- a/js/tests/unit/scrollspy.spec.js +++ b/js/tests/unit/scrollspy.spec.js @@ -858,9 +858,9 @@ describe('ScrollSpy', () => { setTimeout(() => { if (div.scrollTo) { - expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'auto' }) + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset, behavior: 'auto' }) } else { - expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset) } done() @@ -883,9 +883,9 @@ describe('ScrollSpy', () => { setTimeout(() => { if (div.scrollTo) { - expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'smooth' }) + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset, behavior: 'smooth' }) } else { - expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset) } done() @@ -943,17 +943,17 @@ describe('ScrollSpy', () => { const link = fixtureEl.querySelector('[href="#div-jsm-1"]') const observable = fixtureEl.querySelector('#div-jsm-1') const clickSpy = getElementScrollSpy(div) - // eslint-disable-next-line no-new - new ScrollSpy(div, { + + const scrollSpy = new ScrollSpy(div, { offset: 1, smoothScroll: true }) setTimeout(() => { if (div.scrollTo) { - expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'smooth' }) + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset, behavior: 'smooth' }) } else { - expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset) } done() @@ -977,17 +977,17 @@ describe('ScrollSpy', () => { const link = fixtureEl.querySelector('[href="#présentation"]') const observable = fixtureEl.querySelector('#présentation') const clickSpy = getElementScrollSpy(div) - // eslint-disable-next-line no-new - new ScrollSpy(div, { + + const scrollSpy = new ScrollSpy(div, { offset: 1, smoothScroll: true }) setTimeout(() => { if (div.scrollTo) { - expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop, behavior: 'smooth' }) + expect(clickSpy).toHaveBeenCalledWith({ top: observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset, behavior: 'smooth' }) } else { - expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop) + expect(clickSpy).toHaveBeenCalledWith(observable.offsetTop - div.offsetTop - scrollSpy._scrollOffset) } done()