Skip to content

fix(sdk/testing): _skyTestingCheckVisibility overwrites pass across multiple rules and may throw on null element #4390

@coderabbitai

Description

@coderabbitai

Summary

In libs/sdk/testing/private/src/utility/check-visibility.ts, the _skyTestingCheckVisibility function has two related bugs that were carried over from the prior Jasmine toBeVisible matcher implementation.

Bug 1 – Later enabled rules can overwrite an earlier failure

Each enabled rule independently assigns pass rather than ANDing with the accumulated result. For example, with both checkCssDisplay and checkCssVisibility enabled, an element with display: none (which correctly sets pass = false) can have that result overwritten by a subsequent visibility: visible check (which sets pass = true), yielding an incorrect visible result.

Bug 2 – Unsafe access on null/undefined element

When checkExists is false (the default) and a null or undefined value is passed, the function proceeds to call window.getComputedStyle(el) and potentially el.getBoundingClientRect(), which will throw instead of returning false.

Suggested fix

 export function _skyTestingCheckVisibility(
-  el: Element,
+  el: Element | null | undefined,
   options?: _SkyTestingCheckVisibilityOptions,
 ): boolean {
   const settings = { ...DEFAULTS, ...options };

+  if (!el) {
+    return false;
+  }
+
   let pass = true;

-  if (settings.checkExists) {
-    pass = !!el;
-  }
-
   if (pass) {
     const computedStyle = window.getComputedStyle(el);

     if (settings.checkCssDisplay) {
-      pass = computedStyle.display !== 'none';
+      pass = pass && computedStyle.display !== 'none';
     }

     if (settings.checkCssVisibility) {
-      pass = computedStyle.visibility !== 'hidden';
+      pass = pass && computedStyle.visibility !== 'hidden';
     }

     if (settings.checkDimensions) {
       const box = el.getBoundingClientRect();
-      pass = box.width > 0 && box.height > 0;
+      pass = pass && box.width > 0 && box.height > 0;
     }
   }

A regression test with both CSS checks simultaneously enabled would also help lock this down.

Context

AB#3959363

Metadata

Metadata

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions