Repository navigation
fix(web): fit the canvas to the host element, not the window - #2006
meanaverage (meanaverage) wants to merge 2 commits into
Conversation
GitHub Actions
Active
github-actions
Bot
added
risk/low
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
github-actions
Bot
added
risk/medium
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR 2006 fixes canvas sizing when the component is embedded smaller than the window: fitResize and realResize now measure to the host element's bottom-right corner (availableAreaCorner($host())) instead of the window corner, with a window fallback while the host has no layout size; fullResize intentionally still uses the window. The new availableArea.ts module is small and pure, with tests covering host-sized, unlayouted-host, and missing-host cases. Independent inspection of the component found no correctness defects: viewport-coordinate math between wrapper.getBoundingClientRect() and the host box is consistent, host-style helpers touch inner rather than the host box, and the zero-size fallback is sound. Both specialist candidates are valid low-severity maintainability tidy-ups and are accepted; one independent low-severity consistency observation about 'full' scaling is added.
- [code-compressor] getWindowSize() is now a pointless pass-through wrapper — low 🟡 — web-client/iron-remote-desktop/src/iron-remote-desktop.svelte
After this PR, getWindowSize() only forwards to windowCorner() and its single remaining caller is fullResize(). Inlining windowCorner() at that call site and deleting the wrapper removes a one-line indirection with no behavior change, since windowCorner's name already conveys intent. - [general] Full scaling still measures to the window, overflowing embedded hosts — low 🟡 — web-client/iron-remote-desktop/src/iron-remote-desktop.svelte
The PR fixes fit/real to stay within the host, but fullResize still sizes the wrapper to window.innerWidth/innerHeight. In an embedder pane smaller than the window, selecting 'full' can size the wrapper past the host edges — the same cropping problem this PR fixes for the other modes. Possibly intentional (fill the viewport), but inconsistent for embedded use and worth confirming or documenting.
| export function windowCorner(win: Window = window): Corner { | ||
| const docElem = win.document.documentElement; | ||
| const body = win.document.getElementsByTagName('body')[0]; | ||
| return { | ||
| x: win.innerWidth ?? docElem.clientWidth ?? body.clientWidth, | ||
| y: win.innerHeight ?? docElem.clientHeight ?? body.clientHeight, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Bottom-right corner, in viewport coordinates, of the area the component may draw in. | ||
| * | ||
| * The fit and real scalings measure from the component's wrapper to this corner. When the component is | ||
| * embedded in something smaller than the window (a split pane, a sidebar), that is the host element's box, | ||
| * not the window: measuring to the window's corner would size the canvas past the host's edges. A host | ||
| * without a size yet (not laid out) falls back to the window. | ||
| */ | ||
| export function availableAreaCorner(host: Element | null | undefined, win: Window = window): Corner { | ||
| if (host) { | ||
| const box = host.getBoundingClientRect(); | ||
| if (box.width > 0 && box.height > 0) { | ||
| return { x: box.right, y: box.bottom }; | ||
| } | ||
| } | ||
| return windowCorner(win); |
There was a problem hiding this comment.
[code-compressor] Speculative win parameter is never supplied by any caller — low 🟡 — windowCorner and availableAreaCorner accept `win: Window = window`, but no caller (component or tests) passes a custom window. Removing the parameter and using the global directly drops dead plumbing with identical behavior; the only tradeoff is slightly less convenient window mocking, which existing tests do not use.
There was a problem hiding this comment.
Done in 61dbce8: dropped the win parameter, and inlined getWindowSize() into fullResize(), its only remaining caller.
On full scaling still measuring to the window: that is deliberate, full fills the viewport, as before. This PR only changes fit and real, the two that are meant to stay inside the component.
GitHub Actions
Active
github-actions
Bot
added
risk/medium
The fit and real scalings measured the space available from the wrapper to the window's bottom-right corner, which assumes the component reaches the window's edges. Embedded in anything smaller (a split pane, a sidebar), the canvas was sized past the host's edges and got cropped. Measure to the host element's box instead (falling back to the window while the host has no size). The full scaling still uses the window.
… wrapper No caller passes a window to windowCorner or availableAreaCorner, and getWindowSize only forwarded to windowCorner for the full scaling.
61dbce8 to
d4edfa6
Compare
GitHub Actions
Active
github-actions
Bot
added
automation-failed
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
github-actions
Bot
added
the
risk/low
github-actions
Bot
removed
risk/unknown
There was a problem hiding this comment.
PR #2006 fixes fit/real scaling to measure available space from the host element's bottom-right corner instead of the window, so the canvas fits embedded contexts (split panes, sidebars). Verified independently in pr-head: availableAreaCorner($host()) preserves old behavior whenever the host is absent or has zero size (identical windowCorner() fallback chain), fullResize deliberately stays on the window viewport, and viewport-coordinate arithmetic (box.right/bottom minus wrapper x/y) is consistent. The initial resize runs 150ms after visibility, after layout, so the zero-size fallback is a safety net. No correctness or API issues found; the two code-compressor candidates are valid, low-severity, behavior-preserving style simplifications and are published unchanged.
- [code-compressor] getAvailableCorner() is a one-line indirection; inline availableAreaCorner($host()) at its two callers — low 🟡 — web-client/iron-remote-desktop/src/iron-remote-desktop.svelte
The wrapper adds a named function plus a comment whose content (fit/real measure within the host element) is already stated in availableAreaCorner's doc-comment, to adapt a single call. With only two call sites (fitResize and realResize), replacing getAvailableCorner() with availableAreaCorner($host()) removes four lines and one hop while preserving behavior exactly; $host() makes the host argument self-explanatory at the call site. Verified behavior-preserving in pr-head. Tradeoff: call sites get slightly longer and lose the local comment, but the lib documentation carries the same rationale.
| export function windowCorner(): Corner { | ||
| const docElem = document.documentElement; | ||
| const body = document.getElementsByTagName('body')[0]; | ||
| return { | ||
| x: window.innerWidth ?? docElem.clientWidth ?? body.clientWidth, | ||
| y: window.innerHeight ?? docElem.clientHeight ?? body.clientHeight, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[code-compressor] Reach the body via document.body instead of getElementsByTagName('body')[0] — low 🟡 — The new windowCorner() relocates the old lookup verbatim, including document.getElementsByTagName('body')[0]. document.body returns the same element with one call and no indexing, and has identical null behavior (if body is missing, the property access on the fallback chain's last operand throws the same way in both forms; the docElem ??-fallbacks are unaffected). Verified in pr-head. An optional simplification of moved code, not a defect; keeping it verbatim is also defensible for a pure relocation.
github-actions
Bot
added
ai-reviewed/2
This branch was successfully deployed
2 participants
Problem
fitResizeandrealResizecompute the space available aswindow.innerWidth/innerHeightminus the wrapper's position, which assumes the component reaches the window's bottom-right corner. When<iron-remote-desktop>is embedded in anything smaller (a split pane, a sidebar, a tab next to other UI), the canvas is sized past the host's edges and gets cropped.Change
src/lib/availableArea.ts:availableAreaCorner(host)returns the host element's bottom-right corner, falling back to the window while the host has no size yet (not laid out).$host()). Full scaling still uses the window.Testing
src/lib/availableArea.test.ts(3 tests);npm test(21 passing),npm run check(0 errors),npm run lint,npm run build.Side note for embedders: the component refits on window
resizeonly, so a host that changes size on its own can callsetScale(ScreenScale.Fit)from aResizeObserver.Prepared with AI assistance; I reviewed the change and ran the tests above.