From 4ac711b5b4c0f733870cd8dd1f18b73f964275fc Mon Sep 17 00:00:00 2001
From: Ryan Berliner <22206986+RyanBerliner@users.noreply.github.com>
Date: Thu, 20 May 2021 09:50:53 -0400
Subject: [PATCH] Refactor `isVisible` helper, fixing false positives from deep
 nesting or alternate means (#33960)

---
 js/src/util/index.js             | 13 ++------
 js/tests/unit/util/index.spec.js | 52 ++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/js/src/util/index.js b/js/src/util/index.js
index 6b38a05e94..77bdc072fc 100644
--- a/js/src/util/index.js
+++ b/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 => {
diff --git a/js/tests/unit/util/index.spec.js b/js/tests/unit/util/index.spec.js
index ca6430bee5..774945d1f9 100644
--- a/js/tests/unit/util/index.spec.js
+++ b/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', () => {
-- 
2.47.3