From 78642bc6bb6c218ee8fb1a379db78263b5b95b8c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 19:32:11 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A9=B9=20fix:=20revert=20PPTX=20height-fi?= =?UTF-8?q?ll=20changes;=20lock=20DOCX=20CDN=20to=20light=20scheme?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes for separate manual e2e regressions on PR #12934. **1. PPTX black screen (single slide rendering as solid black).** The previous fix removed `body { display: flex }` thinking that was the sole cause, but the regression persisted. Bisecting against the last known-good commit (4e2d538b0, width-fit only), the actual culprit is the COMBINATION of: - `min-height: 100vh` on html/body - `availableHeight()` reading viewport-derived dimensions - `Math.min(sw, sh)` height-aware scale pptx-preview's CSS injection step interacts unpredictably with these. Reverting to width-only `scaleFor(nativeW)` and dropping the viewport min-height restores reliable rendering. Vertical empty space below short decks now shows the body's bg color (`var(--bg)`) which still matches the panel theme — that's an acceptable trade-off vs. the black-screen regression. Negative assertions added: `Math.min(sw, sh)`, `availableHeight`, `min-height: 100vh`, `body { display: flex }` must NOT appear in the bootstrap. So a future "let's fill height" rewrite has to demonstrate it doesn't break pptx-preview before it can land. **2. DOCX body text rendering as faint / translucent grey.** docx-preview emits page-style rendering with white pages and the docs native text colors. The CDN doc declared `color-scheme: light dark`, so on OS dark mode the iframes inheritable `--fg` resolved to `#e5e7eb` (light grey). docx-preview body text (no explicit color in the source DOCX) inherited that light-grey on the white page bg → barely-visible "translucent" rendering. Fix: declare `color-scheme: light` only in `buildDocxCdnDocument`, drop the dark-mode `@media` override. docx-preview is a light-mode- only renderer; matching that produces correct contrast regardless of OS theme. The mammoth-only `wrapAsDocument` path is unaffected — it owns its own bg + text colors and continues to respect the users OS scheme. New regression test pins the lock: CDN doc must contain `color-scheme: light`, must NOT contain `color-scheme: light dark`, must NOT contain `prefers-color-scheme: dark`. --- packages/api/src/files/documents/html.spec.ts | 67 ++++++++++------- packages/api/src/files/documents/html.ts | 71 +++++++++---------- 2 files changed, 77 insertions(+), 61 deletions(-) diff --git a/packages/api/src/files/documents/html.spec.ts b/packages/api/src/files/documents/html.spec.ts index 5ce71fc6fb..681be2bf4d 100644 --- a/packages/api/src/files/documents/html.spec.ts +++ b/packages/api/src/files/documents/html.spec.ts @@ -189,6 +189,26 @@ describe('Office HTML producers', () => { _internal.OFFICE_HTML_OUTPUT_CAP, ); }); + + test('locks color-scheme to light so OS dark-mode does not fade body text', async () => { + /* docx-preview emits page-style rendering (white pages with + * the doc's native colors). When the iframe declares both + * light and dark color-schemes AND the OS is in dark mode, + * browsers inherit dark-mode colors for unset text properties + * — docx-preview's body text (no explicit color) inherited + * our `--fg: #e5e7eb` on the white page bg, rendering as + * barely-visible light grey. Manual e2e regression on PR + * #12934. The CDN doc must declare ONLY light scheme; the + * mammoth-only fallback (`wrapAsDocument`) is unaffected + * because it owns its own bg + text colors. */ + const html = await _internal.wordDocToHtmlViaCdn( + readFixture('sample.docx'), + '

fallback

', + ); + expect(html).toMatch(/color-scheme:\s*light\b/); + expect(html).not.toMatch(/color-scheme:\s*light\s+dark/); + expect(html).not.toMatch(/prefers-color-scheme:\s*dark/); + }); }); describe('OFFICE_PREVIEW_DISABLE_CDN escape hatch', () => { @@ -477,34 +497,33 @@ describe('Office HTML producers', () => { const html = await _internal.pptxToHtmlViaCdn(pptx); /* The wrapper class used by the CSS rules. */ expect(html).toContain('lc-slide-wrap'); - /* The wrap function + the per-slide scale function. The scale - * uses each slide's actual rendered native width AND height - * so panels of any aspect fill correctly — no upscale cap - * means we never leave whitespace on either axis. */ + /* The wrap function + the per-slide scale function. The + * scale uses each slide's actual rendered native width (not + * a constant) so panels wider than 960px still fill — no + * upscale cap means we never leave whitespace on the sides + * of the panel. */ expect(html).toContain('wrapSlides'); expect(html).toContain('scaleFor'); expect(html).toContain('availableWidth'); - expect(html).toContain('availableHeight'); - /* The scale is `min(width-fit, height-fit)` so each slide is - * the largest it can be in the iframe viewport without - * overflowing either dimension — manual e2e feedback on - * PR #12934. */ - expect(html).toContain('Math.min(sw, sh)'); - /* Negative assertion: the previous version capped the scale - * at 1.0 with `Math.min(1, ...)`, which left whitespace on - * panels wider than 960px. The new code must not reintroduce - * that cap. */ + /* The width-only scale formula. Negative assertion below + * forbids re-introducing the height-aware variant — that + * caused pptx-preview to render slides as solid-black + * rectangles (manual e2e regression on PR #12934). */ + expect(html).toContain('availableWidth() / (nativeW || SLIDE_W)'); + /* Negative assertions for two failed iterations: + * - `Math.min(1, ...)` upscale cap left whitespace on + * panels wider than 960px. + * - `Math.min(sw, sh)` height-aware scale + viewport- + * derived height calculations interacted with pptx- + * preview's internal layout and produced black slides. + * - `body { display: flex }` and `min-height: 100vh` on + * html/body also broke pptx-preview's render path. + * Vertical empty space below short decks is acceptable; the + * iframe bg shows through and matches the panel theme. */ expect(html).not.toMatch(/Math\.min\(\s*1\s*,/); - /* Body fills the iframe viewport vertically and inherits the - * dark bg so a short deck (single slide) never reveals a - * "white below" gap below the slides. */ - expect(html).toContain('min-height: 100vh'); - /* Negative assertion: turning the body itself into a flex - * container caused pptx-preview's slides to render as black - * rectangles (manual e2e regression on PR #12934). The - * library's internal layout assumes block flow on its - * ancestor elements; the bg-fill effect must come from - * `min-height` alone, not from `body { display: flex }`. */ + expect(html).not.toContain('Math.min(sw, sh)'); + expect(html).not.toContain('availableHeight'); + expect(html).not.toContain('min-height: 100vh'); expect(html).not.toMatch(/body\s*\{[^}]*display:\s*flex/); /* Container is hidden during render and revealed by the * `finalize` step so the unscaled flash never reaches the diff --git a/packages/api/src/files/documents/html.ts b/packages/api/src/files/documents/html.ts index 538108d363..baa723fd17 100644 --- a/packages/api/src/files/documents/html.ts +++ b/packages/api/src/files/documents/html.ts @@ -405,8 +405,17 @@ function buildDocxCdnDocument(base64: string, mammothFallbackHtml: string): stri Preview @@ -1073,14 +1083,6 @@ function buildPptxCdnDocument(base64: string): string { :root { color-scheme: light dark; --bg: #ffffff; --fg: #1f2937; --muted: #6b7280; } @media (prefers-color-scheme: dark) { :root { --bg: #1a1a2e; --fg: #e5e7eb; --muted: #9ca3af; } } html, body { margin: 0; padding: 0; background: var(--bg); color: var(--fg); font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Helvetica, Arial, sans-serif; } -/* Body fills the iframe viewport vertically and inherits the dark bg - * so even when total slide content is shorter than the panel (e.g. a - * single-slide deck) the artifact card has no visible "white below" - * gap. We do NOT make body a flex container — pptx-preview's - * internal layout assumes block flow; turning body into a flex - * column made slide content render but stay invisible (manual e2e - * regression on PR #12934, "black screen for slides"). */ -html, body { min-height: 100vh; } #lc-render { padding: 16px; box-sizing: border-box; display: flex; flex-direction: column; align-items: center; gap: 16px; } /* Each rendered slide is wrapped post-hoc by the bootstrap script in * an .lc-slide-wrap div with explicit width/height set inline. The @@ -1176,30 +1178,25 @@ html, body { min-height: 100vh; } var w = (container.clientWidth || window.innerWidth) - 32; return w > 0 ? w : 600; } - function availableHeight() { - /* Viewport height inside the iframe minus the same 16px top+ - * bottom padding so a single-slide deck can grow up to the full - * panel height without spilling. */ - var h = (window.innerHeight || document.documentElement.clientHeight || 0) - 32; - return h > 0 ? h : 400; - } - /* Per-slide scale: the largest factor that lets the slide fit in - * the iframe viewport without overflowing either dimension. No - * upscale cap — pptx-preview rasterizes against the init canvas - * (960×540), text/charts are vector or canvas-rendered, so they - * stay legible when scaled up. + /* Width-only scale: each slide divides the panel width by its + * actual rendered native width, so we always fill the panel + * horizontally. No upscale cap — pptx-preview rasterizes against + * the init canvas (960×540), text/charts are vector or canvas- + * rendered, so they stay legible when scaled up. * - * Why the height-aware version (vs. width-only): a tall artifact - * panel with a short deck would otherwise leave the slide at - * panel_w × (panel_w·9/16) and waste the rest of the viewport - * vertically. Min-of-fits makes the slide grow until ONE - * dimension is the constraint, exactly the contain-fit pattern - * the rest of the iframe expects. */ - function scaleFor(nativeW, nativeH) { - var sw = availableWidth() / (nativeW || SLIDE_W); - var sh = availableHeight() / (nativeH || SLIDE_H); - return Math.min(sw, sh); + * NOTE: a previous iteration tried min(width-fit, height-fit) to + * also fill the panel vertically. That broke pptx-previews + * rendering (slides came out as solid black rectangles). The + * librarys internal layout interacts unpredictably with min-height + * 100vh on the document root and viewport-derived height + * calculations during its CSS injection step. Sticking to + * width-only keeps rendering reliable; vertical empty space below + * a short deck shows the bodys bg color, which inherits from + * var(--bg) and matches the panel theme — manual e2e feedback on + * PR #12934. */ + function scaleFor(nativeW) { + return availableWidth() / (nativeW || SLIDE_W); } /* Wrap each rendered slide and apply the scale. Called ONCE, @@ -1223,7 +1220,7 @@ html, body { min-height: 100vh; } slide.dataset.lcNativeW = String(nativeW); slide.dataset.lcNativeH = String(nativeH); } - var scale = scaleFor(nativeW, nativeH); + var scale = scaleFor(nativeW); var wrap = document.createElement('div'); wrap.className = 'lc-slide-wrap'; wrap.style.width = (nativeW * scale) + 'px'; @@ -1246,7 +1243,7 @@ html, body { min-height: 100vh; } if (!inner || !inner.dataset) { continue; } var nativeW = parseFloat(inner.dataset.lcNativeW) || SLIDE_W; var nativeH = parseFloat(inner.dataset.lcNativeH) || SLIDE_H; - var scale = scaleFor(nativeW, nativeH); + var scale = scaleFor(nativeW); wrap.style.width = (nativeW * scale) + 'px'; wrap.style.height = (nativeH * scale) + 'px'; inner.style.transform = 'scale(' + scale + ')';