Refactor isVisible helper, fixing false positives from deep nesting or alternate means (#33960)

Author: RyanBerliner

js/src/util/index.js

@@ -159,20 +159,11 @@ const typeCheckConfig = (componentName, config, configTypes) => {
 }
 
 const isVisible = element => {
-  if (!element) {
+  if (!isElement(element) || element.getClientRects().length === 0) {
     return false
   }
 
-  if (element.style && element.parentNode && element.parentNode.style) {
-    const elementStyle = getComputedStyle(element)
-    const parentNodeStyle = getComputedStyle(element.parentNode)
-
-    return elementStyle.display !== 'none' &&
-      parentNodeStyle.display !== 'none' &&
-      elementStyle.visibility !== 'hidden'
-  }
-
-  return false
+  return getComputedStyle(element).getPropertyValue('visibility') === 'visible'
 }
 
 const isDisabled = element => {

js/tests/unit/util/index.spec.js

@@ -326,10 +326,14 @@ describe('Util', () => {
       expect(Util.isVisible(div)).toEqual(false)
     })
 
-    it('should return false if the parent element is not visible', () => {
+    it('should return false if an ancestor element is display none', () => {
       fixtureEl.innerHTML = [
         '<div style="display: none;">',
-        '  <div class="content"></div>',
+        '  <div>',
+        '    <div>',
+        '      <div class="content"></div>',
+        '    </div>',
+        '  </div>',
         '</div>'
       ].join('')
 
@@ -338,6 +342,38 @@ describe('Util', () => {
       expect(Util.isVisible(div)).toEqual(false)
     })
 
+    it('should return false if an ancestor element is visibility hidden', () => {
+      fixtureEl.innerHTML = [
+        '<div style="visibility: hidden;">',
+        '  <div>',
+        '    <div>',
+        '      <div class="content"></div>',
+        '    </div>',
+        '  </div>',
+        '</div>'
+      ].join('')
+
+      const div = fixtureEl.querySelector('.content')
+
+      expect(Util.isVisible(div)).toEqual(false)
+    })
+
+    it('should return true if an ancestor element is visibility hidden, but reverted', () => {
+      fixtureEl.innerHTML = [
+        '<div style="visibility: hidden;">',
+        '  <div style="visibility: visible;">',
+        '    <div>',
+        '      <div class="content"></div>',
+        '    </div>',
+        '  </div>',
+        '</div>'
+      ].join('')
+
+      const div = fixtureEl.querySelector('.content')
+
+      expect(Util.isVisible(div)).toEqual(true)
+    })
+
     it('should return true if the element is visible', () => {
       fixtureEl.innerHTML = [
         '<div>',
@@ -349,6 +385,18 @@ describe('Util', () => {
 
       expect(Util.isVisible(div)).toEqual(true)
     })
+
+    it('should return false if the element is hidden, but not via display or visibility', () => {
+      fixtureEl.innerHTML = [
+        '<details>',
+        '  <div id="element"></div>',
+        '</details>'
+      ].join('')
+
+      const div = fixtureEl.querySelector('#element')
+
+      expect(Util.isVisible(div)).toEqual(false)
+    })
   })
 
   describe('isDisabled', () => {