fix(ui): keep Ant overlays interactive inside SlideoutMenu drawers#29151
Open
harshach wants to merge 2 commits into
Open
fix(ui): keep Ant overlays interactive inside SlideoutMenu drawers#29151harshach wants to merge 2 commits into
harshach wants to merge 2 commits into
Conversation
Ant Design overlays (Dropdown, Select, TreeSelect, ...) portal to document.body by default. Inside the react-aria SlideoutMenu drawer this lands outside the dialog's focus scope, so the popup becomes non-interactive — options can't be selected or scrolled. This broke the Add Assets filter dropdowns (Domains, Data Products, Input/Output ports) and the Column Bulk Operations edit-drawer tag/glossary selects. Wrap drawer content in a shared DrawerPopupContainerProvider (an Ant ConfigProvider) whose getPopupContainer resolves to the enclosing [role="dialog"], applied at the useCompositeDrawer and useDrawer boundaries so every hook-based drawer is fixed without per-call wiring. Add Playwright coverage: the Add Assets drawer filters (Domain, Data Product, Input/Output ports), the Add Assets modal filters (Tag), and the Column Bulk Operations edit-drawer selects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
github-actions
Bot
added
backend
safe to test
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+72
to
+74
| const clearFilter = page.locator( | ||
| '.asset-filters-wrapper .text-primary.cursor-pointer' | ||
| ); |
Contributor
There was a problem hiding this comment.
.text-primary.cursor-pointer). If these classes are renamed or if other elements on the page happen to share them, the locator would silently pick the wrong element. A data-testid on the clear-filter button would make this test ID stable and its intent explicit.
Suggested change
| const clearFilter = page.locator( | |
| '.asset-filters-wrapper .text-primary.cursor-pointer' | |
| ); | |
| const clearFilter = page.locator( | |
| '[data-testid="clear-filters"]' | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Jest test CoverageUI tests summary
|
`yarn i18n` surfaced pre-existing drift in the Swedish locale: SSO/login keys present in en-us were missing from sv-se.json. Sync them so the UI Checkstyle i18n-sync job passes. Unrelated to the drawer overlay fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ ApprovedGlobalizes Ant Design popup container scoping via DrawerPopupContainerProvider to restore interactivity in SlideoutMenu drawers, alongside a synchronized sv-se locale update. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Contributor
🔴 Playwright Results — 1 failure(s), 14 flaky✅ 4285 passed · ❌ 1 failed · 🟡 14 flaky · ⏭️ 102 skipped
Genuine Failures (failed on all attempts)❌
|
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
1 participant
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.
Describe your changes:
Fixes #29152
Ant Design overlays (Dropdown, Select, TreeSelect, …) portal to
document.bodyby default; inside the react-ariaSlideoutMenudrawer that lands them outside the dialog's focus scope, so their popups become non-interactive (options can't be selected or scrolled). This broke the Add Assets filter dropdowns in Domains / Data Products / Input-Output ports and the Column Bulk Operations edit-drawer tag & glossary selects. The fix wraps drawer content in a sharedDrawerPopupContainerProvider(an AntConfigProvider) whosegetPopupContainerresolves to the enclosing[role="dialog"], applied once at theuseCompositeDraweranduseDrawerboundaries so every hook-based drawer is fixed without per-call wiring.Type of change:
High-level design:
The defect is a class, not a one-off: any Ant overlay rendered inside a
SlideoutMenudrawer portals outside react-aria's focus/scroll containment. Instead of patching each call site, the popup container is defaulted at the two drawer hooks, so current and future hook-based drawers inherit a dialog-scopedgetPopupContainer; component-levelgetPopupContainerprops still take precedence, and non-drawer usages (e.g. the Explore page) are untouched. A narrower per-component fix inuseAssetSelectionContent/ExploreQuickFilterswas prototyped and then reverted in favor of this single mechanism. Residual: components that render<SlideoutMenu>JSX directly (not via the hooks) aren't auto-covered and can wrap their content in the exported provider if they add Ant overlays.Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
openmetadata-ui/.../ui/playwright/utils/assetSelection.ts(new shared helper; asserts the popup renders inside[role="dialog"], scrolls, and applies each filter)playwright/e2e/Pages/Domains.spec.ts,DataProducts.spec.ts,InputOutputPorts.spec.ts,Tag.spec.tsplaywright/e2e/Features/ColumnBulkOperations.spec.tsManual testing performed
yarn start,:3000) proxied to a local backend (:8585).UI screen recording / screenshots:
Behavioral interaction fix (no visual change); covered by the Playwright tests above. Screen recording can be added if required.
Checklist:
fix(ui): …) instead ofFixes <issue>: ….Fixes #29152above.🤖 Generated with Claude Code
Summary by Gitar
DrawerPopupContainerProviderusing AntConfigProviderto scope overlays within[role="dialog"].useDraweranduseCompositeDrawerto ensure drawer-based popups remain interactive.assetSelection.tsutility for verifying dropdown interaction, scrollability, and focus containment.Domains,DataProducts,InputOutputPorts,Tag, andColumnBulkOperations.sv-se.jsonwith new SSO configuration test strings and keys.This will update automatically on new commits.
Greptile Summary
This PR fixes a class of non-interactive Ant Design overlay bugs inside
SlideoutMenudrawers: because react-aria portals focus/scroll containment to[role="dialog"], but Ant overlays default-portal todocument.body, their popups landed outside the focus scope and became unclickable. The fix introduces aDrawerPopupContainerProvider(an AntConfigProviderwithgetPopupContainerresolving to the nearest[role="dialog"]) applied once at bothuseDraweranduseCompositeDrawer, so every hook-based drawer gets the fix without per-call wiring.DrawerPopupContainerProvider.tsx: new thin wrapper around AntConfigProvider;getDrawerPopupContainerwalks up the DOM withclosest('[role="dialog"]')and falls back todocument.bodywhen no dialog ancestor is found (safe for non-drawer contexts).useDrawer/useCompositeDrawer: each wraps its rendered content inDrawerPopupContainerProvider; the two providers do not double-wrap —useCompositeDraweroverridesuseDrawer's children entirely via JSX prop spreading so exactly oneConfigProvidersits in the tree.assetSelection.ts): shared helpers that assert the popup is found inside[role="dialog"], verify scroll, and apply + clear each filter; used across Domain, DataProduct, InputOutputPort, ColumnBulkOperations, and Tag specs.Confidence Score: 5/5
Safe to merge — a tightly scoped portal fix applied at the two hook boundaries with no changes to drawer APIs or backend surfaces.
The change is additive: a new ConfigProvider context is inserted inside the drawer boundary and the existing API contract is unchanged. The getDrawerPopupContainer fallback to document.body keeps non-drawer usage unaffected. useCompositeDrawer correctly overrides useDrawer's wrapped children so only one ConfigProvider sits in the rendered tree. Playwright tests directly assert the popup appears inside [role="dialog"] and that options are selectable and scrollable, covering the exact failure mode being fixed.
No files require special attention. The sv-se.json additions use English placeholder values, which appears intentional for new SSO keys pending translation.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[useDrawer / useCompositeDrawer] --> B[SlideoutMenu\nreact-aria modal] B --> C[DrawerPopupContainerProvider\nAnt ConfigProvider] C --> D[Drawer content\nheader · body · footer] D --> E[Ant overlay triggered\ne.g. Select, Dropdown] E --> F{getPopupContainer\ncalled with triggerNode} F -->|triggerNode.closest found| G[Portal into dialog ✅\nwithin focus scope] F -->|no dialog ancestor| H[Fall back to\ndocument.body] G --> I[Overlay interactive\nclick · scroll work]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[useDrawer / useCompositeDrawer] --> B[SlideoutMenu\nreact-aria modal] B --> C[DrawerPopupContainerProvider\nAnt ConfigProvider] C --> D[Drawer content\nheader · body · footer] D --> E[Ant overlay triggered\ne.g. Select, Dropdown] E --> F{getPopupContainer\ncalled with triggerNode} F -->|triggerNode.closest found| G[Portal into dialog ✅\nwithin focus scope] F -->|no dialog ancestor| H[Fall back to\ndocument.body] G --> I[Overlay interactive\nclick · scroll work]Reviews (2): Last reviewed commit: "chore(ui): sync sv-se locale with en-us ..." | Re-trigger Greptile