Neutralise light panels nested inside dark painted cards
Closes #310. A dark campaign rendered with beige cards inside it. markKeptSurfaces marks any element whose declared background is below the luminance threshold, with no area cap, so a 600px layout card is marked exactly like a button. The CSS then exempted the marked element and its whole subtree via [data-ihm-keep] *, so a light table nested in that card was never touched. In the reported specimen 14 of 21 light panels survived. The rule now is that being inside a painted surface is not inherited past a sheet. The walk tracks that state and emits a second mark, data-ihm-in-keep, for elements sitting on paint with no background of their own; the CSS exempts those explicitly instead of exempting every descendant. A nested light sheet ends the protection, and paint resumes below it, so a button inside such a sheet is still kept whole. The alternatives in the report were not taken. Dropping the descendant half of the selector outright puts back what #294 fixed: a nested label on a coloured cell loses its colour. An area threshold is a magic number that misfires on both a legitimate hero banner and a small dark panel with a light chip in it. The tests assert against the neutraliser selector lifted out of EMAIL_BASE_CSS rather than against the marks. The first draft of them checked which attributes were set and passed against the unfixed code, which proved nothing: the bug was in the rule that reads the marks, not in the marking. All four fail without this change.
This commit is contained in:
@@ -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", () => {
|
||||
|
||||
+61
-16
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user