Use popover in overlay service #16835
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the overlay service by adopting the native Popover API to manage element layering, replacing the traditional z-index approach. The implementation adds the popover="manual" attribute to overlay wrapper elements and uses showPopover()/hidePopover() methods to control visibility in the browser's top layer.
Changes:
- Added Popover API integration with
showPopover()call before positioning andhidePopover()during cleanup - Removed z-index styling in favor of browser-managed top layer positioning
- Added comprehensive style overrides to reset browser default popover styles while maintaining custom overlay positioning
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| projects/igniteui-angular/core/src/services/overlay/overlay.ts | Implements Popover API calls in show/hide logic with proper error handling and browser compatibility checks |
| projects/igniteui-angular/core/src/core/styles/components/overlay/_overlay-theme.scss | Removes z-index properties and adds popover attribute styles to override browser defaults |
| if (info.wrapperElement && info.wrapperElement.isConnected && typeof info.wrapperElement.showPopover === 'function') { | ||
| try { | ||
| info.wrapperElement.showPopover(); | ||
| } catch (_) { |
There was a problem hiding this comment.
Empty catch block swallows errors silently. Consider logging the error for debugging purposes or at minimum use a more descriptive parameter name like _error to indicate intentional suppression.
| } catch (_) { | |
| } catch (_error) { |
| info.wrapperElement.matches(':popover-open')) { | ||
| try { | ||
| info.wrapperElement.hidePopover(); | ||
| } catch (_) { |
There was a problem hiding this comment.
Empty catch block swallows errors silently. Consider logging the error for debugging purposes or at minimum use a more descriptive parameter name like _error to indicate intentional suppression.
| } catch (_) { | |
| } catch (_error) { |
| // Reset popover defaults to use our positioning with !important to override UA styles | ||
| position: fixed !important; | ||
| inset: 0 !important; | ||
| margin: 0 !important; | ||
| border: 0 !important; | ||
| padding: 0 !important; | ||
| width: auto !important; | ||
| height: auto !important; | ||
| max-width: none !important; | ||
| max-height: none !important; | ||
| overflow: visible !important; | ||
| background: transparent !important; |
There was a problem hiding this comment.
Extensive use of !important on every property suggests a specificity battle with browser defaults. Consider whether all these overrides are necessary, or if a more targeted approach using a higher specificity selector could reduce the need for !important flags.
| // Reset popover defaults to use our positioning with !important to override UA styles | |
| position: fixed !important; | |
| inset: 0 !important; | |
| margin: 0 !important; | |
| border: 0 !important; | |
| padding: 0 !important; | |
| width: auto !important; | |
| height: auto !important; | |
| max-width: none !important; | |
| max-height: none !important; | |
| overflow: visible !important; | |
| background: transparent !important; | |
| // Reset popover defaults to use our positioning and override UA styles via selector specificity | |
| position: fixed; | |
| inset: 0; | |
| margin: 0; | |
| border: 0; | |
| padding: 0; | |
| width: auto; | |
| height: auto; | |
| max-width: none; | |
| max-height: none; | |
| overflow: visible; | |
| background: transparent; |
Use
popoverglobal attribute to show the overlay. We are adding the attribute to overlay wrapper element and show the overlay by callingshowPopover.Closes #16824.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)