feat(super-editor): ui.viewport scroll-container access + sidebar-toggle invalidation (SD-3324, SD-3326)#3581
Conversation
Exposes the editor's resolved scroll container — the scrollable ancestor of the painted host (occasionally the host itself) — via a public `scrollContainer` getter on PresentationEditor and `ui.viewport.getScrollContainer()`. Overlay consumers need the element SuperDoc actually scrolls to attach scroll listeners and measure against; getHost() returns the painted host, which is usually not the scroller. Returns HTMLElement | null, with null meaning the document/window scrolls (fall back to window). Also corrects the visibleHost JSDoc that wrongly described it as the scroll container.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca767aaa4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * @returns The scroll container element, or `null` when the window scrolls | ||
| */ | ||
| get scrollContainer(): HTMLElement | null { | ||
| return this.#scrollContainer instanceof HTMLElement ? this.#scrollContainer : null; |
There was a problem hiding this comment.
When SuperDoc is mounted in a different browsing context, such as an iframe, the resolved scroll container is an element from that iframe's realm, so instanceof HTMLElement against the parent/global constructor is false. In that case ui.viewport.getScrollContainer() returns null, causing overlay consumers to fall back to the wrong window and miss scroll events/measurements even though an element scroller exists. Use the container's ownerDocument.defaultView?.HTMLElement (or a structural element check) so cross-realm HTML elements are returned.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 4 files
Linked issue analysis
Linked issue: SD-3324: Add ui.viewport.getScrollContainer for the editor's resolved scroll element
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Expose ui.viewport.getScrollContainer() that returns the resolved scroll container (HTMLElement) or null when the document/window scrolls. | The PR adds a PresentationEditor.scrollContainer getter, exposes getScrollContainer() on the UI handle, and updates types to include the new API. Tests verify the call returns the mounted scroller, null when window scrolls, and null when no editor is mounted. |
| ✅ | Bridge superdoc 'sidebar-toggle' into ui.viewport.observe as a geometry invalidation using reason 'layout' so observers re-query cached rects. | The PR wires a sidebar-toggle handler to scheduleGeometry('layout') and registers/unregisters the listener with superdoc events. A unit test fires 'sidebar-toggle' and expects an observe notification with reason 'layout'. |
| ✅ | Clarify visibleHost JSDoc to note it is the painted host and not necessarily the scroll container. | The PresentationEditor.visibleHost JSDoc was updated to explicitly say the painted host is not necessarily the scroll container and points consumers to use scrollContainer for scroll listeners. |
Tip: cubic could auto-approve low-risk PRs like this, if it thinks it's safe to merge. Learn more
Re-trigger cubic
…D-3326) ui.viewport.observe now fires when the comments rail opens or closes. The rail toggle shifts document geometry without a guaranteed layout repaint, so overlay consumers' cached rects would silently go stale. Bridges the existing sidebar-toggle signal into a geometry invalidation, reusing the 'layout' reason (consumers only re-query, so no new public reason is added).
caio-pizzol
force-pushed
the
caio/viewport-scroll-container-and-sidebar-observe
branch
from
aca767a to
4578ed8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Two small additions to the
ui.viewportsurface for overlay consumers, one commit per work item.ui.viewport.getScrollContainer()(SD-3324) exposes the editor's resolved scroll container — the scrollable ancestor of the painted host (sometimes the host itself).getHost()returns the painted host, which is usually NOT the element that scrolls, so overlay code that attached scroll listeners there never fired. ReturnsHTMLElement | null;nullmeans the document/window scrolls (fall back towindow). Also fixes thevisibleHostJSDoc that wrongly described it as the scroll container.ui.viewport.observenow fires onsidebar-toggle(SD-3326). Toggling the comments rail shifts document geometry without a guaranteed layout repaint, so cached overlay rects went stale silently. This bridges the existingsidebar-togglesignal into a geometry invalidation, reusing thelayoutreason.Closes the IT-1109 and IT-1111 follow-ups.
Review:
getScrollContainer'sHTMLElement | nullcontract (null= window scrolls, documented), and reusing thelayoutobserve reason for the sidebar bridge rather than adding a new public reason.Verified:
vitest src/ui/viewport.test.ts→ 31 passed;check:public:superdoc→ 13/13 PASS; browser —getScrollContainerreturns the real scroller, distinct from the host.