Conversation
The overlay-host launcher/manager column sat at left:14 bottom:14 — the exact coords of the canvas's native zoom/fit/dark-mode control column (TakeoffCanvas 5813) — so ✎ Notes / ⚙ Plugins (and the Notes panel + crash notice at left:14 bottom:60) covered the dark-mode toggle and zoom buttons. Shift all plugin UI to left:58, clear of the 34px-wide native column: launcher/manager (host), the crash notice (error boundary), and the reference Notes panel. Pure layout; no behavior change.
Plugins no longer self-position their overlays. All plugin UI now lives in ONE host-positioned column (left:58, clear of the canvas's native zoom/dark-mode controls); the overlay renders as RELATIVE content into a host-sized, scroll-capped panel slot, so a plugin can't land its UI over app controls. The manager and an open overlay share that slot and are MUTUALLY EXCLUSIVE, so they can no longer cover each other (the overlap seen when both were open). - PluginOverlayHost: single flex column; panel slot (overlay|manager) above the launchers; openOverlay/toggleManager enforce mutual exclusion; PANEL_WIDTH is host-owned; overlay wrapped in a width-bounded maxHeight:60vh scroll box. - takeoff-notes reference plugin: drop self-positioning; render relative content. - PluginErrorBoundary: notice is relative content in the host slot, not absolute. - PLUGIN-AUTHORING: document that overlays render relative content; the host owns placement; don't self-position (position:fixed/absolute escapes the safe zone). #167 frozen core untouched; canvas untouched. Gate green (861 tests).
…, honest docs Apply adversarial-review findings (no blockers; all minor): - Browser MINOR: a tall relative overlay grew to maxHeight:60vh (viewport) and could reach the TOP toolbar. Bound both the overlay slot and the manager to the canvas STAGE via maxHeight:calc(100% - 28px) + overflowY:auto — matching the native panels (TakeoffCanvas:5925) — so a tall overlay scrolls within the stage instead of spilling over the top chrome. The zoom/dark-mode column was never at risk (relative content grows rightward, away from it) — confirmed by review. - Static MINOR: the slot's overflow clipped the inner panel's drop-shadow. Put the shadow on the host wrapper (its own box-shadow isn't self-clipped) and drop the now-redundant shadow from the reference panel + crash notice. - Soften the overstated 'can't cover the canvas's own controls' comment + doc: the honest guarantee is 'clear of the zoom/dark-mode column, bounded to the stage'; position:fixed still escapes (documented convention, not a sandbox). Gate green (861 tests). Core + canvas untouched.
…t cap works Re-verification showed the calc(100% - 28px) overlay cap was inert: PluginOverlay Host was mounted at the 100vh root (sibling of the stage), whose auto-height host column gave the percentage nothing definite to bind to — a tall overlay grew unbounded and covered the top toolbar. Move the mount INSIDE the canvas stage (TakeoffCanvas:5340 — flex:1, position:relative, overflow:hidden, below the chrome), beside the native zoom column. Now the stage is the definite-height positioning ancestor: the cap clamps, a tall overlay scrolls within the stage, and overflow:hidden clips any spillover — so plugin UI can't reach the top toolbar. Launchers stay visually put (stage bottom = viewport bottom). Pure relocation of one element; behavior otherwise unchanged. Gate green (861).
Round-2 fix was still inert: the slot's maxHeight:calc(100%) bound to the auto- height host COLUMN (its containing block), not the stage — so it clamped nothing and a tall overlay was clipped (data loss, no scrollbar) by the stage's overflow:hidden. Correct mechanism: put the stage-relative cap on the host COLUMN (a direct child of the definite-height stage, like the native panel at :5925), then flex-distribute inside it — launchers flexShrink:0, the panel flex:0 1 auto + minHeight:0 + overflowY:auto. A tall overlay now shrinks to the space above the launchers and SCROLLS; a short one keeps its natural size; neither reaches the top toolbar. Gate green (861).
Owner
Author
Evidence — host-owned placement + region protection
|
There was a problem hiding this comment.
Pull request overview
This PR implements host-owned plugin UI placement (region protection) for the in-canvas plugin overlay system. It moves overlay mounting into the canvas stage so the host can enforce a bottom-left safe column that stays clear of the native zoom/dark-mode controls and height-caps/scrolls within the stage.
Changes:
- Relocates
<PluginOverlayHost />into the canvas stage to make stage-relativemaxHeightclamping effective. - Updates
PluginOverlayHostto fully own placement: a single bottom-left column (left:58) with a fixed-width panel slot, internal scrolling, and mutual exclusivity between manager vs. open overlay. - Updates the reference overlay and the error boundary to render relative content (no self-positioning), and documents the new placement contract for plugin authors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/TakeoffCanvas.jsx | Mounts PluginOverlayHost inside the stage container so height caps bind to a definite-height ancestor. |
| web/src/components/PluginOverlayHost.jsx | Implements Option A placement: safe column, stage-relative maxHeight + scroll, and manager/overlay mutual exclusivity. |
| web/src/features/takeoff-notes/plugin.jsx | Converts overlay root UI to relative, host-sized rendering (no absolute positioning). |
| web/src/components/PluginErrorBoundary.jsx | Converts the crash notice UI to relative, host-sized rendering (shadow now provided by host wrapper). |
| docs/plugin-foundation/PLUGIN-AUTHORING.md | Documents “host owns placement” expectations and the non-sandbox limitation. |
| CHANGELOG.md | Records the behavior change and rationale for #178. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Problem
Plugins self-positioned their own overlays. Because each plugin decided where to draw, plugin UI drifted over the canvas's native controls — the zoom and dark-mode column in the bottom-left — and the plugin manager and an open plugin overlay could overlap and cover each other. Placement was effectively unowned, so nothing guaranteed the native controls stayed reachable.
Option A — the host owns placement
The host now owns plugin-UI placement:
Bounded to the canvas stage
The overlay is bounded to the canvas stage, not the viewport:
maxHeight: calc(100% - 28px)cap, plusflex-shrinkand internal scroll.The result: even a very tall overlay (tested at 2000px) scrolls within the stage instead of growing up over the top toolbar. The cap only works because it binds to a definite-height ancestor — see the canvas edit below.
Honest limitation
This is a convention, not a sandbox. A plugin that uses
position: fixedcan still escape the slot and draw anywhere. The host owns the provided slot; it cannot forcibly contain a plugin that opts out of the layout. This is documented as expected behavior, not a security boundary.The one canvas edit
<PluginOverlayHost/>was relocated into the stage so that themaxHeight: calc(100% - 28px)cap binds to a definite-height ancestor (the stage) rather than resolving against an indefinite one and silently doing nothing. This is the only canvas change. The #167 frozen core is untouched.Verification
This took three empirical browser rounds. The first two height-cap approaches looked correct on paper but were inert in the browser — the cap resolved against an indefinite-height ancestor and had no effect. Both were caught in-browser before the working flexbox-based fix landed. The gate is green (861 tests). A CHANGELOG entry is included.
This PR is stacked on the plugin-disable work — base is
feat/plugin-disable, branch-only. The diff here is just the placement / region-protection work.Closes #178
Part of #179 (epic) · design in #166