bootstrap icon indicating copy to clipboard operation
bootstrap copied to clipboard

Suggest a new Scrollspy fix for version 6.

Open DenisLopatin opened this issue 2 months ago • 5 comments

Prerequisites

Proposal

The Scrollspy component was broken when switching to the observer api. I decided to look into it. Basically, I understood why the tests were passed, which tests did not work as they should, what exactly broke, and so on. I started fixing the component while getting rid of the old functionality, rewriting, adding and correcting the tests. I've managed to make some progress with the component. But I have a question, are you interested in this and what are your plans for it? Can I continue working on it and send a PR, what do you say?

Motivation and context

The component has been in a broken state for a long time. I'd like to fix it.

Here's what's ready at the moment. I just have to fix a couple of bugs and add scroll-offset-top for the header, getting rid of the offset parameter. It can even be made dynamic by passing an overlapping absolute positioning element in the configuration and adding styles. There's a lot to think about.

https://github.com/user-attachments/assets/ada510af-e987-4fb4-889a-89d5137c4f08

https://github.com/user-attachments/assets/42dba1ec-6278-4762-a7a9-081488f2170a

https://github.com/user-attachments/assets/96ecdd51-9c07-4207-bb6a-8bb8e82218e7

For now, I'll just leave the code of the new component:

/**
 * --------------------------------------------------------------------------
 * Bootstrap scrollspy.js
 * Licensed under MIT (https://github.com/twbs/bootstrap/blob/main/LICENSE)
 * --------------------------------------------------------------------------
 */

import BaseComponent from './base-component.js'
import EventHandler from './dom/event-handler.js'
import SelectorEngine from './dom/selector-engine.js'
import {
  getElement, isDisabled, isVisible
} from './util/index.js'

/**
 * Constants
 */

const NAME = 'scrollspy'
const DATA_KEY = 'bs.scrollspy'
const EVENT_KEY = `.${DATA_KEY}`
const DATA_API_KEY = '.data-api'

const EVENT_ACTIVATE = `activate${EVENT_KEY}`
const EVENT_LOAD_DATA_API = `load${EVENT_KEY}${DATA_API_KEY}`

const CLASS_NAME_DROPDOWN_ITEM = 'dropdown-item'
const CLASS_NAME_ACTIVE = 'active'

const SELECTOR_DATA_SPY = '[data-bs-spy="scroll"]'
const SELECTOR_TARGET_LINKS = '[href]'
const SELECTOR_NAV_LIST_GROUP = '.nav, .list-group'
const SELECTOR_NAV_LINKS = '.nav-link'
const SELECTOR_NAV_ITEMS = '.nav-item'
const SELECTOR_LIST_ITEMS = '.list-group-item'
const SELECTOR_LINK_ITEMS = `${SELECTOR_NAV_LINKS}, ${SELECTOR_NAV_ITEMS} > ${SELECTOR_NAV_LINKS}, ${SELECTOR_LIST_ITEMS}`
const SELECTOR_DROPDOWN = '.dropdown'
const SELECTOR_DROPDOWN_TOGGLE = '.dropdown-toggle'

export const SPY_ENGINE_CONFIG = {
  rootMargin: '-2% 0px -98% 0px',
  threshold: [0]
}

export const SPY_SENTRY_CONFIG = {
  rootMargin: '0px 0px 0px 0px',
  threshold: [0]
}

const Default = {
  rootMargin: SPY_ENGINE_CONFIG.rootMargin,
  threshold: SPY_ENGINE_CONFIG.threshold,
  smoothScroll: false,
  target: null
}

const DefaultType = {
  rootMargin: 'string',
  threshold: 'array',
  smoothScroll: 'boolean',
  target: 'element'
}

/**
 * Class definition
 */

class ScrollSpy extends BaseComponent {
  constructor(element, config) {
    super(element, config)

    // this._element is the observablesContainer and config.target the menu links wrapper
    this._targetLinks = new Map()
    this._observableSections = new Map()
    this._rootElement = getComputedStyle(this._element).overflowY === 'visible' ? null : this._element
    this._activeTarget = null
    this._observer = null
    this._sentryObserver = null
    this._sentryObserverElement = null

    this.refresh() // initialize
  }

  // Getters
  static get Default() {
    return Default
  }

  static get DefaultType() {
    return DefaultType
  }

  static get NAME() {
    return NAME
  }

  // Public
  refresh() {
    this._initializeTargets()
    this._captureTargets()
    this._customizeScrollBehavior()
    this._activateAnchor()

    this._observer?.disconnect()
    this._observer = this._getNewObserver()

    this._sentryObserver?.disconnect()
    this._sentryObserver = this._getNewSentryObserver()

    for (const section of this._observableSections.values()) {
      this._observer.observe(section)
    }

    this._sentryObserver.observe(this._sentryObserverElement)
  }

  dispose() {
    this._observer.disconnect()
    super.dispose()
  }

  // Private
  _configAfterMerge(config) {
    config.target = getElement(config.target)

    if (!config.target) {
      throw new TypeError('Bootstrap ScrollSpy: You must specify a valid "target" element')
    }

    return config
  }

  _initializeTargets() {
    this._targetLinks = new Map()
    this._observableSections = new Map()
    this._sentryObserverElement = SelectorEngine.findOne('.sentry-observer', this._element)

    if (!this._sentryObserverElement) {
      const sentryObserverElement = document.createElement('div')

      sentryObserverElement.classList.add('sentry-observer', 'visibility-hidden')
      sentryObserverElement.style.height = '1px'
      sentryObserverElement.style.width = '1px'
      this._element.append(sentryObserverElement)
    }
  }

  _captureTargets() {
    const targetLinks = SelectorEngine.find(SELECTOR_TARGET_LINKS, this._config.target)

    for (const anchor of targetLinks) {
      // ensure that the anchor has an id and is not disabled
      if (!anchor.hash || isDisabled(anchor)) {
        continue
      }

      const observableSection = SelectorEngine.findOne(decodeURI(anchor.hash), this._element)

      // ensure that the observableSection exists & is visible
      if (isVisible(observableSection)) {
        this._targetLinks.set(decodeURI(anchor.hash), anchor)
        this._observableSections.set(anchor.hash, observableSection)
      }
    }

    this._sentryObserverElement = SelectorEngine.findOne('.sentry-observer', this._element)
  }

  _customizeScrollBehavior() {
    if (this._rootElement && this._config.smoothScroll) {
      this._rootElement.classList.add('scroll-smooth')
    }

    if (this._rootElement && !this._config.smoothScroll) {
      this._rootElement.classList.remove('scroll-smooth')
    }
  }

  _activateAnchor() {
    const [, firstObservableSection] = [...this._observableSections].shift()
    const [, firstAnchorElement] = [...this._targetLinks].shift()

    const rect = firstObservableSection.getBoundingClientRect()

    if (rect.top > 0) {
      this._setActiveClass(firstAnchorElement)
    }
  }

  _getNewSentryObserver() {
    const options = {
      root: this._rootElement,
      threshold: SPY_SENTRY_CONFIG.threshold,
      rootMargin: SPY_SENTRY_CONFIG.rootMargin
    }

    return new IntersectionObserver(entries => this._sentryObserverCallback(entries), options)
  }

  _sentryObserverCallback(entries) {
    const visibleEntry = entries.find(entry => entry.isIntersecting)

    if (!visibleEntry) {
      return
    }

    const targets = [...this._targetLinks]
    const [, lastAnchorElement] = targets.pop()

    if (!lastAnchorElement.classList.contains(CLASS_NAME_ACTIVE)) {
      for (const [, element] of targets) {
        this._clearActiveClass(element)
      }

      this._setActiveClass(lastAnchorElement)
    }
  }

  _getNewObserver() {
    const options = {
      root: this._rootElement,
      threshold: this._config.threshold,
      rootMargin: this._config.rootMargin
    }

    return new IntersectionObserver(entries => this._observerCallback(entries), options)
  }

  _observerCallback(entries) {
    const visibleEntry = entries.find(entry => entry.isIntersecting)

    if (!visibleEntry) {
      return
    }

    const element = this._targetLinks.get(`#${visibleEntry.target.id}`)

    this._process(element)
  }

  _process(target) {
    if (this._activeTarget === target) {
      return
    }

    this._clearActiveClass(this._config.target)
    this._setActiveClass(target)

    this._activateDropdownParentElement(target)
    this._activateListGroupParentElement(target)

    EventHandler.trigger(this._element, EVENT_ACTIVATE, { relatedTarget: target })
  }

  _clearActiveClass(parent) {
    parent.classList.remove(CLASS_NAME_ACTIVE)

    const activeNodes = SelectorEngine.find(`${SELECTOR_TARGET_LINKS}.${CLASS_NAME_ACTIVE}`, parent)

    for (const node of activeNodes) {
      node.classList.remove(CLASS_NAME_ACTIVE)
    }
  }

  _setActiveClass(target) {
    this._activeTarget = target
    target.classList.add(CLASS_NAME_ACTIVE)
  }

  _activateDropdownParentElement(target) {
    // Set the parent active dropdown class if dropdown is the target of the clicked link or the current viewport
    if (target.classList.contains(CLASS_NAME_DROPDOWN_ITEM)) {
      SelectorEngine.findOne(SELECTOR_DROPDOWN_TOGGLE, target.closest(SELECTOR_DROPDOWN))
        .classList.add(CLASS_NAME_ACTIVE)
    }
  }

  _activateListGroupParentElement(target) {
    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_LINK_ITEMS)) {
        item.classList.add(CLASS_NAME_ACTIVE)
      }
    }
  }
}

/**
 * Data API implementation
 */

EventHandler.on(window, EVENT_LOAD_DATA_API, () => {
  for (const spy of SelectorEngine.find(SELECTOR_DATA_SPY)) {
    ScrollSpy.getOrCreateInstance(spy)
  }
})

export default ScrollSpy

DenisLopatin avatar Dec 02 '25 22:12 DenisLopatin

I passed all the old tests on this code (or this one is a little newer, maybe something won't work already). The bottom line is that the old tests were unreliable. So I decided to change some of them and write new ones.

DenisLopatin avatar Dec 02 '25 22:12 DenisLopatin

If you leave the observer API and don't catch thousands of bugs, you'll have to do two things. First, you will need to wrap the IDs

old

<h4 id="list-item-1">Item 1 (Intro)</h4>
  <p>
    Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
  </p>

new

<div id="list-item-1">
  <h4>Item 1 (Intro)</h4>
    <p>
      Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
    </p>
</div>

Secondly, you will have to set it strictly at the top and even remove from the user the ability to change the low-level details of the API of the observer, such as rootMargin and threshold.

DenisLopatin avatar Dec 02 '25 22:12 DenisLopatin

You can give the user the opportunity to change the rootMargin, as there may be problems with the header, I have not tested this point yet.

DenisLopatin avatar Dec 02 '25 23:12 DenisLopatin

Hello @DenisLopatin and thank you for this code ; don't you need to make a PR to gain clarity on the purpose of your code?

CyrilKrylatov avatar Dec 03 '25 08:12 CyrilKrylatov

Hi @CyrilKrylatov. There will be the same code that I have attached, if we are talking about a component. As for the PR, I don't want to send it until I've passed all the tests and covered all the cases. I'm going to need time for this.

DenisLopatin avatar Dec 03 '25 12:12 DenisLopatin