Repository navigation
fix: preserve page scrolling over popups - #4353
StekPerepolnen merged 8 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The scroll-container helper needs a fallback for the app’s custom fullscreen boundary.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates popup portals to preserve page scrolling while maintaining popup styling and fullscreen behavior.
Changes:
- Detects nearest scrollable ancestors for popup portals.
- Preserves Gravity typography and caller-provided styles.
- Adds PDisk/VDisk popup scrolling coverage.
File summaries
| File | Summary |
|---|---|
tests/suites/storage/vdiskColoring.test.ts |
Adds popup scrolling E2E coverage. |
src/components/HoverPopup/HoverPopup.tsx |
Applies scroll-container and font styling. |
src/components/HoverPopup/getPopupScrollContainer.ts |
Finds popup scroll boundaries. |
src/components/ContentWithPopup/ContentWithPopup.tsx |
Reuses shared portal placement and styling. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
App fullscreen overlays may allow popups to escape to document.body; the fullscreen fallback should be addressed.
Review details
Suppressed comments (1)
src/components/HoverPopup/getPopupScrollContainer.ts:11
- The app's fullscreen mode is not the browser Fullscreen API:
Fullscreenmoves its portal under the.ydb-fullscreen_fullscreenoverlay, sodocument.fullscreenElementremains unset. If that overlay's.ydb-fullscreen__contentis not vertically overflowing, this loop falls through toundefinedand portals the popup todocument.body, allowing it to escape the app fullscreen boundary. Treat the app fullscreen overlay as a fallback boundary as well as the native fullscreen element.
parent === doc.fullscreenElement ||
(/auto|scroll/.test(doc.defaultView?.getComputedStyle(parent).overflowY ?? '') &&
parent.scrollHeight > parent.clientHeight)
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Raubzeug
left a comment
There was a problem hiding this comment.
Found one clipping edge case in ContentWithPopup; details inline. The three focused Jest cases and four disk-popup E2E cases passed (Chromium and WebKit, retries disabled).
Wheel scrolling over a popup should continue scrolling the surrounding page. Place the portal in the nearest scrollable ancestor of its anchor so the browser handles wheel scrolling naturally, whether that ancestor contains a table or other page content. Document scrolling retains the default portal; native fullscreen and the app’s active
.ydb-fullscreen_fullscreenoverlay stay within their boundaries, including when the content has no vertical overflow.The shared
HoverPopupandContentWithPopupuse one small DOM helper. Both popup components use fixed positioning so constrained scroll containers do not clip their content. ExplicitContentWithPopupcontainers and positioning strategies still take precedence. Popup dimensions and overflow are unchanged. Portals explicitly retain the Gravity body font instead of inheriting the embedded page’s Rubik font; caller-provided floating styles still take precedence. No table context, height calculations, or internal popup scrollbars are introduced.One new E2E test covers PDisk and VDisk popup scrolling using existing fixtures. A focused Jest file covers document-default and fullscreen container selection, plus ContentWithPopup rendering through the selected or explicitly overridden portal. No new fixture files.
Validation:
CI regression follow-up:
Safe to merge; the remaining test-coverage concern is non-blocking.
Fix with agent prompt
Summary
HoverPopupandContentWithPopup.ContentWithPopupportal paths have no retained regression coverage.Reviews (1) · Last reviewed commit: "fix: preserve popup typography inside pa..."
Custom fullscreen follow-up: one production condition recognizes the active app fullscreen boundary even without overflowing content. Test files are unchanged. Typecheck, lint, all 1930 unit tests, and both builds passed. Existing Linux popup checks: 7 passed initially; one timed out waiting for storage/groups before opening the popup and passed in a separate rerun with retries disabled.