Move get element functionality to a helper (#33327)

Looking around on js components I found out many checks, different expressed but with same purpose.
Some of them are trying to parse string to element, others, jQuery element to js simple nodeElement etc

With this Pr, I am trying to give a standard way to parse an element

So this pr:

* Creates `getElement` helper that tries to parse an argument to element or null
* Changes `isElement` to make  explicit checks and return Boolean
* fixes tests deficiencies

diff --git a/js/src/base-component.js b/js/src/base-component.js
index eacc8420bce03616e45f191f2d50c31e86a4337b..05b0adcd26e1dc05c752f48ac0de2c44dc41e6c2 100644 (file)
--- a/js/src/base-component.js
+++ b/js/src/base-component.js
@@ -9,6 +9,7 @@ import Data from './dom/data'
 import {
   emulateTransitionEnd,
   execute,
+  getElement,
   getTransitionDurationFromElement
 } from './util/index'
 import EventHandler from './dom/event-handler'
@@ -23,7 +24,7 @@ const VERSION = '5.0.0'
 
 class BaseComponent {
   constructor(element) {
-    element = typeof element === 'string' ? document.querySelector(element) : element
+    element = getElement(element)
 
     if (!element) {
       return

diff --git a/js/src/collapse.js b/js/src/collapse.js
index 1c8f53ebd62584d95627a1c847d44ad2d0094f61..3241a71327e101b79f95005f5b33134cd3bfd376 100644 (file)
--- a/js/src/collapse.js
+++ b/js/src/collapse.js
@@ -7,9 +7,9 @@
 
 import {
   defineJQueryPlugin,
+  getElement,
   getSelectorFromElement,
   getElementFromSelector,
-  isElement,
   reflow,
   typeCheckConfig
 } from './util/index'
@@ -272,14 +272,7 @@ class Collapse extends BaseComponent {
   _getParent() {
     let { parent } = this._config
 
-    if (isElement(parent)) {
-      // it's a jQuery object
-      if (typeof parent.jquery !== 'undefined' || typeof parent[0] !== 'undefined') {
-        parent = parent[0]
-      }
-    } else {
-      parent = SelectorEngine.findOne(parent)
-    }
+    parent = getElement(parent)
 
     const selector = `${SELECTOR_DATA_TOGGLE}[data-bs-parent="${parent}"]`
 

diff --git a/js/src/dropdown.js b/js/src/dropdown.js
index 8f501d8113e258d01b6ea2dd7724f20be93da678..3eb5891f8f855c7933fff4f2d94f4db1a017c876 100644 (file)
--- a/js/src/dropdown.js
+++ b/js/src/dropdown.js
@@ -9,6 +9,7 @@ import * as Popper from '@popperjs/core'
 
 import {
   defineJQueryPlugin,
+  getElement,
   getElementFromSelector,
   isDisabled,
   isElement,
@@ -166,12 +167,7 @@ class Dropdown extends BaseComponent {
       if (this._config.reference === 'parent') {
         referenceElement = parent
       } else if (isElement(this._config.reference)) {
-        referenceElement = this._config.reference
-
-        // Check if it's jQuery element
-        if (typeof this._config.reference.jquery !== 'undefined') {
-          referenceElement = this._config.reference[0]
-        }
+        referenceElement = getElement(this._config.reference)
       } else if (typeof this._config.reference === 'object') {
         referenceElement = this._config.reference
       }

diff --git a/js/src/tooltip.js b/js/src/tooltip.js
index e440573829fdcd69043f8ccb96c03fded0002f40..fb46578393e553a6df453a0b8cc489afe101a209 100644 (file)
--- a/js/src/tooltip.js
+++ b/js/src/tooltip.js
@@ -10,6 +10,7 @@ import * as Popper from '@popperjs/core'
 import {
   defineJQueryPlugin,
   findShadowRoot,
+  getElement,
   getUID,
   isElement,
   isRTL,
@@ -256,7 +257,7 @@ class Tooltip extends BaseComponent {
     const attachment = this._getAttachment(placement)
     this._addAttachmentClass(attachment)
 
-    const container = this._getContainer()
+    const { container } = this._config
     Data.set(tip, this.constructor.DATA_KEY, this)
 
     if (!this._element.ownerDocument.documentElement.contains(this.tip)) {
@@ -385,10 +386,8 @@ class Tooltip extends BaseComponent {
       return
     }
 
-    if (typeof content === 'object' && isElement(content)) {
-      if (content.jquery) {
-        content = content[0]
-      }
+    if (isElement(content)) {
+      content = getElement(content)
 
       // content is a DOM node or a jQuery
       if (this._config.html) {
@@ -518,18 +517,6 @@ class Tooltip extends BaseComponent {
     this.getTipElement().classList.add(`${CLASS_PREFIX}-${this.updateAttachment(attachment)}`)
   }
 
-  _getContainer() {
-    if (this._config.container === false) {
-      return document.body
-    }
-
-    if (isElement(this._config.container)) {
-      return this._config.container
-    }
-
-    return SelectorEngine.findOne(this._config.container)
-  }
-
   _getAttachment(placement) {
     return AttachmentMap[placement.toUpperCase()]
   }
@@ -664,16 +651,14 @@ class Tooltip extends BaseComponent {
       }
     })
 
-    if (config && typeof config.container === 'object' && config.container.jquery) {
-      config.container = config.container[0]
-    }
-
     config = {
       ...this.constructor.Default,
       ...dataAttributes,
       ...(typeof config === 'object' && config ? config : {})
     }
 
+    config.container = config.container === false ? document.body : getElement(config.container)
+
     if (typeof config.delay === 'number') {
       config.delay = {
         show: config.delay,

diff --git a/js/src/util/index.js b/js/src/util/index.js
index 0dd6b1d454cb87519f65dbadadd32b284798b58f..5ee211c73b9e5005ae34fe11febb2092c396eb58 100644 (file)
--- a/js/src/util/index.js
+++ b/js/src/util/index.js
@@ -1,3 +1,5 @@
+import SelectorEngine from '../dom/selector-engine'
+
 /**
  * --------------------------------------------------------------------------
  * Bootstrap (v5.0.0): util/index.js
@@ -100,7 +102,29 @@ const triggerTransitionEnd = element => {
   element.dispatchEvent(new Event(TRANSITION_END))
 }
 
-const isElement = obj => (obj[0] || obj).nodeType
+const isElement = obj => {
+  if (!obj || typeof obj !== 'object') {
+    return false
+  }
+
+  if (typeof obj.jquery !== 'undefined') {
+    obj = obj[0]
+  }
+
+  return typeof obj.nodeType !== 'undefined'
+}
+
+const getElement = obj => {
+  if (isElement(obj)) { // it's a jQuery object or a node element
+    return obj.jquery ? obj[0] : obj
+  }
+
+  if (typeof obj === 'string' && obj.length > 0) {
+    return SelectorEngine.findOne(obj)
+  }
+
+  return null
+}
 
 const emulateTransitionEnd = (element, duration) => {
   let called = false
@@ -238,6 +262,7 @@ const execute = callback => {
 }
 
 export {
+  getElement,
   getUID,
   getSelectorFromElement,
   getElementFromSelector,

diff --git a/js/tests/unit/collapse.spec.js b/js/tests/unit/collapse.spec.js
index 590b5795c76e0d1b99b67f4d9cc2b5a10b6fd2cc..0997ae7c205d08ba52277c471b01e835b354e417 100644 (file)
--- a/js/tests/unit/collapse.spec.js
+++ b/js/tests/unit/collapse.spec.js
@@ -58,7 +58,8 @@ describe('Collapse', () => {
       const collapseEl = fixtureEl.querySelector('div.collapse')
       const myCollapseEl = fixtureEl.querySelector('.my-collapse')
       const fakejQueryObject = {
-        0: myCollapseEl
+        0: myCollapseEl,
+        jquery: 'foo'
       }
       const collapse = new Collapse(collapseEl, {
         parent: fakejQueryObject

diff --git a/js/tests/unit/dropdown.spec.js b/js/tests/unit/dropdown.spec.js
index 7121857841453cd6826d52bf34bc0e3b8468af01..5275f1a5567031654841a046df1c656000d9cca9 100644 (file)
--- a/js/tests/unit/dropdown.spec.js
+++ b/js/tests/unit/dropdown.spec.js
@@ -3,7 +3,7 @@ import EventHandler from '../../src/dom/event-handler'
 import { noop } from '../../src/util'
 
 /** Test helpers */
-import { getFixture, clearFixture, createEvent, jQueryMock } from '../helpers/fixture'
+import { clearFixture, createEvent, getFixture, jQueryMock } from '../helpers/fixture'
 
 describe('Dropdown', () => {
   let fixtureEl
@@ -467,6 +467,7 @@ describe('Dropdown', () => {
 
       const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
       const virtualElement = {
+        nodeType: 1,
         getBoundingClientRect() {
           return {
             width: 0,

diff --git a/js/tests/unit/util/index.spec.js b/js/tests/unit/util/index.spec.js
index a7c1c28982efcd04cff7beac12f67988e3dc627e..b4010e0e0e25cc9d20f62e0977d6e342238d26e6 100644 (file)
--- a/js/tests/unit/util/index.spec.js
+++ b/js/tests/unit/util/index.spec.js
@@ -1,7 +1,7 @@
 import * as Util from '../../../src/util/index'
 
 /** Test helpers */
-import { getFixture, clearFixture } from '../../helpers/fixture'
+import { clearFixture, getFixture } from '../../helpers/fixture'
 
 describe('Util', () => {
   let fixtureEl
@@ -171,24 +171,58 @@ describe('Util', () => {
   })
 
   describe('isElement', () => {
-    it('should detect if the parameter is an element or not', () => {
-      fixtureEl.innerHTML = '<div></div>'
+    it('should detect if the parameter is an element or not and return Boolean', () => {
+      fixtureEl.innerHTML =
+        [
+          '<div id="foo" class="test"></div>',
+          '<div id="bar" class="test"></div>'
+        ].join('')
 
-      const el = document.querySelector('div')
+      const el = fixtureEl.querySelector('#foo')
 
-      expect(Util.isElement(el)).toEqual(el.nodeType)
-      expect(Util.isElement({})).toEqual(undefined)
+      expect(Util.isElement(el)).toEqual(true)
+      expect(Util.isElement({})).toEqual(false)
+      expect(Util.isElement(fixtureEl.querySelectorAll('.test'))).toEqual(false)
     })
 
     it('should detect jQuery element', () => {
       fixtureEl.innerHTML = '<div></div>'
 
-      const el = document.querySelector('div')
+      const el = fixtureEl.querySelector('div')
       const fakejQuery = {
-        0: el
+        0: el,
+        jquery: 'foo'
+      }
+
+      expect(Util.isElement(fakejQuery)).toEqual(true)
+    })
+  })
+
+  describe('getElement', () => {
+    it('should try to parse element', () => {
+      fixtureEl.innerHTML =
+        [
+          '<div id="foo" class="test"></div>',
+          '<div id="bar" class="test"></div>'
+        ].join('')
+
+      const el = fixtureEl.querySelector('div')
+
+      expect(Util.getElement(el)).toEqual(el)
+      expect(Util.getElement('#foo')).toEqual(el)
+      expect(Util.getElement('#fail')).toBeNull()
+      expect(Util.getElement({})).toBeNull()
+      expect(Util.getElement([])).toBeNull()
+      expect(Util.getElement()).toBeNull()
+      expect(Util.getElement(null)).toBeNull()
+      expect(Util.getElement(fixtureEl.querySelectorAll('.test'))).toBeNull()
+
+      const fakejQueryObject = {
+        0: el,
+        jquery: 'foo'
       }
 
-      expect(Util.isElement(fakejQuery)).toEqual(el.nodeType)
+      expect(Util.getElement(fakejQueryObject)).toEqual(el)
     })
   })