-
Notifications
You must be signed in to change notification settings - Fork 230
chore: testing overlay fixes #5625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dd0c119
e87567c
87868a8
74c9065
b6dd797
fd7dd4b
d8eaafa
07569c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ import { | |
|
||
import styles from './overlay.css.js'; | ||
import { FocusTrap } from 'focus-trap'; | ||
import '@spectrum-web-components/underlay/sp-underlay.js'; | ||
|
||
const browserSupportsPopover = 'showPopover' in document.createElement('div'); | ||
|
||
|
@@ -420,9 +421,9 @@ export class Overlay extends ComputedOverlayBase { | |
* Determines the value for the popover attribute based on the overlay type. | ||
* | ||
* @private | ||
* @returns {'auto' | 'manual' | undefined} The popover value or undefined if not applicable. | ||
* @returns {'auto' | 'manual' | 'hint' | undefined} The popover value or undefined if not applicable. | ||
*/ | ||
private get popoverValue(): 'auto' | 'manual' | undefined { | ||
private get popoverValue(): 'auto' | 'manual' | 'hint' | undefined { | ||
const hasPopoverAttribute = 'popover' in this; | ||
|
||
if (!hasPopoverAttribute) { | ||
|
@@ -431,10 +432,8 @@ export class Overlay extends ComputedOverlayBase { | |
|
||
switch (this.type) { | ||
case 'modal': | ||
return 'auto'; | ||
case 'page': | ||
return 'manual'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this changed, but github diff doesn't show it very well |
||
case 'hint': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed hint to be hint, because why not? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can't do this because previously multiple hints could be open by using hint it closes the previous open hint |
||
case 'page': | ||
return 'manual'; | ||
default: | ||
return this.type; | ||
|
@@ -542,6 +541,9 @@ export class Overlay extends ComputedOverlayBase { | |
const focusTrap = await import('focus-trap'); | ||
this._focusTrap = focusTrap.createFocusTrap(this.dialogEl, { | ||
initialFocus: focusEl || undefined, | ||
allowOutsideClick: (event) => { | ||
return !event.isTrusted; | ||
}, | ||
tabbableOptions: { | ||
getShadowRoot: true, | ||
}, | ||
|
@@ -554,7 +556,10 @@ export class Overlay extends ComputedOverlayBase { | |
escapeDeactivates: false, | ||
}); | ||
|
||
if (this.type === 'modal' || this.type === 'page') { | ||
if ( | ||
(this.type === 'modal' || this.type === 'page') && | ||
this.receivesFocus !== 'false' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this condition to the initialFocus instead, because we want to have focus trap anyway |
||
) { | ||
this._focusTrap.activate(); | ||
} | ||
} | ||
|
@@ -1141,6 +1146,19 @@ export class Overlay extends ComputedOverlayBase { | |
*/ | ||
public override render(): TemplateResult { | ||
return html` | ||
${this.type === 'modal' || this.type === 'page' | ||
? html` | ||
<popover> | ||
<sp-underlay | ||
?open=${this.open} | ||
@close=${() => { | ||
this.open = false; | ||
}} | ||
style="--spectrum-underlay-background-color: transparent" | ||
></sp-underlay> | ||
</popover> | ||
` | ||
: ''} | ||
${this.renderPopover()} | ||
<slot name="longpress-describedby-descriptor"></slot> | ||
`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
/* eslint-disable no-console */ | ||
/** | ||
* Copyright 2025 Adobe. All rights reserved. | ||
* This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
|
@@ -27,6 +28,10 @@ class OverlayStack { | |
|
||
stack: Overlay[] = []; | ||
|
||
private originalBodyOverflow = ''; | ||
|
||
private bodyScrollBlocked = false; | ||
|
||
constructor() { | ||
this.bindEvents(); | ||
} | ||
|
@@ -79,6 +84,25 @@ class OverlayStack { | |
this.stack.splice(overlayIndex, 1); | ||
} | ||
overlay.open = false; | ||
|
||
this.manageBodyScroll(); | ||
} | ||
|
||
/** | ||
* Manage body scroll blocking based on modal/page overlays | ||
*/ | ||
private manageBodyScroll(): void { | ||
const shouldBlock = this.stack.some( | ||
(overlay) => overlay.type === 'modal' || overlay.type === 'page' | ||
); | ||
if (shouldBlock && !this.bodyScrollBlocked) { | ||
this.originalBodyOverflow = document.body.style.overflow || ''; | ||
document.body.style.overflow = 'hidden'; | ||
this.bodyScrollBlocked = true; | ||
} else if (!shouldBlock && this.bodyScrollBlocked) { | ||
document.body.style.overflow = this.originalBodyOverflow; | ||
this.bodyScrollBlocked = false; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -151,14 +175,14 @@ class OverlayStack { | |
|
||
private handleKeydown = (event: KeyboardEvent): void => { | ||
if (event.code !== 'Escape') return; | ||
if (event.defaultPrevented) return; // Don't handle if already handled | ||
if (!this.stack.length) return; | ||
const last = this.stack[this.stack.length - 1]; | ||
if (last?.type === 'page') { | ||
event.preventDefault(); | ||
return; | ||
} | ||
if (last?.type === 'manual') { | ||
// Manual overlays should close on "Escape" key, but not when losing focus or interacting with other parts of the page. | ||
if (last?.type === 'manual' || last?.type === 'modal') { | ||
this.closeOverlay(last); | ||
return; | ||
} | ||
|
@@ -213,11 +237,29 @@ class OverlayStack { | |
const path = event.composedPath(); | ||
this.stack.forEach((overlayEl) => { | ||
const inPath = path.find((el) => el === overlayEl); | ||
|
||
// Check if the trigger element is inside this overlay | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dubious if needed |
||
const triggerInOverlay = | ||
overlay.triggerElement && | ||
overlay.triggerElement instanceof HTMLElement && | ||
overlayEl.contains && | ||
overlayEl.contains(overlay.triggerElement); | ||
console.log( | ||
'overlayEl.type:', | ||
overlayEl.type, | ||
'triggerInOverlay:', | ||
triggerInOverlay, | ||
'inPath:', | ||
!!inPath | ||
); | ||
|
||
if ( | ||
!inPath && | ||
!triggerInOverlay && | ||
overlayEl.type !== 'manual' && | ||
overlayEl.type !== 'modal' | ||
) { | ||
console.log('Closing overlay:', overlayEl); | ||
this.closeOverlay(overlayEl); | ||
} | ||
}); | ||
|
@@ -248,6 +290,7 @@ class OverlayStack { | |
overlay.addEventListener('beforetoggle', this.handleBeforetoggle, { | ||
once: true, | ||
}); | ||
this.manageBodyScroll(); | ||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected object keys to be in specified order. 'types' should be before 'dependencies'.
reviewdog suggestion error
GitHub comment range and suggestion line range must be same. L177-L177 v.s. L153-L177