Repository navigation
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.
Greptile SummaryAdds an in-app popup substitute for desktop printing.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/studio-host/shims/openhwp-popup.js | Adds a same-origin iframe-backed Window facade for popup-dependent print previews while preserving native handling for cross-origin URLs and navigation targets. |
| scripts/build-studio.ts | Copies the popup shim into the built studio and injects its classic script tag, failing the build when the HTML anchor is absent. |
| apps/studio-host/README.md | Documents why the desktop host requires the additive popup shim and where it enters the build. |
Sequence Diagram
sequenceDiagram
participant Studio
participant Shim as window.open shim
participant Frame as Preview iframe
participant System as System print dialog
Studio->>Shim: window.open(blank or same-origin URL)
Shim->>Frame: Create full-window iframe overlay
Shim-->>Studio: Return Window-like facade
Studio->>Frame: Build or load print preview
Studio->>Frame: print()
Frame->>System: Open print dialog
Studio->>Shim: close()
Shim->>Frame: Remove overlay
Reviews (5): Last reviewed commit: "chore: apply AI code review suggestions" | 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 |
Applied: match the </head> anchor case-insensitively and tolerate trailing whitespace, since HTML end tags allow both. Verified against </head>, </HEAD>, "</head >" and "</Head\t>" — all inject — while a document with no end tag still trips the guard and fails the build. Not applied: moving the frame's Escape listener from the document to the window. The premise does not hold in CEF — measured, neither survives a document.open()/write(): both listener counts stay flat afterwards. What already handles it is the re-bind on the frame's load event, which document.close() fires. Comment updated to say so, since the old wording credited only navigation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a window.open() shim (openhwp-popup.js) to enable printing functionality in the CEF desktop host, which does not natively support popup windows. The shim overlays a same-origin iframe on the window and returns a proxy facade. The build script is updated to inject this shim into the built index.html. Feedback suggests enhancing the proxy facade to prevent bypasses via self-referential properties like window, self, or frames, and hardening the URL resolver to prevent loading non-standard protocols (e.g., javascript:) when the origin is null.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a window.open() shim (openhwp-popup.js) for the CEF-based desktop host to support the print preview feature via a same-origin iframe overlay, along with build script integration and documentation updates. The review feedback suggests improving the shim's robustness by supporting direct onload property assignments on the proxy facade, restricting interception to only blank or empty targets (preventing incorrect interception of _self or _parent), and refactoring the shim function to comply with the 50-line limit.
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a window.open() shim (openhwp-popup.js) for the CEF-based desktop host to enable printing functionality via an iframe overlay, and updates the build script to inject this shim into the built index.html. The review feedback recommends two improvements to the shim: dynamically retrieving the contentWindow in the event listener proxy to prevent stale references after navigation, and respecting event.defaultPrevented in the Escape key listener to avoid conflicts with internal elements.
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. Supports blank and same-origin popups for v0.7.x–v0.8.x compatibility.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 forwardsloadin a way that survives navigation; cross-origin popups are unchanged; Esc closes.scripts/build-studio.tsto copy the shim and inject a non-module<script>before the deferred bundle; the</head>anchor match is case-insensitive and tolerates trailing whitespace; still throws if injection fails.apps/studio-host/README.mdto document the shim and why it exists.Written for commit 8d0673c. Summary will update on new commits.