Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Rajdeepc
Copy link
Contributor

@Rajdeepc Rajdeepc commented Aug 5, 2025

Description

This PR adds allowOutsideClick property for the external click registration behavior in the sp-overlay component. The implementation verifies that programmatic clicks on elements outside of modal overlays properly register and close the overlay, while user-initiated clicks are prevented from doing so.

Technical Details

External Click Behavior

The overlay component uses a focus trap for modal and page type overlays with special handling for external clicks:

// From Overlay.ts 
allowOutsideClick: (event) => {
    return !event.isTrusted;
}

This configuration ensures:

  • Programmatic clicks (non-trusted events) → Will close the overlay
  • User clicks (trusted events) → Will NOT close the overlay

Motivation and context

This bug came when showModal() api was changed to dialog.show() and this edge case was missed.

Related issue(s)

  • fixes SWC - 1018

Screenshots (if appropriate)


Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes.
  • I have included a well-written changeset if my change needs to be published.
  • I have included updated documentation if my change required it.

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • Manual Testing - Chrome, Firefox & Safari

    1. Go here
    2. Open developer console
    3. Open Overlay and Click on the button
    4. Check it prints the log from the button event
  • Test Automation

    1. Tests external click functionality after user interactions within the overlay
    2. Simulates clicking buttons and typing in inputs inside the overlay
    3. Verifies that external clicks still register and close the overlay even after internal interactions

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

@Rajdeepc Rajdeepc self-assigned this Aug 5, 2025
@Rajdeepc Rajdeepc added the bug Something isn't working label Aug 5, 2025
Copy link

changeset-bot bot commented Aug 5, 2025

🦋 Changeset detected

Latest commit: 9ce6b30

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 84 packages
Name Type
@spectrum-web-components/overlay Minor
@spectrum-web-components/combobox Minor
@spectrum-web-components/contextual-help Minor
@spectrum-web-components/menu Minor
@spectrum-web-components/picker Minor
@spectrum-web-components/popover Minor
@spectrum-web-components/tooltip Minor
@spectrum-web-components/story-decorator Minor
@spectrum-web-components/bundle Minor
@spectrum-web-components/truncated Minor
@spectrum-web-components/breadcrumbs Minor
@spectrum-web-components/custom-vars-viewer Minor
example-project-rollup Patch
example-project-webpack Patch
@spectrum-web-components/action-menu Minor
@spectrum-web-components/action-bar Minor
@spectrum-web-components/card Minor
documentation Patch
@spectrum-web-components/vrt-compare Minor
@spectrum-web-components/eslint-plugin Minor
@spectrum-web-components/accordion Minor
@spectrum-web-components/action-button Minor
@spectrum-web-components/action-group Minor
@spectrum-web-components/alert-banner Minor
@spectrum-web-components/alert-dialog Minor
@spectrum-web-components/asset Minor
@spectrum-web-components/avatar Minor
@spectrum-web-components/badge Minor
@spectrum-web-components/button-group Minor
@spectrum-web-components/button Minor
@spectrum-web-components/checkbox Minor
@spectrum-web-components/clear-button Minor
@spectrum-web-components/close-button Minor
@spectrum-web-components/coachmark Minor
@spectrum-web-components/color-area Minor
@spectrum-web-components/color-field Minor
@spectrum-web-components/color-handle Minor
@spectrum-web-components/color-loupe Minor
@spectrum-web-components/color-slider Minor
@spectrum-web-components/color-wheel Minor
@spectrum-web-components/dialog Minor
@spectrum-web-components/divider Minor
@spectrum-web-components/dropzone Minor
@spectrum-web-components/field-group Minor
@spectrum-web-components/field-label Minor
@spectrum-web-components/help-text Minor
@spectrum-web-components/icon Minor
@spectrum-web-components/icons-ui Minor
@spectrum-web-components/icons-workflow Minor
@spectrum-web-components/icons Minor
@spectrum-web-components/iconset Minor
@spectrum-web-components/illustrated-message Minor
@spectrum-web-components/infield-button Minor
@spectrum-web-components/link Minor
@spectrum-web-components/meter Minor
@spectrum-web-components/modal Minor
@spectrum-web-components/number-field Minor
@spectrum-web-components/picker-button Minor
@spectrum-web-components/progress-bar Minor
@spectrum-web-components/progress-circle Minor
@spectrum-web-components/radio Minor
@spectrum-web-components/search Minor
@spectrum-web-components/sidenav Minor
@spectrum-web-components/slider Minor
@spectrum-web-components/split-view Minor
@spectrum-web-components/status-light Minor
@spectrum-web-components/swatch Minor
@spectrum-web-components/switch Minor
@spectrum-web-components/table Minor
@spectrum-web-components/tabs Minor
@spectrum-web-components/tags Minor
@spectrum-web-components/textfield Minor
@spectrum-web-components/thumbnail Minor
@spectrum-web-components/toast Minor
@spectrum-web-components/top-nav Minor
@spectrum-web-components/tray Minor
@spectrum-web-components/underlay Minor
@spectrum-web-components/base Minor
@spectrum-web-components/grid Minor
@spectrum-web-components/opacity-checkerboard Minor
@spectrum-web-components/reactive-controllers Minor
@spectrum-web-components/shared Minor
@spectrum-web-components/styles Minor
@spectrum-web-components/theme Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rajdeepc Rajdeepc changed the title Rajdeep/swc 1018 fix(overlay): Programmatic clicks on elements outside of the overlay now registers Aug 5, 2025
Copy link

github-actions bot commented Aug 5, 2025

📚 Branch Preview

🔍 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Deployed to Azure Blob Storage: pr-5670

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

Copy link

github-actions bot commented Aug 5, 2025

Tachometer results

Chrome

overlay permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 821 kB 406.36ms - 412.40ms - faster ✔
1% - 3%
4.05ms - 10.71ms
branch 789 kB 415.36ms - 418.16ms slower ❌
1% - 3%
4.05ms - 10.71ms
-

directive-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 825 kB 24.54ms - 25.52ms - faster ✔
4% - 9%
1.00ms - 2.38ms
branch 793 kB 26.23ms - 27.21ms slower ❌
4% - 10%
1.00ms - 2.38ms
-

element-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 814 kB 336.08ms - 341.18ms - faster ✔
2% - 3%
5.39ms - 11.25ms
branch 782 kB 345.52ms - 348.38ms slower ❌
2% - 3%
5.39ms - 11.25ms
-

lazy-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 615 kB 42.05ms - 43.19ms - faster ✔
4% - 8%
1.98ms - 3.51ms
branch 584 kB 44.85ms - 45.88ms slower ❌
5% - 8%
1.98ms - 3.51ms
-
Firefox

overlay permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 821 kB 780.12ms - 795.76ms - unsure 🔍
-1% - +2%
-7.06ms - +14.46ms
branch 789 kB 776.85ms - 791.63ms unsure 🔍
-2% - +1%
-14.46ms - +7.06ms
-

directive-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 825 kB 47.31ms - 49.05ms - faster ✔
2% - 6%
1.03ms - 3.01ms
branch 793 kB 49.73ms - 50.67ms slower ❌
2% - 6%
1.03ms - 3.01ms
-

element-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 814 kB 625.84ms - 633.64ms - unsure 🔍
-1% - +1%
-4.35ms - +5.79ms
branch 782 kB 625.79ms - 632.25ms unsure 🔍
-1% - +1%
-5.79ms - +4.35ms
-

lazy-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 615 kB 90.15ms - 92.49ms - faster ✔
3% - 6%
2.63ms - 6.05ms
branch 584 kB 94.42ms - 96.90ms slower ❌
3% - 7%
2.63ms - 6.05ms
-

@Rajdeepc Rajdeepc marked this pull request as ready for review August 5, 2025 09:14
@Rajdeepc Rajdeepc requested a review from a team as a code owner August 5, 2025 09:14
@Rajdeepc Rajdeepc added the SEV 2 label Aug 7, 2025
Copy link
Contributor

@nikkimk nikkimk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we go further, we should make sure that modals should support outside clicks.


```javascript
// Programmatic click outside will close the overlay
function closeOverlayProgrammatically() {
Copy link
Contributor

@nikkimk nikkimk Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From: https://w3c.github.io/aria/#aria-modal

The aria-modal attribute is used to indicate that the presence of a "modal" element precludes usage of other content on the page. For example, when a modal dialog is displayed, it is expected that the user's interaction is limited to the contents of the dialog, until the modal dialog loses focus or is no longer displayed.

When a modal element is displayed, assistive technologies SHOULD navigate to the element unless focus has explicitly been set elsewhere. Some assistive technologies limit navigation to the modal element's contents. If focus moves to an element outside the modal element, assistive technologies SHOULD NOT limit navigation to the modal element.

When a modal element is displayed, authors MUST ensure the interface can be controlled using only descendants of the modal element. In other words, if a modal dialog has a close button, the button should be a descendant of the dialog. When a modal element is displayed, authors SHOULD mark all other contents as inert (such as "inert subtrees" in HTML) if the ability to do so exists in the host language.

Clicking should not be allowed because keyboard focus outside the modal should not be allowed.

Tagging @jnurthen and @majornista on this one for their thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready-for-review SEV 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants