fix(engine): narrow visibility:hidden ancestor skip to sub-comp hosts

liriansu-opus · miguel-heygen · commit f3bb6dc125d1 · 2026-05-26T00:37:13.000-04:00
`isVisualAncestorHidden` was treating any `visibility: hidden` ancestor as a
signal to skip injecting the replacement frame. That's too broad — for plain
`[data-start]` containers, the replacement `&lt;img&gt;`'s explicit
`visibility: visible` correctly overrides the ancestor per CSS spec, and
consumers rely on that to hold the final GSAP-driven frame when an authored
`data-duration` outlives the composition's GSAP timeline (e.g.
`style-9-prod`, where the runtime truncates the host to `visibility: hidden`
after the timeline ends and the replacement frame must paint through).

Restrict the `visibility: hidden` skip to ancestors that carry
`data-composition-src` or `data-composition-file` — the actual sub-composition
hosts this guard was added for. `display: none` keeps the broad behavior:
it takes the whole subtree out of layout and a child override cannot escape.

Update the existing regression suite to mark the host as a sub-composition,
and add two new cases pinning the plain-`[data-start]` behavior: both
`injectVideoFramesBatch` and `syncVideoFrameVisibility` must still produce a
visible replacement `&lt;img&gt;` when the host is `visibility: hidden` but does
not carry a sub-composition attribute.
diff --git a/packages/engine/src/services/screenshotService.test.ts b/packages/engine/src/services/screenshotService.test.ts
@@ -202,6 +202,17 @@ describe("video-frame injection respects ancestor visibility", () => {
   // injected `data-start="0"` + probed full-source duration cover the
   // whole timeline), so the bug produced one full-bleed speaker overlay
   // per inactive sub-comp — covering whichever moment was actually visible.
+  //
+  // The skip is intentionally narrow: `visibility:hidden` on a regular
+  // `[data-start]` container must NOT skip injection, because the
+  // replacement <img>'s explicit `visibility:visible` overrides the
+  // ancestor (CSS spec) and consumers rely on that to hold the final
+  // GSAP-driven frame when an authored `data-duration` outlives the
+  // composition's GSAP timeline. We therefore only treat
+  // `visibility:hidden` as a skip signal on sub-composition hosts
+  // (`[data-composition-src]` / `[data-composition-file]`). `display:none`,
+  // by contrast, takes the whole subtree out of layout regardless of any
+  // child override, so it always triggers the skip.
 
   type StyleLike = {
     display?: string;
@@ -212,9 +223,19 @@ describe("video-frame injection respects ancestor visibility", () => {
     zIndex?: string;
   };
 
-  function setupHostHiddenScenario(hostStyle: StyleLike) {
+  type HostAttribute = "data-composition-src" | "data-composition-file" | "data-start";
+
+  function setupHostHiddenScenario(
+    hostStyle: StyleLike,
+    options: { hostAttribute?: HostAttribute } = {},
+  ) {
+    const hostAttribute = options.hostAttribute ?? "data-composition-src";
+    const hostAttrMarkup =
+      hostAttribute === "data-start"
+        ? 'data-start="0" data-duration="10"'
+        : `${hostAttribute}="sub.html"`;
     const { window, document } = parseHTML(
-      '<html><body><div id="host"><div id="pip-frame"><video id="pip" data-start="0" data-duration="10"></video></div></div></body></html>',
+      `<html><body><div id="host" ${hostAttrMarkup}><div id="pip-frame"><video id="pip" data-start="0" data-duration="10"></video></div></div></body></html>`,
     );
 
     Object.defineProperty(window.HTMLImageElement.prototype, "decode", {
@@ -382,4 +403,54 @@ describe("video-frame injection respects ancestor visibility", () => {
 
     expect(seededImg.style.visibility).toBe("hidden");
   });
+
+  it("still injects when a plain [data-start] host is visibility:hidden (CSS-escapable)", async () => {
+    // Regression guard for the style-9-prod symptom: a regular
+    // `[data-start]` container whose GSAP timeline is shorter than its
+    // authored `data-duration` ends up `visibility: hidden` past the
+    // timeline end. The replacement <img>'s explicit `visibility: visible`
+    // correctly overrides that per CSS spec, so the injector must NOT
+    // short-circuit — it would otherwise drop the final-state frame and
+    // produce blank tail frames.
+    const { teardown, setup } = withGlobals(
+      setupHostHiddenScenario({ visibility: "hidden" }, { hostAttribute: "data-start" }),
+    );
+
+    try {
+      await injectVideoFramesBatch(passthroughPage(), [
+        {
+          videoId: "pip",
+          dataUri:
+            "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkAAIAAAoAAv/lxKUAAAAASUVORK5CYII=",
+        },
+      ]);
+    } finally {
+      teardown();
+    }
+
+    const sibling = setup.video.nextElementSibling as HTMLElement | null;
+    expect(sibling).not.toBeNull();
+    expect(sibling?.classList.contains("__render_frame__")).toBe(true);
+    expect(sibling?.style.visibility).toBe("visible");
+  });
+
+  it("syncVideoFrameVisibility shows the replacement <img> when a plain [data-start] host is visibility:hidden", async () => {
+    const { teardown, setup } = withGlobals(
+      setupHostHiddenScenario({ visibility: "hidden" }, { hostAttribute: "data-start" }),
+    );
+    const seededImg = setup.document.createElement("img");
+    seededImg.classList.add("__render_frame__");
+    seededImg.style.visibility = "hidden";
+    setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling);
+
+    try {
+      await syncVideoFrameVisibility(passthroughPage(), ["pip"]);
+    } finally {
+      teardown();
+    }
+
+    // The host's `visibility: hidden` is escapable; sync must flip the
+    // <img> to `visibility: visible` so it overrides the ancestor.
+    expect(seededImg.style.visibility).toBe("visible");
+  });
 });
diff --git a/packages/engine/src/services/screenshotService.ts b/packages/engine/src/services/screenshotService.ts
@@ -386,19 +386,40 @@ export async function injectVideoFramesBatch(
         "bottom",
         "inset",
       ]);
-      // Walk ancestors looking for a host that the page has hidden via
-      // `display:none` or `visibility:hidden`. The runtime hides
-      // `[data-composition-src]` and `[data-start]` hosts that fall outside
-      // their time window using exactly these properties; a nested
-      // `<video data-start>` inside such a host still appears "active" in the
-      // raw time-window check (its own `data-start`/`data-end` cover the
-      // whole clip), so without this guard we would paint a full-bleed
-      // replacement frame over a sibling host that *is* visible.
+      // Walk ancestors looking for a host that the page has hidden. The
+      // runtime hides `[data-composition-src]` and `[data-start]` hosts that
+      // fall outside their time window; a nested `<video data-start>` inside
+      // such a host still appears "active" in the raw time-window check (its
+      // own `data-start`/`data-end` cover the whole clip), so without this
+      // guard we would paint a full-bleed replacement frame over a sibling
+      // host that *is* visible.
+      //
+      // `display: none` is always a skip signal — a `display: none` ancestor
+      // takes its whole subtree out of layout, and a child `<img>` cannot
+      // escape that. `visibility: hidden`, by contrast, is escapable: a
+      // descendant with `visibility: visible` overrides an ancestor's
+      // `visibility: hidden` per the CSS spec, and the replacement `<img>`
+      // intentionally sets `visibility: visible`. We therefore only treat
+      // `visibility: hidden` as a skip signal on sub-composition hosts
+      // (`[data-composition-src]` / `[data-composition-file]`), which is the
+      // scenario this guard exists for. Plain `[data-start]` containers may
+      // be hidden with `visibility: hidden` while still wanting their inner
+      // video's final-state frame to paint through (e.g. a GSAP timeline
+      // shorter than the host's authored data-duration, where the runtime
+      // truncates visibility but the replacement <img> must hold its last
+      // frame) — those must NOT be skipped here.
       const isVisualAncestorHidden = (el: HTMLElement): boolean => {
         let parent = el.parentElement;
         while (parent !== null && parent !== document.documentElement) {
           const computed = window.getComputedStyle(parent);
-          if (computed.display === "none" || computed.visibility === "hidden") return true;
+          if (computed.display === "none") return true;
+          if (
+            computed.visibility === "hidden" &&
+            (parent.hasAttribute("data-composition-src") ||
+              parent.hasAttribute("data-composition-file"))
+          ) {
+            return true;
+          }
           parent = parent.parentElement;
         }
         return false;
@@ -516,17 +537,22 @@ export async function syncVideoFrameVisibility(
   activeVideoIds: string[],
 ): Promise<void> {
   await page.evaluate((ids: string[]) => {
-    // Mirror the ancestor-visibility guard from `injectVideoFramesBatch`: a
-    // video whose host is `display:none` / `visibility:hidden` (e.g., a
-    // sub-composition that the runtime has marked out-of-window) must not
-    // have its replacement <img> reach `visibility:visible` here, otherwise
-    // it would paint through the hidden host onto whichever sibling host is
-    // currently visible.
+    // Mirror the ancestor-visibility guard from `injectVideoFramesBatch`.
+    // See that copy for the full rationale on why `visibility: hidden` is
+    // narrowed to sub-composition hosts only — keep these two functions in
+    // sync so the inactive-arm decision matches the inject-time decision.
     const isVisualAncestorHidden = (el: HTMLElement): boolean => {
       let parent = el.parentElement;
       while (parent !== null && parent !== document.documentElement) {
         const computed = window.getComputedStyle(parent);
-        if (computed.display === "none" || computed.visibility === "hidden") return true;
+        if (computed.display === "none") return true;
+        if (
+          computed.visibility === "hidden" &&
+          (parent.hasAttribute("data-composition-src") ||
+            parent.hasAttribute("data-composition-file"))
+        ) {
+          return true;
+        }
         parent = parent.parentElement;
       }
       return false;
