fix(studio): make 파일 → 인쇄 work in the desktop app - #23
Conversation
Printing reported "팝업이 차단되었습니다. 팝업 허용 후 다시 시도해주세요."
and nothing happened.
Upstream's file:print builds its preview into a popup:
const printWin = window.open('', '_blank');
if (!printWin) { alert('팝업이 차단되었습니다. ...'); return; }
but deno desktop's CEF backend never creates popup windows. Probed against
the real host: window.open('', '_blank') returns null both with and without
a user gesture, so it is not a popup blocker a gesture can satisfy, and
Deno.BrowserWindow exposes no popup hook for the shell to opt in with.
Printing itself is fine — the system print dialog opens and PrintCore is
loaded — so the only missing piece is something for window.open() to return.
Add apps/studio-host/shims/openhwp-popup.js: window.open() for a blank
target now returns a same-origin about:blank iframe overlaid on the window,
which supplies everything the caller uses — .document to build into,
.print() to print just that document, .close() to dismiss. Popups with a
real URL keep the host's behaviour so link targets never change silently.
Upstream's print stylesheet already suits this: @media screen styles the
document as a centred preview with a fixed toolbar, and @media print hides
the toolbar. So the overlay reproduces the intended preview rather than
approximating it.
This is additive — the studio is still built unmodified and
config/rhwp-studio-overrides.json stays empty. build-studio.ts copies the
shim into dist/ and injects the script tag, throwing if </head> is absent
rather than silently shipping a bundle that shows the popup alert again.
Verified in a real CEF build serving the actual studio bundle:
window.open -> Window, overlay present, document writable, print is a
function, close() removes the overlay; and the preview renders — overlay
1100x772 filling the window, toolbar 36px with both buttons, A4 pages at
794x1123 on white with the SVG painted. The injection guard was confirmed
to throw (deno exits 1) when the anchor is missing.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a window.open() shim (openhwp-popup.js) for the CEF host to support printing by overlaying an iframe, and updates the build script to inject this shim into the built index.html. The review feedback recommends attaching the keydown listener to the iframe's window object instead of its document to prevent it from being cleared during document writes, using Object.defineProperty to safely shadow win.close without throwing a TypeError in strict mode, and utilizing a case-insensitive regular expression for the HTML head injection to increase build resilience.
| try { | ||
| win.document.addEventListener("keydown", onKeydown, true); | ||
| } catch { | ||
| // A cross-origin document would throw; blank popups never are, so this | ||
| // is only belt-and-braces. | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Attach keydown listener to the window object instead of the document
Problem: The caller of window.open() typically writes content into the new document using document.write() or document.open(). Doing so clears the document and discards any event listeners attached directly to win.document. As a result, the Escape key listener on the iframe's document will be lost, and the user won't be able to dismiss the print preview using the Escape key.
Rationale: Event listeners attached to the window object (win) persist across document.write() and document.open() calls, whereas listeners on win.document are destroyed.
Suggestion: Attach the keydown event listener to win instead of win.document.
| try { | |
| win.document.addEventListener("keydown", onKeydown, true); | |
| } catch { | |
| // A cross-origin document would throw; blank popups never are, so this | |
| // is only belt-and-braces. | |
| } | |
| try { | |
| win.addEventListener("keydown", onKeydown, true); | |
| } catch { | |
| // A cross-origin document would throw; blank popups never are, so this | |
| // is only belt-and-braces. | |
| } |
| // Shadow close() on the pseudo-window. Window.prototype.close() would be a | ||
| // no-op for an iframe, leaving the overlay stuck over the editor. | ||
| try { | ||
| win.close = dismiss; | ||
| } catch { | ||
| // If the assignment is refused the overlay is still dismissable with | ||
| // Escape, so this stays non-fatal. | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Use Object.defineProperty to shadow win.close
Problem: Direct assignment to win.close in strict mode will throw a TypeError in environments where window.close is a read-only property on the Window prototype, causing the override to fail and leaving the print preview overlay stuck when the user clicks the close button.
Rationale: In strict mode, assigning to a read-only property throws an error. Using Object.defineProperty safely shadows the property on the instance.
Suggestion: Use Object.defineProperty to define the close method on the iframe's window object.
// Shadow close() on the pseudo-window. Window.prototype.close() would be a
// no-op for an iframe, leaving the overlay stuck over the editor.
try {
Object.defineProperty(win, "close", {
value: dismiss,
writable: true,
configurable: true,
});
} catch {
// If the assignment is refused the overlay is still dismissable with
// Escape, so this stays non-fatal.
}| await Deno.copyFile(SHIM_SRC, `${OUT}/${SHIM_NAME}`); | ||
| const htmlPath = `${OUT}/index.html`; | ||
| const htmlOrig = await Deno.readTextFile(htmlPath); | ||
| const htmlPatched = htmlOrig.replace("</head>", ` ${SHIM_TAG}\n</head>`); |
There was a problem hiding this comment.
[MEDIUM] Use case-insensitive regex for HTML head injection
Problem: If the upstream index.html uses a different casing for the closing head tag (e.g., </HEAD> or </head with extra whitespace), the string replacement will fail, causing the build script to throw an error and halt.
Rationale: Using a case-insensitive regular expression makes the build process more resilient to minor upstream formatting changes.
Suggestion: Use a case-insensitive regular expression /(<\/head>)/i for the replacement.
const htmlPatched = htmlOrig.replace(/(<\/head>)/i, " " + SHIM_TAG + "\n$1");
Greptile SummaryAdds an in-app print-preview substitute for unsupported CEF popup windows.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/studio-host/shims/openhwp-popup.js | Adds the iframe-backed popup façade used to host and dismiss Studio print previews in CEF. |
| scripts/build-studio.ts | Copies the popup shim into the output and injects it before the deferred Studio bundle, with an explicit failure guard. |
| apps/studio-host/README.md | Documents the desktop popup limitation and additive host shim. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Studio calls window.open] --> B{Blank or same-origin URL?}
B -->|No| C[Delegate to native window.open]
B -->|Yes| D[Create full-window iframe]
D --> E[Return popup façade]
E --> F[Studio builds print preview]
F --> G[Print iframe document]
E --> H[Close or Escape]
H --> I[Remove iframe overlay]
Reviews (2): Last reviewed commit: "fix(studio): shim same-origin popups too..." | Re-trigger Greptile
Upstream v0.8.x reworked printing. file:print no longer opens a blank
popup and builds into it — it opens print.html:
const previewWindow = hostWindow.open(surfaceUrl, '_blank');
if (!previewWindow) return Promise.reject(new PrintPreviewBlockedError());
That is a real same-origin URL, so a blank-only shim would pass it
straight through to the host, get null back, and silently restore the
exact bug this fixes the moment the pin moves to 0.8.x.
Handle same-origin URLs as well, still passing cross-origin popups to the
host so external link targets never change silently.
A navigated frame needs more than the previous approach gave it. Both
shortcuts that worked for a blank popup break once the frame loads a URL:
an own `close` property set on the window is discarded along with that
window, and a `load` listener registered on it never fires, because the
document that finishes loading belongs to the *next* window. So return a
façade instead — reads forward to whichever window the frame currently
holds, and `load` is served by the frame element, which survives
navigation.
Verified in a real CEF build against a faithful replica of v0.8.2's
createPrintPreviewSurface: the surface resolves (the load event does reach
the caller through the façade), document is the loaded page, origin and
href match, print is a function, fonts.ready + requestAnimationFrame work
(the waitForPrintSurfaceReady contract), the closed flag transitions, and
close() removes the overlay. The v0.7.19 blank path still passes
unchanged, the 닫기 button dismisses through the façade, and a
cross-origin popup still returns null with no overlay created.
Widened to same-origin popups (1ac16f7)Prompted by upstream v0.8.2. That release is a hotfix for the browser extension's print (
At v0.8.x that popup targets Why a façade was neededBoth shortcuts that worked for a blank popup break once the frame navigates:
So VerificationRun in a real CEF build, against a faithful replica of v0.8.2's
And the existing behaviour is unchanged:
Moving to v0.8.2 is tracked separately, per the plan — it needs the matching |
|
1 participant
The bug
파일 → 인쇄 shows "팝업이 차단되었습니다. 팝업 허용 후 다시 시도해주세요." and nothing prints.
Cause
Upstream's
file:printbuilds its preview into a popup window(
rhwp-studio/src/command/commands/file.ts:493):deno desktop's CEF backend never creates popup windows. Probed against a real CEF build:window.open('', '_blank')— no gesturenull (BLOCKED)window.open('', '_blank')— from a clicknull (BLOCKED)So it is not a popup blocker that a user gesture can satisfy, and
Deno.BrowserWindowexposes no popup/new-window hook for the shell to opt in with (its events are
close,navigation,resize,menuclick).Printing itself works. With a print call in flight the app's window list contains a real
system dialog and
PrintCore.frameworkis loaded:print()blocks only because that dialog is modal. The single missing piece is something forwindow.open()to return.Fix
apps/studio-host/shims/openhwp-popup.js—window.open()for a blank target returns asame-origin
about:blankiframe overlaid on the window, which provides exactly what the calleruses:
.documentto build into,.print()to print just that document,.close()to dismiss(shadowed, since
Window.close()is a no-op for an iframe and would strand the overlay).Popups with a real URL keep the host's behaviour, so link targets never change silently. Escape
also dismisses, since the overlay has no window chrome.
Upstream's print stylesheet is already written for this —
@media screenstyles the document asa centred preview with a fixed toolbar,
@media printhides the toolbar — so the overlayreproduces the intended preview rather than approximating it.
Additive, not an override. The studio is still built unmodified and
config/rhwp-studio-overrides.jsonstays empty.scripts/build-studio.ts(step 7) copies theshim into
dist/and injects a classic<script>tag, following upstream's owntheme-init.jspattern so it runs before the deferred module bundle.
Verification
Run in a real CEF build serving the actual studio bundle:
window.open('', '_blank')Window(wasnull)iframe.openhwp-popuppresent.documentwritabletypeof .printfunction.close()@media print { .print-bar { display: none } }denoexits 1deno fmt --check,deno lint,deno checkall clean.Note
The upstream popup approach is fragile in browsers too (a popup outside a user gesture is
blocked there as well), so this is worth reporting upstream — but the shim fixes the desktop app
without waiting on it.
Summary by cubic
Fixes 파일 → 인쇄 in the desktop app by shimming
window.openso the print preview renders in-app and printing works without popups. Now handles both blank and same-origin popups to stay compatible with upstream v0.8.x.apps/studio-host/shims/openhwp-popup.js: shimswindow.openfor blank and same-origin URLs, returning a full-window same-origin iframe overlay with a façade that exposes.document,.print(),.close(), and forwardsload; cross-origin popups are unchanged; Esc closes.scripts/build-studio.tsto copy the shim and inject a non-module<script>before the deferred bundle; throws if injection fails.apps/studio-host/README.mdto document the shim and why it exists.Written for commit 1ac16f7. Summary will update on new commits.