Fix event handler removal in dropdown/carousel dispose (#33000)

* Fix event handler removal in carousel dispose * Fix event handler removal in dropdown dispose * Test event handlers in scrollspy dispose * Test event handlers in toast dispose * Test event handlers in tooltip dispose Co-authored-by: XhmikosR <xhmikosr@gmail.com> Co-authored-by: Rohit Sharma <rohit2sharma95@gmail.com>
twbs · Feb 12, 2021 · 02dbd87 · 02dbd87
1 parent 0a9d392
commit 02dbd87
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 9 deletions.
diff --git a/.bundlewatch.config.json b/.bundlewatch.config.json
@@ -38,7 +38,7 @@
     },
     {
       "path": "./dist/js/bootstrap.bundle.min.js",
-      "maxSize": "21.5 kB"
+      "maxSize": "21.75 kB"
     },
     {
       "path": "./dist/js/bootstrap.esm.js",
@@ -54,7 +54,7 @@
     },
     {
       "path": "./dist/js/bootstrap.min.js",
-      "maxSize": "15.5 kB"
+      "maxSize": "15.75 kB"
     }
   ],
   "ci": {

diff --git a/js/src/carousel.js b/js/src/carousel.js
@@ -216,7 +216,6 @@ class Carousel extends BaseComponent {
   }
 
   dispose() {
-    super.dispose()
     EventHandler.off(this._element, EVENT_KEY)
 
     this._items = null
@@ -226,6 +225,8 @@ class Carousel extends BaseComponent {
     this._isSliding = null
     this._activeElement = null
     this._indicatorsElement = null
+
+    super.dispose()
   }
 
   // Private

diff --git a/js/src/dropdown.js b/js/src/dropdown.js
@@ -232,14 +232,15 @@ class Dropdown extends BaseComponent {
   }
 
   dispose() {
-    super.dispose()
     EventHandler.off(this._element, EVENT_KEY)
     this._menu = null
 
     if (this._popper) {
       this._popper.destroy()
       this._popper = null
     }
+
+    super.dispose()
   }
 
   update() {

diff --git a/js/tests/unit/carousel.spec.js b/js/tests/unit/carousel.spec.js
@@ -295,6 +295,8 @@ describe('Carousel', () => {
 
       spyOn(Carousel.prototype, '_addTouchEventListeners')
 
+      // Headless browser does not support touch events, so need to fake it
+      // to test that touch events are add properly.
       document.documentElement.ontouchstart = () => {}
       const carousel = new Carousel(carouselEl)
 
@@ -1056,13 +1058,30 @@ describe('Carousel', () => {
       ].join('')
 
       const carouselEl = fixtureEl.querySelector('#myCarousel')
+      const addEventSpy = spyOn(carouselEl, 'addEventListener').and.callThrough()
+      const removeEventSpy = spyOn(carouselEl, 'removeEventListener').and.callThrough()
+
+      // Headless browser does not support touch events, so need to fake it
+      // to test that touch events are add/removed properly.
+      document.documentElement.ontouchstart = () => {}
+
       const carousel = new Carousel(carouselEl)
 
-      spyOn(EventHandler, 'off').and.callThrough()
+      const expectedArgs = [
+        ['keydown', jasmine.any(Function), jasmine.any(Boolean)],
+        ['mouseover', jasmine.any(Function), jasmine.any(Boolean)],
+        ['mouseout', jasmine.any(Function), jasmine.any(Boolean)],
+        ['pointerdown', jasmine.any(Function), jasmine.any(Boolean)],
+        ['pointerup', jasmine.any(Function), jasmine.any(Boolean)]
+      ]
+
+      expect(addEventSpy.calls.allArgs()).toEqual(expectedArgs)
 
       carousel.dispose()
 
-      expect(EventHandler.off).toHaveBeenCalled()
+      expect(removeEventSpy.calls.allArgs()).toEqual(expectedArgs)
+
+      delete document.documentElement.ontouchstart
     })
   })
 

diff --git a/js/tests/unit/dropdown.spec.js b/js/tests/unit/dropdown.spec.js
@@ -884,16 +884,21 @@ describe('Dropdown', () => {
       ].join('')
 
       const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
+      spyOn(btnDropdown, 'addEventListener').and.callThrough()
+      spyOn(btnDropdown, 'removeEventListener').and.callThrough()
+
       const dropdown = new Dropdown(btnDropdown)
 
       expect(dropdown._popper).toBeNull()
       expect(dropdown._menu).toBeDefined()
       expect(dropdown._element).toBeDefined()
+      expect(btnDropdown.addEventListener).toHaveBeenCalledWith('click', jasmine.any(Function), jasmine.any(Boolean))
 
       dropdown.dispose()
 
       expect(dropdown._menu).toBeNull()
       expect(dropdown._element).toBeNull()
+      expect(btnDropdown.removeEventListener).toHaveBeenCalledWith('click', jasmine.any(Function), jasmine.any(Boolean))
     })
 
     it('should dispose dropdown with Popper', () => {

diff --git a/js/tests/unit/scrollspy.spec.js b/js/tests/unit/scrollspy.spec.js
@@ -1,6 +1,5 @@
 import ScrollSpy from '../../src/scrollspy'
 import Manipulator from '../../src/dom/manipulator'
-import EventHandler from '../../src/dom/event-handler'
 
 /** Test helpers */
 import { getFixture, clearFixture, createEvent, jQueryMock } from '../helpers/fixture'
@@ -560,14 +559,18 @@ describe('ScrollSpy', () => {
 
   describe('dispose', () => {
     it('should dispose a scrollspy', () => {
-      spyOn(EventHandler, 'off')
       fixtureEl.innerHTML = '<div style="display: none;"></div>'
 
       const divEl = fixtureEl.querySelector('div')
+      spyOn(divEl, 'addEventListener').and.callThrough()
+      spyOn(divEl, 'removeEventListener').and.callThrough()
+
       const scrollSpy = new ScrollSpy(divEl)
+      expect(divEl.addEventListener).toHaveBeenCalledWith('scroll', jasmine.any(Function), jasmine.any(Boolean))
 
       scrollSpy.dispose()
-      expect(EventHandler.off).toHaveBeenCalledWith(divEl, '.bs.scrollspy')
+
+      expect(divEl.removeEventListener).toHaveBeenCalledWith('scroll', jasmine.any(Function), jasmine.any(Boolean))
     })
   })
 

diff --git a/js/tests/unit/toast.spec.js b/js/tests/unit/toast.spec.js
@@ -274,13 +274,18 @@ describe('Toast', () => {
       fixtureEl.innerHTML = '<div></div>'
 
       const toastEl = fixtureEl.querySelector('div')
+      spyOn(toastEl, 'addEventListener').and.callThrough()
+      spyOn(toastEl, 'removeEventListener').and.callThrough()
+
       const toast = new Toast(toastEl)
 
       expect(Toast.getInstance(toastEl)).toBeDefined()
+      expect(toastEl.addEventListener).toHaveBeenCalledWith('click', jasmine.any(Function), jasmine.any(Boolean))
 
       toast.dispose()
 
       expect(Toast.getInstance(toastEl)).toBeNull()
+      expect(toastEl.removeEventListener).toHaveBeenCalledWith('click', jasmine.any(Function), jasmine.any(Boolean))
     })
 
     it('should allow to destroy toast and hide it before that', done => {

diff --git a/js/tests/unit/tooltip.spec.js b/js/tests/unit/tooltip.spec.js
@@ -327,13 +327,26 @@ describe('Tooltip', () => {
       fixtureEl.innerHTML = '<a href="#" rel="tooltip" title="Another tooltip">'
 
       const tooltipEl = fixtureEl.querySelector('a')
+      const addEventSpy = spyOn(tooltipEl, 'addEventListener').and.callThrough()
+      const removeEventSpy = spyOn(tooltipEl, 'removeEventListener').and.callThrough()
+
       const tooltip = new Tooltip(tooltipEl)
 
       expect(Tooltip.getInstance(tooltipEl)).toEqual(tooltip)
 
+      const expectedArgs = [
+        ['mouseover', jasmine.any(Function), jasmine.any(Boolean)],
+        ['mouseout', jasmine.any(Function), jasmine.any(Boolean)],
+        ['focusin', jasmine.any(Function), jasmine.any(Boolean)],
+        ['focusout', jasmine.any(Function), jasmine.any(Boolean)]
+      ]
+
+      expect(addEventSpy.calls.allArgs()).toEqual(expectedArgs)
+
       tooltip.dispose()
 
       expect(Tooltip.getInstance(tooltipEl)).toEqual(null)
+      expect(removeEventSpy.calls.allArgs()).toEqual(expectedArgs)
     })
 
     it('should destroy a tooltip after it is shown and hidden', done => {