-
Notifications
You must be signed in to change notification settings - Fork 233
fix(overlay): Programmatic clicks on elements outside of the overlay now registers #5670
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1fdf47b
fix(overlay): added allowOutsideClick property to dialog show for pag…
185906f
chore: added a new story to demonstrate this behaviour
dc2e19c
chore: added test
cdc864b
chore: updated readme
82653ad
chore: added changeset
9ce6b30
Merge branch 'main' into rajdeep/SWC-1018
Rajdeepc 424195d
chore: revert change to allow programmatic click hindering a11y contract
8ad244a
docs: modal type should not allow external clicks docs update
25937df
fix(overlay): allow-outside-click added as deprecation
df51141
fix(overlay): removed redundant docs
d922d70
Merge branch 'main' into rajdeep/SWC-1018
Rajdeepc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
--- | ||
'@spectrum-web-components/overlay': minor | ||
--- | ||
|
||
Added `allow-outside-click` property to `<sp-overlay>` with deprecation notice. This property allows clicks outside the overlay to close it, but is not recommended for accessibility reasons and will be removed in a future version. | ||
|
||
This property is being added as deprecated to support the fallback for `showModal()` which was removed as part of performance optimization. We will no longer support outside clicks for modal overlays as they violate accessibility guidelines. | ||
|
||
**Breaking Change**: The property defaults to `false` and shows deprecation warnings when used. Consider using explicit close buttons or modal/page overlay types instead for better accessibility. |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@spectrum-web-components/overlay': minor | ||
--- | ||
|
||
**Fixed** : external click registration behavior in the `sp-overlay` component. Programmatic clicks on elements outside of modal overlays now properly register and close the overlay, while user-initiated clicks are prevented from doing so. |
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,7 @@ if (!browserSupportsPopover) { | |
* @attr {string} receives-focus - How focus should be handled ('true'|'false'|'auto') | ||
* @attr {boolean} delayed - Whether the overlay should wait for a warm-up period before opening | ||
* @attr {boolean} open - Whether the overlay is currently open | ||
* @attr {boolean} allow-outside-click - @deprecated Whether clicks outside the overlay should close it (not recommended for accessibility) | ||
*/ | ||
export class Overlay extends ComputedOverlayBase { | ||
static override styles = [styles]; | ||
|
@@ -289,6 +290,18 @@ export class Overlay extends ComputedOverlayBase { | |
@property({ attribute: 'receives-focus' }) | ||
override receivesFocus: 'true' | 'false' | 'auto' = 'auto'; | ||
|
||
/** | ||
* @deprecated This property will be removed in a future version. | ||
* We do not recommend using this property for accessibility reasons. | ||
* It allows clicks outside the overlay to close it, which can cause | ||
* unexpected behavior and accessibility issues. | ||
* | ||
* @type {boolean} | ||
* @default false | ||
*/ | ||
Comment on lines
+293
to
+301
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 like this. |
||
@property({ type: Boolean, attribute: 'allow-outside-click' }) | ||
allowOutsideClick = false; | ||
|
||
/** | ||
* A reference to the slot element within the overlay. | ||
* | ||
|
@@ -495,7 +508,6 @@ export class Overlay extends ComputedOverlayBase { | |
* | ||
* This method handles the necessary steps to open the popover, including managing delays, | ||
* ensuring the popover is in the DOM, making transitions, and applying focus. | ||
* | ||
* @protected | ||
* @override | ||
* @returns {Promise<void>} A promise that resolves when the popover has been fully opened. | ||
|
@@ -552,6 +564,7 @@ export class Overlay extends ComputedOverlayBase { | |
}, | ||
// disable escape key capture to close the overlay, the focus-trap library captures it otherwise | ||
escapeDeactivates: false, | ||
allowOutsideClick: this.allowOutsideClick, | ||
}); | ||
|
||
if (this.type === 'modal' || this.type === 'page') { | ||
|
@@ -972,6 +985,23 @@ export class Overlay extends ComputedOverlayBase { | |
); | ||
} | ||
|
||
// Warn about deprecated allowOutsideClick property | ||
if (changes.has('allowOutsideClick') && this.allowOutsideClick) { | ||
if (window.__swc?.DEBUG) { | ||
window.__swc.warn( | ||
this, | ||
`The "allow-outside-click" attribute on <${this.localName}> has been deprecated and will be removed in a future release. We do not recommend using this attribute for accessibility reasons. It allows clicks outside the overlay to close it, which can cause unexpected behavior and accessibility issues.`, | ||
'https://opensource.adobe.com/spectrum-web-components/components/overlay/', | ||
{ level: 'deprecation' } | ||
); | ||
} else { | ||
// Fallback for testing environments or when SWC is not available | ||
console.warn( | ||
`[${this.localName}] The "allow-outside-click" attribute has been deprecated and will be removed in a future release. We do not recommend using this attribute for accessibility reasons. It allows clicks outside the overlay to close it, which can cause unexpected behavior and accessibility issues.` | ||
); | ||
} | ||
} | ||
|
||
// Manage the open state if the 'open' property has changed. | ||
if (changes.has('open') && (this.hasUpdated || this.open)) { | ||
this.manageOpen(changes.get('open')); | ||
|
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
Oops, something went wrong.
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.
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.
I like the wording on this. Thanks for including it!