bootstrap icon indicating copy to clipboard operation
bootstrap copied to clipboard

Fix: add null check for activeIndicator in Carousel

Open PGrayCS opened this issue 4 months ago • 4 comments

Description

Adds null check for activeIndicator to match the pattern used for newActiveIndicator on line 282.

Motivation & Context

While testing carousel behavior with modified DOM, I noticed SelectorEngine.findOne() can return null if there's no active indicator, which would cause an error on classList.remove(). This adds the same safety check that's already in place for newActiveIndicator.

Type of changes

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Refactoring (non-breaking change)
  • [ ] Breaking change (fix or feature that would change existing functionality)

Checklist

  • [x] I have read the contributing guidelines
  • [x] My code follows the code style of the project (using npm run lint)
  • [ ] My change introduces changes to the documentation
  • [ ] I have updated the documentation accordingly
  • [ ] I have added tests to cover my changes
  • [x] All new and existing tests passed

Related issues

N/A

PGrayCS avatar Sep 30 '25 21:09 PGrayCS

This smells like AI.. Why would this be null in the first place? Also, most if not all JS changes need an accompanied test case.

XhmikosR avatar Oct 01 '25 06:10 XhmikosR

Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.

Happy to add a test case if you think this scenario is worth covering, or close this if you'd rather keep it as-is since it might be an edge case.

PGrayCS avatar Oct 03 '25 18:10 PGrayCS

Fair question. I ran into this when the carousel was initialized programmatically and the indicators element existed but had no active indicator yet. The inconsistency is that newActiveIndicator already has this check (line 282) but activeIndicator doesn't.

Could you please provide a reproducible use case so we can test it manually, and if possible, include the corresponding unit test in this PR?

julien-deramond avatar Oct 04 '25 07:10 julien-deramond

Sure thing. This happens when the carousel is initialized programmatically and the indicators element exists but has no active indicator yet. In that case, SelectorEngine.findOne(SELECTOR_ACTIVE, this._indicatorsElement) returns null, and then the code tries to call .classList.remove() on null which throws a TypeError.

I ran into this when I was building indicators dynamically and hadn't synced up the active states properly before the first slide transition.

I've added a test case in the latest commit that covers this scenario - creates a carousel with indicators but none marked as active, then calls next(). Verifies it slides correctly and activates the new indicator without erroring.

The null check matches what's already there for newActiveIndicator on line 282, so it keeps the code consistent.

PGrayCS avatar Oct 04 '25 16:10 PGrayCS