Fix carousel RTL and refactor code, fix rtl swipe issues (#32913)

* move common code to reusable functions

* add/re-factor tests, directionToOrder func

* add _orderToDirection tests

Co-authored-by: XhmikosR <xhmikosr@gmail.com>

.bundlewatch.config.json

@@ -38,7 +38,7 @@
     },
     {
       "path": "./dist/js/bootstrap.bundle.min.js",
-      "maxSize": "22.5 kB"
+      "maxSize": "22.75 kB"
     },
     {
       "path": "./dist/js/bootstrap.esm.js",

js/src/carousel.js

@@ -10,8 +10,8 @@ import {
   emulateTransitionEnd,
   getElementFromSelector,
   getTransitionDurationFromElement,
-  isVisible,
   isRTL,
+  isVisible,
   reflow,
   triggerTransitionEnd,
   typeCheckConfig
@@ -56,8 +56,8 @@ const DefaultType = {
   touch: 'boolean'
 }
 
-const DIRECTION_NEXT = 'next'
+const ORDER_NEXT = 'next'
-const DIRECTION_PREV = 'prev'
+const ORDER_PREV = 'prev'
 const DIRECTION_LEFT = 'left'
 const DIRECTION_RIGHT = 'right'
 
@@ -137,7 +137,7 @@ class Carousel extends BaseComponent {
 
   next() {
     if (!this._isSliding) {
-      this._slide(DIRECTION_NEXT)
+      this._slide(ORDER_NEXT)
     }
   }
 
@@ -151,7 +151,7 @@ class Carousel extends BaseComponent {
 
   prev() {
     if (!this._isSliding) {
-      this._slide(DIRECTION_PREV)
+      this._slide(ORDER_PREV)
     }
   }
 
@@ -208,11 +208,11 @@ class Carousel extends BaseComponent {
       return
     }
 
-    const direction = index > activeIndex ?
+    const order = index > activeIndex ?
-      DIRECTION_NEXT :
+      ORDER_NEXT :
-      DIRECTION_PREV
+      ORDER_PREV
 
-    this._slide(direction, this._items[index])
+    this._slide(order, this._items[index])
   }
 
   dispose() {
@@ -251,23 +251,11 @@ class Carousel extends BaseComponent {
 
     this.touchDeltaX = 0
 
-    // swipe left
+    if (!direction) {
-    if (direction > 0) {
+      return
-      if (isRTL()) {
-        this.next()
-      } else {
-        this.prev()
-      }
     }
 
-    // swipe right
+    this._slide(direction > 0 ? DIRECTION_RIGHT : DIRECTION_LEFT)
-    if (direction < 0) {
-      if (isRTL()) {
-        this.prev()
-      } else {
-        this.next()
-      }
-    }
   }
 
   _addEventListeners() {
@@ -350,18 +338,10 @@ class Carousel extends BaseComponent {
 
     if (event.key === ARROW_LEFT_KEY) {
       event.preventDefault()
-      if (isRTL()) {
+      this._slide(DIRECTION_LEFT)
-        this.next()
-      } else {
-        this.prev()
-      }
     } else if (event.key === ARROW_RIGHT_KEY) {
       event.preventDefault()
-      if (isRTL()) {
+      this._slide(DIRECTION_RIGHT)
-        this.prev()
-      } else {
-        this.next()
-      }
     }
   }
 
@@ -373,19 +353,18 @@ class Carousel extends BaseComponent {
     return this._items.indexOf(element)
   }
 
-  _getItemByDirection(direction, activeElement) {
+  _getItemByOrder(order, activeElement) {
-    const isNextDirection = direction === DIRECTION_NEXT
+    const isNext = order === ORDER_NEXT
-    const isPrevDirection = direction === DIRECTION_PREV
+    const isPrev = order === ORDER_PREV
     const activeIndex = this._getItemIndex(activeElement)
     const lastItemIndex = this._items.length - 1
-    const isGoingToWrap = (isPrevDirection && activeIndex === 0) ||
+    const isGoingToWrap = (isPrev && activeIndex === 0) || (isNext && activeIndex === lastItemIndex)
-                            (isNextDirection && activeIndex === lastItemIndex)
 
     if (isGoingToWrap && !this._config.wrap) {
       return activeElement
     }
 
-    const delta = direction === DIRECTION_PREV ? -1 : 1
+    const delta = isPrev ? -1 : 1
     const itemIndex = (activeIndex + delta) % this._items.length
 
     return itemIndex === -1 ?
@@ -441,17 +420,19 @@ class Carousel extends BaseComponent {
     }
   }
 
-  _slide(direction, element) {
+  _slide(directionOrOrder, element) {
+    const order = this._directionToOrder(directionOrOrder)
     const activeElement = SelectorEngine.findOne(SELECTOR_ACTIVE_ITEM, this._element)
     const activeElementIndex = this._getItemIndex(activeElement)
-    const nextElement = element || (activeElement && this._getItemByDirection(direction, activeElement))
+    const nextElement = element || this._getItemByOrder(order, activeElement)
 
     const nextElementIndex = this._getItemIndex(nextElement)
     const isCycling = Boolean(this._interval)
 
-    const directionalClassName = direction === DIRECTION_NEXT ? CLASS_NAME_START : CLASS_NAME_END
+    const isNext = order === ORDER_NEXT
-    const orderClassName = direction === DIRECTION_NEXT ? CLASS_NAME_NEXT : CLASS_NAME_PREV
+    const directionalClassName = isNext ? CLASS_NAME_START : CLASS_NAME_END
-    const eventDirectionName = direction === DIRECTION_NEXT ? DIRECTION_LEFT : DIRECTION_RIGHT
+    const orderClassName = isNext ? CLASS_NAME_NEXT : CLASS_NAME_PREV
+    const eventDirectionName = this._orderToDirection(order)
 
     if (nextElement && nextElement.classList.contains(CLASS_NAME_ACTIVE)) {
       this._isSliding = false
@@ -524,6 +505,30 @@ class Carousel extends BaseComponent {
     }
   }
 
+  _directionToOrder(direction) {
+    if (![DIRECTION_RIGHT, DIRECTION_LEFT].includes(direction)) {
+      return direction
+    }
+
+    if (isRTL()) {
+      return direction === DIRECTION_RIGHT ? ORDER_PREV : ORDER_NEXT
+    }
+
+    return direction === DIRECTION_RIGHT ? ORDER_NEXT : ORDER_PREV
+  }
+
+  _orderToDirection(order) {
+    if (![ORDER_NEXT, ORDER_PREV].includes(order)) {
+      return order
+    }
+
+    if (isRTL()) {
+      return order === ORDER_NEXT ? DIRECTION_LEFT : DIRECTION_RIGHT
+    }
+
+    return order === ORDER_NEXT ? DIRECTION_RIGHT : DIRECTION_LEFT
+  }
+
   // Static
 
   static carouselInterface(element, config) {

js/tests/unit/carousel.spec.js

@@ -2,7 +2,8 @@ import Carousel from '../../src/carousel'
 import EventHandler from '../../src/dom/event-handler'
 
 /** Test helpers */
-import { getFixture, clearFixture, createEvent, jQueryMock } from '../helpers/fixture'
+import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
+import * as util from '../../src/util'
 
 describe('Carousel', () => {
   const { Simulator, PointerEvent } = window
@@ -175,8 +176,7 @@ describe('Carousel', () => {
       })
 
       const spyKeydown = spyOn(carousel, '_keydown').and.callThrough()
-      const spyPrev = spyOn(carousel, 'prev')
+      const spySlide = spyOn(carousel, '_slide')
-      const spyNext = spyOn(carousel, 'next')
 
       const keydown = createEvent('keydown', { bubbles: true, cancelable: true })
       keydown.key = 'ArrowRight'
@@ -189,12 +189,10 @@ describe('Carousel', () => {
       input.dispatchEvent(keydown)
 
       expect(spyKeydown).toHaveBeenCalled()
-      expect(spyPrev).not.toHaveBeenCalled()
+      expect(spySlide).not.toHaveBeenCalled()
-      expect(spyNext).not.toHaveBeenCalled()
 
       spyKeydown.calls.reset()
-      spyPrev.calls.reset()
+      spySlide.calls.reset()
-      spyNext.calls.reset()
 
       Object.defineProperty(keydown, 'target', {
         value: textarea
@@ -202,8 +200,7 @@ describe('Carousel', () => {
       textarea.dispatchEvent(keydown)
 
       expect(spyKeydown).toHaveBeenCalled()
-      expect(spyPrev).not.toHaveBeenCalled()
+      expect(spySlide).not.toHaveBeenCalled()
-      expect(spyNext).not.toHaveBeenCalled()
     })
 
     it('should wrap around from end to start when wrap option is true', done => {
@@ -320,7 +317,7 @@ describe('Carousel', () => {
       expect(carousel._addTouchEventListeners).toHaveBeenCalled()
     })
 
-    it('should allow swiperight and call prev with pointer events', done => {
+    it('should allow swiperight and call _slide with pointer events', done => {
       if (!supportPointerEvent) {
         expect().nothing()
         done()
@@ -348,11 +345,12 @@ describe('Carousel', () => {
       const item = fixtureEl.querySelector('#item')
       const carousel = new Carousel(carouselEl)
 
-      spyOn(carousel, 'prev').and.callThrough()
+      spyOn(carousel, '_slide').and.callThrough()
 
-      carouselEl.addEventListener('slid.bs.carousel', () => {
+      carouselEl.addEventListener('slid.bs.carousel', event => {
         expect(item.classList.contains('active')).toEqual(true)
-        expect(carousel.prev).toHaveBeenCalled()
+        expect(carousel._slide).toHaveBeenCalledWith('right')
+        expect(event.direction).toEqual('right')
         document.head.removeChild(stylesCarousel)
         delete document.documentElement.ontouchstart
         done()
@@ -364,7 +362,7 @@ describe('Carousel', () => {
       })
     })
 
-    it('should allow swipeleft and call next with pointer events', done => {
+    it('should allow swipeleft and call previous with pointer events', done => {
       if (!supportPointerEvent) {
         expect().nothing()
         done()
@@ -392,11 +390,12 @@ describe('Carousel', () => {
       const item = fixtureEl.querySelector('#item')
       const carousel = new Carousel(carouselEl)
 
-      spyOn(carousel, 'next').and.callThrough()
+      spyOn(carousel, '_slide').and.callThrough()
 
-      carouselEl.addEventListener('slid.bs.carousel', () => {
+      carouselEl.addEventListener('slid.bs.carousel', event => {
         expect(item.classList.contains('active')).toEqual(false)
-        expect(carousel.next).toHaveBeenCalled()
+        expect(carousel._slide).toHaveBeenCalledWith('left')
+        expect(event.direction).toEqual('left')
         document.head.removeChild(stylesCarousel)
         delete document.documentElement.ontouchstart
         done()
@@ -409,7 +408,7 @@ describe('Carousel', () => {
       })
     })
 
-    it('should allow swiperight and call prev with touch events', done => {
+    it('should allow swiperight and call _slide with touch events', done => {
       Simulator.setType('touch')
       clearPointerEvents()
       document.documentElement.ontouchstart = () => {}
@@ -431,11 +430,12 @@ describe('Carousel', () => {
       const item = fixtureEl.querySelector('#item')
       const carousel = new Carousel(carouselEl)
 
-      spyOn(carousel, 'prev').and.callThrough()
+      spyOn(carousel, '_slide').and.callThrough()
 
-      carouselEl.addEventListener('slid.bs.carousel', () => {
+      carouselEl.addEventListener('slid.bs.carousel', event => {
         expect(item.classList.contains('active')).toEqual(true)
-        expect(carousel.prev).toHaveBeenCalled()
+        expect(carousel._slide).toHaveBeenCalledWith('right')
+        expect(event.direction).toEqual('right')
         delete document.documentElement.ontouchstart
         restorePointerEvents()
         done()
@@ -447,7 +447,7 @@ describe('Carousel', () => {
       })
     })
 
-    it('should allow swipeleft and call next with touch events', done => {
+    it('should allow swipeleft and call _slide with touch events', done => {
       Simulator.setType('touch')
       clearPointerEvents()
       document.documentElement.ontouchstart = () => {}
@@ -469,11 +469,12 @@ describe('Carousel', () => {
       const item = fixtureEl.querySelector('#item')
       const carousel = new Carousel(carouselEl)
 
-      spyOn(carousel, 'next').and.callThrough()
+      spyOn(carousel, '_slide').and.callThrough()
 
-      carouselEl.addEventListener('slid.bs.carousel', () => {
+      carouselEl.addEventListener('slid.bs.carousel', event => {
         expect(item.classList.contains('active')).toEqual(false)
-        expect(carousel.next).toHaveBeenCalled()
+        expect(carousel._slide).toHaveBeenCalledWith('left')
+        expect(event.direction).toEqual('left')
         delete document.documentElement.ontouchstart
         restorePointerEvents()
         done()
@@ -600,7 +601,7 @@ describe('Carousel', () => {
       const carousel = new Carousel(carouselEl, {})
 
       const onSlide = e => {
-        expect(e.direction).toEqual('left')
+        expect(e.direction).toEqual('right')
         expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true)
         expect(e.from).toEqual(0)
         expect(e.to).toEqual(1)
@@ -612,7 +613,7 @@ describe('Carousel', () => {
       }
 
       const onSlide2 = e => {
-        expect(e.direction).toEqual('right')
+        expect(e.direction).toEqual('left')
         done()
       }
 
@@ -635,7 +636,7 @@ describe('Carousel', () => {
       const carousel = new Carousel(carouselEl, {})
 
       const onSlid = e => {
-        expect(e.direction).toEqual('left')
+        expect(e.direction).toEqual('right')
         expect(e.relatedTarget.classList.contains('carousel-item')).toEqual(true)
         expect(e.from).toEqual(0)
         expect(e.to).toEqual(1)
@@ -647,7 +648,7 @@ describe('Carousel', () => {
       }
 
       const onSlid2 = e => {
-        expect(e.direction).toEqual('right')
+        expect(e.direction).toEqual('left')
         done()
       }
 
@@ -1061,6 +1062,77 @@ describe('Carousel', () => {
       })
     })
   })
+  describe('rtl function', () => {
+    it('"_directionToOrder" and "_orderToDirection" must return the right results', () => {
+      fixtureEl.innerHTML = '<div></div>'
+
+      const carouselEl = fixtureEl.querySelector('div')
+      const carousel = new Carousel(carouselEl, {})
+
+      expect(carousel._directionToOrder('left')).toEqual('prev')
+      expect(carousel._directionToOrder('prev')).toEqual('prev')
+      expect(carousel._directionToOrder('right')).toEqual('next')
+      expect(carousel._directionToOrder('next')).toEqual('next')
+
+      expect(carousel._orderToDirection('next')).toEqual('right')
+      expect(carousel._orderToDirection('prev')).toEqual('left')
+    })
+
+    it('"_directionToOrder" and "_orderToDirection" must return the right results when rtl=true', () => {
+      document.documentElement.dir = 'rtl'
+      fixtureEl.innerHTML = '<div></div>'
+
+      const carouselEl = fixtureEl.querySelector('div')
+      const carousel = new Carousel(carouselEl, {})
+      expect(util.isRTL()).toEqual(true, 'rtl has to be true')
+
+      expect(carousel._directionToOrder('left')).toEqual('next')
+      expect(carousel._directionToOrder('prev')).toEqual('prev')
+      expect(carousel._directionToOrder('right')).toEqual('prev')
+      expect(carousel._directionToOrder('next')).toEqual('next')
+
+      expect(carousel._orderToDirection('next')).toEqual('left')
+      expect(carousel._orderToDirection('prev')).toEqual('right')
+      document.documentElement.dir = 'ltl'
+    })
+
+    it('"_slide" has to call _directionToOrder and "_orderToDirection"', () => {
+      fixtureEl.innerHTML = '<div></div>'
+
+      const carouselEl = fixtureEl.querySelector('div')
+      const carousel = new Carousel(carouselEl, {})
+      const spy = spyOn(carousel, '_directionToOrder').and.callThrough()
+      const spy2 = spyOn(carousel, '_orderToDirection').and.callThrough()
+
+      carousel._slide('left')
+      expect(spy).toHaveBeenCalledWith('left')
+      expect(spy2).toHaveBeenCalledWith('prev')
+
+      carousel._slide('right')
+      expect(spy).toHaveBeenCalledWith('right')
+      expect(spy2).toHaveBeenCalledWith('next')
+    })
+
+    it('"_slide" has to call "_directionToOrder" and "_orderToDirection" when rtl=true', () => {
+      document.documentElement.dir = 'rtl'
+      fixtureEl.innerHTML = '<div></div>'
+
+      const carouselEl = fixtureEl.querySelector('div')
+      const carousel = new Carousel(carouselEl, {})
+      const spy = spyOn(carousel, '_directionToOrder').and.callThrough()
+      const spy2 = spyOn(carousel, '_orderToDirection').and.callThrough()
+
+      carousel._slide('left')
+      expect(spy).toHaveBeenCalledWith('left')
+      expect(spy2).toHaveBeenCalledWith('next')
+
+      carousel._slide('right')
+      expect(spy).toHaveBeenCalledWith('right')
+      expect(spy2).toHaveBeenCalledWith('prev')
+
+      document.documentElement.dir = 'ltl'
+    })
+  })
 
   describe('dispose', () => {
     it('should destroy a carousel', () => {