From 8691645dcaeba4354a76757f67843d6ad11517b7 Mon Sep 17 00:00:00 2001
From: Ernesto Garcia <gnapse@gmail.com>
Date: Thu, 13 Jan 2022 18:29:19 -0300
Subject: [PATCH 1/2] Refactor toBeVisible tests to be more structured

---
 src/__tests__/to-be-visible.js | 145 ++++++++++++++++++++++++++++-----
 1 file changed, 123 insertions(+), 22 deletions(-)

diff --git a/src/__tests__/to-be-visible.js b/src/__tests__/to-be-visible.js
index f2b4277a..25f01271 100644
--- a/src/__tests__/to-be-visible.js
+++ b/src/__tests__/to-be-visible.js
@@ -2,44 +2,145 @@ import {render} from './helpers/test-utils'
 import document from './helpers/document'
 
 describe('.toBeVisible', () => {
-  it('returns the visibility of an element', () => {
+  it('considers elements to be visible by default', () => {
     const {container} = render(`
       <div>
         <header>
-          <h1 style="display: none">Main title</h1>
-          <h2 style="visibility: hidden">Secondary title</h2>
-          <h3 style="visibility: collapse">Secondary title</h3>
-          <h4 style="opacity: 0">Secondary title</h4>
-          <h5 style="opacity: 0.1">Secondary title</h5>
+          <h1>This is the title</h1>
         </header>
-        <button hidden>Hidden button</button>
-        <section style="display: block; visibility: hidden">
+        <button>Hidden button</button>
+        <section>
           <p>Hello <strong>World</strong></p>
         </section>
       </div>
     `)
 
     expect(container.querySelector('header')).toBeVisible()
-    expect(container.querySelector('h1')).not.toBeVisible()
-    expect(container.querySelector('h2')).not.toBeVisible()
-    expect(container.querySelector('h3')).not.toBeVisible()
-    expect(container.querySelector('h4')).not.toBeVisible()
-    expect(container.querySelector('h5')).toBeVisible()
-    expect(container.querySelector('button')).not.toBeVisible()
-    expect(container.querySelector('strong')).not.toBeVisible()
+    expect(container.querySelector('h1')).toBeVisible()
+    expect(container.querySelector('button')).toBeVisible()
+    expect(container.querySelector('strong')).toBeVisible()
 
     expect(() =>
       expect(container.querySelector('header')).not.toBeVisible(),
     ).toThrowError()
-    expect(() =>
-      expect(container.querySelector('p')).toBeVisible(),
-    ).toThrowError()
   })
 
-  test('detached element is not visible', () => {
-    const subject = document.createElement('div')
-    expect(subject).not.toBeVisible()
-    expect(() => expect(subject).toBeVisible()).toThrowError()
+  describe('with the "hidden" attribute', () => {
+    it('considers an element to not be visible', () => {
+      const {container} = render('<button hidden>Click me</button>')
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
+  })
+
+  describe('display', () => {
+    it.each([
+      ['inline'],
+      ['block'],
+      ['inline-block'],
+      ['flex'],
+      ['inline-flex'],
+      ['grid'],
+      ['inline-grid'],
+    ])('considers "display: %s" as visible', display => {
+      const {container} = render(`
+        <div style="display: ${display}">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).not.toBeVisible(),
+      ).toThrowError()
+    })
+
+    it('considers "display: none" as not visible', () => {
+      const {container} = render(`
+        <div style="display: none">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
+  })
+
+  describe('visibility', () => {
+    it('considers "visibility: collapse" as visible', () => {
+      const {container} = render(`
+        <div style="visibility: visible">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).not.toBeVisible(),
+      ).toThrowError()
+    })
+
+    it('considers "visibility: hidden" as not visible', () => {
+      const {container} = render(`
+        <div style="visibility: hidden">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
+
+    it('considers "visibility: collapse" as not visible', () => {
+      const {container} = render(`
+        <div style="visibility: hidden">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
+  })
+
+  describe('opacity', () => {
+    it('considers "opacity: 0" as not visible', () => {
+      const {container} = render(`
+        <div style="opacity: 0">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
+
+    it('considers "opacity > 0" as visible', () => {
+      const {container} = render(`
+        <div style="opacity: 0.1">
+          <button>Click me</button>
+        </div>,
+      `)
+      expect(container.querySelector('button')).toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).not.toBeVisible(),
+      ).toThrowError()
+    })
+  })
+
+  describe('detached element', () => {
+    it('is not visible', () => {
+      const subject = document.createElement('div')
+      expect(subject).not.toBeVisible()
+      expect(() => {
+        expect(subject).toBeVisible()
+      }).toThrowError()
+    })
   })
 
   describe('with a <details /> element', () => {

From 78b786ea99f6ad91425b1ed7a0cba13ed2cf64ba Mon Sep 17 00:00:00 2001
From: Ernesto Garcia <gnapse@gmail.com>
Date: Thu, 13 Jan 2022 19:33:00 -0300
Subject: [PATCH 2/2] Fix toBeVisible matcher to correctly evaluate visibility
 CSS

---
 src/__tests__/to-be-visible.js | 38 +++++++++++++++++++-
 src/to-be-visible.js           | 66 ++++++++++++++++++++++++----------
 2 files changed, 85 insertions(+), 19 deletions(-)

diff --git a/src/__tests__/to-be-visible.js b/src/__tests__/to-be-visible.js
index 25f01271..002bfc0a 100644
--- a/src/__tests__/to-be-visible.js
+++ b/src/__tests__/to-be-visible.js
@@ -67,10 +67,22 @@ describe('.toBeVisible', () => {
         expect(container.querySelector('button')).toBeVisible(),
       ).toThrowError()
     })
+
+    it('does not allow child elements to override invisibility by changing their own display style', () => {
+      const {container} = render(`
+        <div style="display: none">
+          <button style="display: block">Click me</button>
+        </div>
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
   })
 
   describe('visibility', () => {
-    it('considers "visibility: collapse" as visible', () => {
+    it('considers "visibility: visible" as visible', () => {
       const {container} = render(`
         <div style="visibility: visible">
           <button>Click me</button>
@@ -105,6 +117,18 @@ describe('.toBeVisible', () => {
         expect(container.querySelector('button')).toBeVisible(),
       ).toThrowError()
     })
+
+    it('allows child elements to override invisibility by changing their own visibility style', () => {
+      const {container} = render(`
+        <div style="visibility: hidden">
+          <button style="visibility: visible">Click me</button>
+        </div>
+      `)
+      expect(container.querySelector('button')).toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).not.toBeVisible(),
+      ).toThrowError()
+    })
   })
 
   describe('opacity', () => {
@@ -131,6 +155,18 @@ describe('.toBeVisible', () => {
         expect(container.querySelector('button')).not.toBeVisible(),
       ).toThrowError()
     })
+
+    it('does not allow child elements to override invisibility by increasing their own opacity', () => {
+      const {container} = render(`
+        <div style="opacity: 0">
+          <button style="opacity: 1">Click me</button>
+        </div>
+      `)
+      expect(container.querySelector('button')).not.toBeVisible()
+      expect(() =>
+        expect(container.querySelector('button')).toBeVisible(),
+      ).toThrowError()
+    })
   })
 
   describe('detached element', () => {
diff --git a/src/to-be-visible.js b/src/to-be-visible.js
index 518b7d56..12786a6a 100644
--- a/src/to-be-visible.js
+++ b/src/to-be-visible.js
@@ -1,33 +1,63 @@
 import {checkHtmlElement} from './utils'
 
-function isStyleVisible(element) {
+function getElementVisibilityStyle(element) {
+  if (!element) return 'visible'
   const {getComputedStyle} = element.ownerDocument.defaultView
+  const {visibility} = getComputedStyle(element)
+  return visibility || getElementVisibilityStyle(element.parentElement)
+}
 
-  const {display, visibility, opacity} = getComputedStyle(element)
-  return (
-    display !== 'none' &&
-    visibility !== 'hidden' &&
-    visibility !== 'collapse' &&
-    opacity !== '0' &&
-    opacity !== 0
-  )
+function isVisibleSummaryDetails(element, previousElement) {
+  return element.nodeName === 'DETAILS' &&
+    previousElement.nodeName !== 'SUMMARY'
+    ? element.hasAttribute('open')
+    : true
 }
 
-function isAttributeVisible(element, previousElement) {
+function isElementTreeVisible(element, previousElement = undefined) {
+  const {getComputedStyle} = element.ownerDocument.defaultView
+  const {display, opacity} = getComputedStyle(element)
   return (
+    display !== 'none' &&
+    opacity !== '0' &&
     !element.hasAttribute('hidden') &&
-    (element.nodeName === 'DETAILS' && previousElement.nodeName !== 'SUMMARY'
-      ? element.hasAttribute('open')
+    isVisibleSummaryDetails(element, previousElement) &&
+    (element.parentElement
+      ? isElementTreeVisible(element.parentElement, element)
       : true)
   )
 }
 
-function isElementVisible(element, previousElement) {
-  return (
-    isStyleVisible(element) &&
-    isAttributeVisible(element, previousElement) &&
-    (!element.parentElement || isElementVisible(element.parentElement, element))
-  )
+function isElementVisibilityVisible(element) {
+  const visibility = getElementVisibilityStyle(element)
+  return visibility !== 'hidden' && visibility !== 'collapse'
+}
+
+/**
+ * Computes the boolean value that determines if an element is considered visible from the
+ * `toBeVisible` custom matcher point of view.
+ *
+ * Visibility is controlled via two different sets of properties and styles.
+ *
+ * 1. One set of properties allow parent elements to fully controls its sub-tree visibility. This
+ *    means that if higher up in the tree some element is not visible by this criteria, it makes the
+ *    entire sub-tree not visible too, and there's nothing that child elements can do to revert it.
+ *    This includes `display: none`, `opacity: 0`, the presence of the `hidden` attribute`, and the
+ *    open state of a details/summary elements pair.
+ *
+ * 2. The other aspect influencing if an element is visible is the CSS `visibility` style. This one
+ *    is also inherited. But unlike the previous case, this one can be reverted by child elements.
+ *    A parent element can set its visibiilty to `hidden` or `collapse`, but a child element setting
+ *    its own styles to `visibility: visible` can rever that, and it makes itself visible. Hence,
+ *    this criteria needs to be checked independently of the other one.
+ *
+ * Hence, the apprach taken by this function is two-fold: it first gets the first set of criteria
+ * out of the way, analyzing the target element and up its tree. If this branch yields that the
+ * element is not visible, there's nothing the element could be doing to revert that, so it returns
+ * false. Only if the first check is true, if proceeds to analyze the `visibility` CSS.
+ */
+function isElementVisible(element) {
+  return isElementTreeVisible(element) && isElementVisibilityVisible(element)
 }
 
 export function toBeVisible(element) {