Merge pull request #311 from Coffey-Labs/theme-nested-light-panels

Neutralise light panels nested inside dark painted cards

web/src/lib/__tests__/html.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { LIGHT_SURFACE_LUMINANCE, htmlDeclaresColors, markKeptSurfaces, relativeLuminance, sanitizeEditorHtml, sanitizeEmailHtml } from "../html";
+import { EMAIL_BASE_CSS, LIGHT_SURFACE_LUMINANCE, htmlDeclaresColors, markKeptSurfaces, relativeLuminance, sanitizeEditorHtml, sanitizeEmailHtml } from "../html";
 
 describe("sanitizeEmailHtml", () => {
   it("removes scripts and event handlers", () => {
@@ -96,6 +96,24 @@ describe("markKeptSurfaces", () => {
     return d;
   };
 
+  /*
+   * Marking is only half of it — the other half is the rule in EMAIL_BASE_CSS
+   * that reads the marks, and #310 was a bug in that half rather than in the
+   * marking. So these assert what the reader actually sees: does the
+   * neutraliser hit this element? The selector is lifted out of the stylesheet
+   * rather than copied, so a test cannot quietly drift from the rule it checks.
+   */
+  const NEUTRALISER = (() => {
+    const m = EMAIL_BASE_CSS.match(
+      /\.ihm-email-root\.forced\s+(\*:not\([^{]*?)\s*\{\s*color: inherit/,
+    );
+    if (!m) throw new Error("could not find the neutraliser rule in EMAIL_BASE_CSS");
+    return m[1]!.trim();
+  })();
+
+  /** True when the theme is forced onto this element rather than leaving it alone. */
+  const neutralised = (el: Element) => el.matches(NEUTRALISER);
+
   it("keeps a coloured button and drops the white sheet around it", () => {
     // The shape reported in #290: a Shopify/Klaviyo template whose outer 600px
     // wrapper carries bgcolor="#ffffff" and whose CTA carries bgcolor="#1155CC".
@@ -103,9 +121,87 @@ describe("markKeptSurfaces", () => {
     expect(markKeptSurfaces(d)).toBe(1);
     expect(d.querySelector("table")!.hasAttribute("data-ihm-keep")).toBe(false);
     expect(d.querySelector("td")!.hasAttribute("data-ihm-keep")).toBe(true);
-    // The label is not marked itself; the CSS keeps it because it is inside
-    // something that is, which is what stops white-on-blue turning unreadable.
+    // The label is not a painted surface itself. It is marked as sitting on
+    // one, which is what stops white-on-blue turning unreadable.
     expect(d.querySelector("a")!.hasAttribute("data-ihm-keep")).toBe(false);
+    expect(d.querySelector("a")!.hasAttribute("data-ihm-in-keep")).toBe(true);
+  });
+
+  it("neutralises a light panel nested inside a dark painted card", () => {
+    // The shape reported in #310: a dark Klaviyo campaign whose 600px cards
+    // are dark enough to be marked, with light content tables inside them.
+    // Those tables used to inherit the card's exemption and render as beige
+    // sheets in an otherwise themed message.
+    const d = frag(
+      '<div style="background-color:#e7e5e2">' +
+        '<div style="background-color:#2b2b2b">' +
+          '<table style="background-color:#e7e5e2"><tr><td>copy</td></tr></table>' +
+        '</div>' +
+      '</div>',
+    );
+    expect(markKeptSurfaces(d)).toBe(1);
+
+    const divs = Array.from(d.querySelectorAll("div"));
+    const surround = divs[0]!;
+    const card = divs[1]!;
+    const nested = d.querySelector("table")!;
+
+    // The page surround is a sheet and always was.
+    expect(surround.hasAttribute("data-ihm-keep")).toBe(false);
+    // The card is paint and stays paint.
+    expect(card.hasAttribute("data-ihm-keep")).toBe(true);
+    // The fix, stated the way the reader experiences it: the nested sheet is
+    // themed, and so is the copy inside it. Before #310 both were exempt for
+    // being descendants of the card.
+    expect(neutralised(nested)).toBe(true);
+    expect(neutralised(d.querySelector("td")!)).toBe(true);
+    // The card itself is still left alone, and the page surround still goes.
+    expect(neutralised(card)).toBe(false);
+    expect(neutralised(surround)).toBe(true);
+  });
+
+  it("still keeps a button that sits inside a nested light panel", () => {
+    // Paint resumes below a sheet, however deep it is: the fix must not cost
+    // a call to action its label just because a sheet came between it and the
+    // card it is on.
+    const d = frag(
+      '<div style="background-color:#2b2b2b">' +
+        '<table style="background-color:#ffffff"><tr>' +
+          '<td bgcolor="#1155CC"><a style="color:#FFFFFF">Buy</a></td>' +
+        '</tr></table>' +
+      '</div>',
+    );
+    expect(markKeptSurfaces(d)).toBe(2);
+    expect(neutralised(d.querySelector("table")!)).toBe(true);
+    expect(neutralised(d.querySelector("td")!)).toBe(false);
+    // The label keeps its white, which is the thing #294 bought and this must
+    // not spend.
+    expect(neutralised(d.querySelector("a")!)).toBe(false);
+  });
+
+  it("leaves no light panel exempt across the whole reported specimen", () => {
+    // #310 as reported: a dark campaign with no bgcolor attributes, 21 light
+    // panels, 14 of them nested inside dark 600px cards. Those fourteen were
+    // the ones rendering as beige sheets.
+    let cards = "";
+    for (let i = 0; i < 7; i++) {
+      cards +=
+        '<div style="background-color:#2b2b2b">' +
+        '<table style="background-color:#e7e5e2"><tr><td>copy</td></tr></table>' +
+        '<table style="background-color:#e7e5e2"><tr><td>more</td></tr></table>' +
+        "</div>";
+    }
+    let loose = "";
+    for (let i = 0; i < 7; i++) {
+      loose += '<table style="background-color:#e7e5e2"><tr><td>loose</td></tr></table>';
+    }
+    const d = frag('<div style="background-color:#e7e5e2">' + cards + loose + "</div>");
+
+    const panels = Array.from(d.querySelectorAll<HTMLElement>("table"));
+    expect(panels.length).toBe(21);
+
+    expect(markKeptSurfaces(d)).toBe(7);
+    expect(panels.filter((p) => !neutralised(p))).toHaveLength(0);
   });
 
   it("reads an inline background as well as the attribute", () => {

web/src/lib/html.ts

@@ -194,13 +194,14 @@ export const EMAIL_BASE_CSS = `
 
 /* "Even mail that styles itself" — the second, opt-in switch, applied on top of
    .themed. Everything the sender coloured is neutralised except the surfaces
-   marked by markKeptSurfaces() and their contents, so a white wrapper table
+   marked by markKeptSurfaces() and what it marked as sitting on them, so a
+   white wrapper table
    stops being a bright card while a blue button keeps its white label. The
    sender's markup is untouched; this is all cascade, so the switch is
    reversible and print still pins the tokens to ink on white. */
 .ihm-email-root.forced { color: var(--fg, #1f2937) !important; background: var(--bg-elev, #fff) !important; }
-.ihm-email-root.forced *:not([data-ihm-keep]):not([data-ihm-keep] *) { color: inherit !important; background-color: transparent !important; }
-.ihm-email-root.forced a:not([data-ihm-keep]):not([data-ihm-keep] *) { color: var(--link, #0f766e) !important; }
+.ihm-email-root.forced *:not([data-ihm-keep]):not([data-ihm-in-keep]) { color: inherit !important; background-color: transparent !important; }
+.ihm-email-root.forced a:not([data-ihm-keep]):not([data-ihm-in-keep]) { color: var(--link, #0f766e) !important; }
 `;
 
 /**
@@ -275,6 +276,13 @@ export function relativeLuminance(color: string): number | null {
  */
 export const LIGHT_SURFACE_LUMINANCE = 0.5;
 
+/** The background an element declares itself, or null if it declares none we can read. */
+function declaredLuminance(el: HTMLElement): number | null {
+  const declared = el.getAttribute("bgcolor") ?? el.style?.backgroundColor ?? "";
+  if (!declared) return null;
+  return relativeLuminance(declared);
+}
+
 /**
  * Mark the surfaces that must survive being themed, and count them.
  *
@@ -285,23 +293,60 @@ export const LIGHT_SURFACE_LUMINANCE = 0.5;
  * a **painted surface** — a button, a banner — which is kept whole so its
  * label stays legible on it.
  *
- * Only the second is marked, with `data-ihm-keep`, and one CSS rule in
- * EMAIL_BASE_CSS neutralises everything that is not marked or inside something
- * marked. Nothing the sender wrote is removed, so turning the switch off puts
- * the message back exactly as it was — and a colour that arrived from a
- * `<style>` block rather than an attribute is covered too, which is most of
- * them in modern templates.
+ * Two attributes come out of this. `data-ihm-keep` is a painted surface, which
+ * keeps its own colours. `data-ihm-in-keep` is an element sitting on one with
+ * no background of its own, whose colour is left alone so a white label on a
+ * blue button stays readable. One rule in EMAIL_BASE_CSS neutralises
+ * everything else.
+ *
+ * The distinction that matters is that being *inside* a painted surface is not
+ * inherited past a sheet. A light table nested in a dark 600px card is still a
+ * sheet and is still neutralised — that is issue #310, where a dark campaign
+ * rendered with beige cards inside it because the exemption used to be
+ * `[data-ihm-keep] *` in CSS and could not see the difference. Paint resumes
+ * below it: a dark button inside that nested table is kept as usual.
+ *
+ * Nothing the sender wrote is removed, so turning the switch off puts the
+ * message back exactly as it was — and a colour that arrived from a `<style>`
+ * block rather than an attribute is covered too, which is most of them in
+ * modern templates.
  */
 export function markKeptSurfaces(root: ParentNode): number {
   let kept = 0;
-  for (const el of Array.from(root.querySelectorAll<HTMLElement>("*"))) {
-    const declared = el.getAttribute("bgcolor") ?? el.style?.backgroundColor ?? "";
-    if (!declared) continue;
-    const lum = relativeLuminance(declared);
-    if (lum === null || lum >= LIGHT_SURFACE_LUMINANCE) continue;
-    el.setAttribute("data-ihm-keep", "");
-    kept++;
+
+  // An explicit stack rather than recursion: this walks untrusted mail, and
+  // deeply nested tables are exactly what old newsletter HTML is made of.
+  const stack: Array<{ el: HTMLElement; onPaint: boolean }> = [];
+  const push = (parent: ParentNode, onPaint: boolean) => {
+    for (const child of Array.from(parent.children)) {
+      stack.push({ el: child as HTMLElement, onPaint });
+    }
+  };
+
+  push(root, false);
+
+  while (stack.length) {
+    const { el, onPaint } = stack.pop()!;
+    const lum = declaredLuminance(el);
+    let childrenOnPaint = onPaint;
+
+    if (lum !== null && lum < LIGHT_SURFACE_LUMINANCE) {
+      // Painted: keep it whole, and anything on it inherits that protection.
+      el.setAttribute("data-ihm-keep", "");
+      kept++;
+      childrenOnPaint = true;
+    } else if (lum !== null) {
+      // A sheet, wherever it sits. Left unmarked so it neutralises, and it
+      // ends the protection rather than passing it on.
+      childrenOnPaint = false;
+    } else if (onPaint) {
+      // No background of its own, sitting on paint: leave its colour alone.
+      el.setAttribute("data-ihm-in-keep", "");
+    }
+
+    push(el, childrenOnPaint);
   }
+
   return kept;
 }