fix(export): keep the caption background plate through the native bridge - #188
Merged
Merged
Conversation
Issue #178 — captions appear in the preview (DOM overlay + native compositor) but the exported video is missing the background plate, so depending on the recording's brightness the user reads it as 'no captions in the export'. Root cause: the caption inspector stores colour and opacity as two separate fields, and \captionBackgroundCss\ recombines them into a CSS string the editor overlay can render directly (e.g. \ gba(0, 0, 0, 0.55)\). The native side's \parse_hex\ only understood 3- or 6-char hex strings, so anything else — including the caption background — fell through to the \[0, 0, 0, 0]\ fallback (alpha 0, no plate). The text was drawn, the plate was not, and the contrast that made captions legible in the preview disappeared in the file. Fix: teach \parse_hex\ to accept the CSS surface area the JS bridge already produces — \ gba(...)\, \ gb(...)\, and \ ransparent\ — alongside the existing hex format. The fallback to None (and from there the caller's \[0,0,0,0]\) is preserved for everything that isn't a recognised colour, so a regression in any of the existing annotation colours still surfaces as a missing element rather than a wrong one. This also unblocks the gradient stop path, which has been sending the same \ gba(...)\ strings through \parse_hex\ for the same reason — they silently fell back to the colour preset. Not the issue's headline, but the same fix. The JS contract is now pinned by a test in \sceneDescription.test.ts\: the caption background leaves the bridge as \ gba(0, 0, 0, 0.55)\ exactly, and the text colour remains the ColorField hex (\#ffffff\). A future rewrite of either side that drops the alpha will fail this test instead of silently regressing the user-visible output.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ollow CSS arity Review follow-up on the caption-plate fix. `strip_color_fn` sliced `s[..name.len()]` without checking the UTF-8 char boundary, so any colour string whose byte 3 or 4 lands mid-character aborted the process instead of returning None. `parseWallpaper` hands every `#`-prefixed string straight through to `SceneBackground::Color`, so `#ab€cd` was enough to take out the render loop — and a panic crossing the N-API bridge is exactly what this parser's None-then-caller-fallback contract exists to avoid. Taking the tail with `get` first proves the boundary, which makes the head slice safe by construction and makes the old length guard redundant. The hex path had the same latent bug independently (`h[i..=i]` / `h[0..2]` on a body of exactly 3 or 6 bytes, e.g. `éa` or `€€`); an `is_ascii` guard closes it before any slicing happens. Both predate the rgba work, which only widened the first one's reach. Also collapses the two component parsers into one. CSS Color 4 makes `rgb()` and `rgba()` synonyms, both taking 3 or 4 components, so rejecting `rgba(0, 0, 0)` was non-standard — and it failed the same silent way #178 did, by falling through to the caller's alpha-0 fallback and painting no plate at all. Tests: 83/83 in the compositor lib (81 before, +2 here).
The preview showed every caption twice, most visibly at widths where the two copies wrapped differently — one broke to a second line, the other did not. `PreviewCanvas` hosts the native D3D canvas as "the sole pixel source" (its own header comment) with the DOM overlays kept interactive-only. `CaptionLayer` was not interactive: `aria-hidden`, `pointerEvents: none`, and a visible `<span>` painting the cue text and its background plate. Meanwhile the native compositor already draws the same cue, because captions are emitted into the scene as ordinary text annotations. Two painters, two line-breakers — CSS `word-break`/`pre-wrap` against DirectWrite laying out into `box_px` — so the same string wrapped at two different points. The component's own comment explains how it got here: it "mirrors annotationRenderer.renderText's box model ... so what the preview shows is what the export writes". That mirror was right when the preview was DOM-drawn and the exporter was a separate renderer; it became a duplicate once the native compositor started drawing the preview too. Deleted rather than gated: there is no non-native preview path left to fall back to (`VITE_NATIVE_COMPOSITOR` is read nowhere in the tree, and `NativeCompositorOverlay` mounts unconditionally). Dropping the DOM copy also makes preview and export agree by construction instead of by two implementations of the same box model. `useCaptions` stays — `CaptionsPane` still uses it. Pre-existing, not introduced by the plate fix: the native side always drew the caption text, it just drew it without a plate, so the duplicate read as a faint ghost. Restoring the plate made both copies solid and the doubling obvious. Tests: 96 files / 1061 pass, tsc clean, biome clean.
The caption doc still described a DOM preview painter alongside the native exporter, and linked to `CaptionLayer.tsx`, which no longer exists — the docs check failed on the dead link. Rewritten around what is actually there: one path, cues -> synthetic text regions -> annotation plumbing -> native compositor, which draws preview and export alike. That is a stronger version of the property the old text was reaching for: preview and export cannot drift because they are the same renderer, not two box models kept in sync by hand. Kept a short note on why the DOM layer existed and why it went, so the next reader does not re-add a "preview overlay" to fix a perceived gap. Also recorded that `captionBackgroundCss` emits `rgba(...)`, which is what forces the native colour parser to accept CSS colours rather than hex only. `docs:check` OK (22 files).
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
The caption inspector stores colour and opacity as two separate fields, and \captionBackgroundCss\ recombines them into a CSS string the editor overlay renders directly (e.g.
gba(0, 0, 0, 0.55)). The native compositor's \parse_hex\ only understood 3- or 6-char hex strings, so anything else fell through to the [0, 0, 0, 0]\ fallback — alpha 0, no plate. The text was drawn, the plate was not, and the contrast that made captions legible in the preview disappeared in the file. Depending on the recording's brightness, the user reads it as 'no captions in the export'.
Fix: teach \parse_hex\ to accept the CSS surface area the JS bridge already produces —
gba(...),
gb(...), and \ ransparent\ — alongside the existing hex format. The \None-then-caller-fallback path is preserved, so a regression in any existing annotation colour still surfaces as a missing element rather than a wrong one. The gradient stop path had the same latent issue (it sends
gba(...)\ from the parser upstream) and gets fixed by the same change.
Related issue
Fixes #178
Type of change
Release impact
Desktop impact
Testing
pm run test\ (jsdom unit suite): 1145/1145 pass.
px tsc --noEmit: clean.
pm run lint: no new warnings (7 pre-existing, unrelated).
pm run format: no changes needed.
A real Windows smoke test (record a clip, transcribe, enable captions, export MP4, scrub the file) is the manual gate the user-facing side still needs — the unit tests prove the native parser is no longer dropping the alpha, but the visual confirmation has to happen on a real D3D path.