feat(lint): flag dir="rtl" on <html> as a confirmed silent render failure#1893
Draft
miguel-heygen wants to merge 1 commit into
Draft
feat(lint): flag dir="rtl" on <html> as a confirmed silent render failure#1893miguel-heygen wants to merge 1 commit into
miguel-heygen wants to merge 1 commit into
Conversation
…lure Two independent reports diagnosed the same exact bug: dir="rtl" (or any non-ltr value) on <html> renders correctly in preview/snapshot but produces a fully blank/black video from render, with no other lint/validate/inspect check catching it - output file size (far smaller than expected) was the only tell for both reporters. Both independently confirmed the same fix: drop dir from <html>, keep lang, and scope direction: rtl to individual text-containing elements via CSS instead. Could not empirically verify the render pipeline's own root cause in this session (headless Chrome screenshot capture is unreliable in this sandboxed environment - even a baseline, non-RTL capture timed out), so this ships the safe, already-confirmed advisory rather than guessing at a runtime fix. Both reporters explicitly asked for exactly this: "deserves a lint rule or render-time warning."
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two independent reports diagnosed the exact same bug:
dir="rtl"(or any non-ltrvalue) on<html>renders correctly in preview/snapshot but produces a fully blank/black video fromrender, with no other lint/validate/inspect check catching it — output file size (far smaller than expected) was the only tell for both reporters.Both independently confirmed the same fix: drop
dirfrom<html>, keeplang, and scopedirection: rtlto individual text-containing elements via CSS instead (text still bidi-shapes correctly through the browser's own algorithm).Investigation notes (please read before assuming this is "fixed")
This PR is advisory only — it does not fix the underlying render-pipeline bug, because I could not empirically verify it in this session.
My working hypothesis:
packages/engine/src/services/screenshotService.tsclips itsPage.captureScreenshotCDP calls to a fixed{x: 0, y: 0, width, height}region, assuming the composition's content sits at the document's top-left origin. RTL layout can affect scroll-origin/viewport semantics in ways that could shift the effective content position away from(0,0)for absolutely-positioned elements, which would explain why a fixed top-left clip captures blank space while an interactive tab (snapshot/preview) doesn't hit the same issue.I attempted to confirm this empirically with an isolated Puppeteer repro (bare RTL vs LTR page, same
Page.captureScreenshotcall as the engine) but headless Chrome CDP screenshot capture is unreliable in this sandboxed environment — even the LTR baseline call timed out onPage.captureScreenshot. I could not get a working comparison.Given two independent, already-self-verified reports but no way to confirm the exact mechanism myself, I judged it safer to ship the confirmed advisory (both reporters explicitly asked for this) than to guess at a runtime fix to
screenshotService.tsI couldn't test. Whoever picks up the actual render-pipeline fix should start withscreenshotService.ts's clip region and verify against a real RTL composition in an environment where headless Chrome screenshot capture works reliably.Fix (this PR)
New lint rule
html_dir_attribute_breaks_render(error severity, matching the severity of other silent-default-breaking checks in this file) flags<html dir="...">for any value other thanltr, with a fix hint naming the exact confirmed workaround.Test plan
bunx vitest run packages/lint/src/rules/composition.test.ts— 98 tests pass (5 new)bunx vitest run packages/lint/— full package, 314 tests pass, no regressionsdir="rtl", flags any non-ltrvalue, does not flagdir="ltr", does not flag a missingdirattribute, does not flag the documented fix (element-scopeddirection: rtlvia CSS instead of the<html>attribute)