From cfcbaec018194c1d96ec1146f4d1d966cfa6953f Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Wed, 15 Oct 2025 17:37:15 +0530 Subject: [PATCH 1/2] fix(overlay): added prepareDescription method to clickController --- packages/overlay/src/ClickController.ts | 81 +++++++++++++++++++ packages/overlay/src/HoverController.ts | 5 ++ packages/overlay/src/InteractionController.ts | 22 ++++- packages/overlay/src/Overlay.ts | 11 +++ 4 files changed, 118 insertions(+), 1 deletion(-) diff --git a/packages/overlay/src/ClickController.ts b/packages/overlay/src/ClickController.ts index ca5b4d92478..6aa78781959 100644 --- a/packages/overlay/src/ClickController.ts +++ b/packages/overlay/src/ClickController.ts @@ -10,6 +10,9 @@ * governing permissions and limitations under the License. */ +import { conditionAttributeWithId } from '@spectrum-web-components/base/src/condition-attribute-with-id.js'; +import { randomID } from '@spectrum-web-components/shared/src/random-id.js'; +import { noop } from './AbstractOverlay.js'; import { InteractionController, InteractionTypes, @@ -18,6 +21,10 @@ import { export class ClickController extends InteractionController { override type = InteractionTypes.click; + private elementIds: string[] = []; + + override releaseDescription = noop; + /** * An overlay with a `click` interaction should not close on click `triggerElement`. * When a click is initiated (`pointerdown`), apply `preventNextToggle` when the @@ -37,6 +44,80 @@ export class ClickController extends InteractionController { this.preventNextToggle = this.open; } + /** + * Prepares ARIA description for the trigger element. + * + * Establishes an accessible relationship between the trigger and overlay content + * by setting `aria-describedby` on the trigger element. This enables screen readers + * to announce the overlay content when the trigger receives focus. + * + * The method determines the appropriate ARIA strategy based on the DOM tree + * relationship between the trigger, overlay, and content elements: + * - If trigger and overlay share the same root: references the overlay element ID + * - If trigger and content share the same root: references the content element IDs + * + * @param {HTMLElement} trigger - The trigger element that will receive the aria-describedby attribute + * @override + */ + override prepareDescription(trigger: HTMLElement): void { + // Do not reapply until target is recycled + if (this.releaseDescription !== noop) return; + + // Require "content" to apply relationship + if (!this.overlay.elements.length) { + return; + } + + const triggerRoot = trigger.getRootNode(); + const contentRoot = this.overlay.elements[0].getRootNode(); + const overlayRoot = this.overlay.getRootNode(); + + if (triggerRoot === overlayRoot) { + this.prepareOverlayRelativeDescription(trigger); + } else if (triggerRoot === contentRoot) { + this.prepareContentRelativeDescription(trigger); + } + } + + private prepareOverlayRelativeDescription(trigger: HTMLElement): void { + const releaseDescription = conditionAttributeWithId( + trigger, + 'aria-describedby', + [this.overlay.id] + ); + + this.releaseDescription = () => { + releaseDescription(); + this.releaseDescription = noop; + }; + } + + private prepareContentRelativeDescription(trigger: HTMLElement): void { + const elementIds: string[] = []; + const appliedIds = this.overlay.elements.map((el) => { + elementIds.push(el.id); + if (!el.id) { + el.id = `${this.overlay.tagName.toLowerCase()}-helper-${randomID()}`; + } + return el.id; + }); + this.elementIds = elementIds; + + const releaseDescription = conditionAttributeWithId( + trigger, + 'aria-describedby', + appliedIds + ); + + this.releaseDescription = () => { + releaseDescription(); + this.overlay.elements.map((el, index) => { + el.id = this.elementIds[index]; + }); + this.releaseDescription = noop; + }; + } + override init(): void { // Clean up listeners if they've already been bound this.abortController?.abort(); diff --git a/packages/overlay/src/HoverController.ts b/packages/overlay/src/HoverController.ts index bd77ae8fdc9..77ec1774841 100644 --- a/packages/overlay/src/HoverController.ts +++ b/packages/overlay/src/HoverController.ts @@ -27,6 +27,8 @@ export class HoverController extends InteractionController { private elementIds: string[] = []; + override releaseDescription = noop; + focusedin = false; private hoverTimeout?: ReturnType; @@ -89,6 +91,9 @@ export class HoverController extends InteractionController { } override prepareDescription(): void { + // do not reapply until target is recycled + if (this.releaseDescription !== noop) return; + // require "content" to apply relationship if (!this.overlay.elements.length) return; diff --git a/packages/overlay/src/InteractionController.ts b/packages/overlay/src/InteractionController.ts index c9f9278409e..0c4c260e986 100644 --- a/packages/overlay/src/InteractionController.ts +++ b/packages/overlay/src/InteractionController.ts @@ -74,7 +74,7 @@ export class InteractionController implements ReactiveController { this.overlay.open = true; this.target[lastInteractionType] = this.type; }); - import('@spectrum-web-components/overlay/sp-overlay.js'); + import('../sp-overlay.js'); } public get overlay(): AbstractOverlay { @@ -91,9 +91,29 @@ export class InteractionController implements ReactiveController { this.overlay.addController(this); this.initOverlay(); this.prepareDescription(this.target); + + // Observe when overlay elements change and call prepareDescription again + // This handles the case where content is slotted after the overlay is created + this.observeOverlayElements(); + this.handleOverlayReady?.(this.overlay); } + private observeOverlayElements(): void { + // Use requestUpdate to be notified when overlay elements change + const checkElements = (): void => { + if (this.overlay.elements && this.overlay.elements.length > 0) { + // Elements are now available, call prepareDescription + this.prepareDescription(this.target); + } + }; + + // Listen for the overlay's update cycle + this.overlay.updateComplete.then(() => { + checkElements(); + }); + } + private _overlay!: AbstractOverlay; protected isPersistent = false; diff --git a/packages/overlay/src/Overlay.ts b/packages/overlay/src/Overlay.ts index f1ec3e07f29..6bd1ec3fd7d 100644 --- a/packages/overlay/src/Overlay.ts +++ b/packages/overlay/src/Overlay.ts @@ -1037,7 +1037,18 @@ export class Overlay extends ComputedOverlayBase { // If the trigger element has changed, bind the new events. if (oldTrigger !== false) { + // Release description from the old trigger before binding new events + this.strategy?.releaseDescription(); + this.bindEvents(); + + // Prepare description for new trigger if elements are already present + // This ensures screen reader accessibility is maintained after trigger changes + if (this.hasNonVirtualTrigger && this.elements.length) { + this.strategy?.prepareDescription( + this.triggerElement as HTMLElement + ); + } } } From 5a1ef0cb9f2b9f2c23f4d561bf66d049a2aab60a Mon Sep 17 00:00:00 2001 From: Rajdeep Chandra Date: Thu, 6 Nov 2025 10:59:28 +0530 Subject: [PATCH 2/2] chore: all fixes for testing --- .../stories/action-button.stories.ts | 11 ++ .../action-button/test/action-button.test.ts | 69 +++++++++ packages/button/src/ButtonBase.ts | 18 ++- packages/button/test/button.test.ts | 69 +++++++++ packages/menu/src/MenuItem.ts | 16 +++ .../overlay/BUGFIX-listenerHost-analysis.md | 134 ++++++++++++++++++ .../overlay/src/overlay-trigger-directive.ts | 3 + 7 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 packages/overlay/BUGFIX-listenerHost-analysis.md diff --git a/packages/action-button/stories/action-button.stories.ts b/packages/action-button/stories/action-button.stories.ts index 477ca716caf..8a2ae59b3c8 100644 --- a/packages/action-button/stories/action-button.stories.ts +++ b/packages/action-button/stories/action-button.stories.ts @@ -57,3 +57,14 @@ export const hrefWithTarget = (): TemplateResult => html` Click me `; + +export const singleClick = (): TemplateResult => html` + + console.log(`click handler, event is trusted: ${event.isTrusted}`)} + href="https://partners.adobe.com/channelpartnerassets/assets/public/public_1/aem_assets_dynamic_media_capability_spotlight_ue.pdf" + download="Adobe Experience Manager Assets Dynamic Media Capability Spotlight.pdf" + > + Icon Download + +`; diff --git a/packages/action-button/test/action-button.test.ts b/packages/action-button/test/action-button.test.ts index fafaac529c1..b2fc716e470 100644 --- a/packages/action-button/test/action-button.test.ts +++ b/packages/action-button/test/action-button.test.ts @@ -313,4 +313,73 @@ describe('ActionButton', () => { await elementUpdated(el); expect(clicked).to.be.true; }); + it('dispatches only one click event per user interaction', async () => { + let clickCount = 0; + const el = await fixture(html` + + Download + + `); + + await elementUpdated(el); + + // Track all click events on the action button + el.addEventListener('click', () => { + clickCount++; + }); + + // Prevent the anchor from actually navigating + el.shadowRoot + ?.querySelector('.anchor') + ?.addEventListener('click', (event: Event) => { + event.preventDefault(); + }); + + // Simulate a user click + await mouseClickOn(el); + await elementUpdated(el); + + // Should only have one click event, not two + expect(clickCount).to.equal(1); + }); + it('allows keyboard activation with href', async () => { + let clickCount = 0; + const el = await fixture(html` + + Download + + `); + + await elementUpdated(el); + + // Track all click events on the action button + el.addEventListener('click', () => { + clickCount++; + }); + + // Prevent the anchor from actually navigating + el.shadowRoot + ?.querySelector('.anchor') + ?.addEventListener('click', (event: Event) => { + event.preventDefault(); + }); + + // Test Enter key + el.focus(); + await sendKeys({ press: 'Enter' }); + await elementUpdated(el); + expect(clickCount).to.equal(1); + + // Test Space key + clickCount = 0; + await sendKeys({ press: 'Space' }); + await elementUpdated(el); + expect(clickCount).to.equal(1); + }); }); diff --git a/packages/button/src/ButtonBase.ts b/packages/button/src/ButtonBase.ts index 9fbae9d6a79..43ea726ddd2 100644 --- a/packages/button/src/ButtonBase.ts +++ b/packages/button/src/ButtonBase.ts @@ -93,7 +93,23 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [ return false; } - if (this.shouldProxyClick(event as MouseEvent)) { + // If this is a synthetic click (isTrusted: false) bubbling up from our + // anchor element proxy, stop it to prevent duplicate click events. + // However, allow synthetic clicks that originate from the button itself + // (e.g., from keyboard interactions or programmatic clicks). + // We check composedPath() because event.target gets retargeted across + // shadow boundaries. + const mouseEvent = event as MouseEvent; + if ( + !mouseEvent.isTrusted && + this.anchorElement && + event.composedPath()[0] === this.anchorElement + ) { + event.stopPropagation(); + return false; + } + + if (this.shouldProxyClick(mouseEvent)) { return; } } diff --git a/packages/button/test/button.test.ts b/packages/button/test/button.test.ts index c923084f622..ebe830ee04e 100644 --- a/packages/button/test/button.test.ts +++ b/packages/button/test/button.test.ts @@ -245,6 +245,75 @@ describe('Button', () => { await elementUpdated(el); expect(clicked).to.be.true; }); + it('dispatches only one click event per user interaction', async () => { + let clickCount = 0; + const el = await fixture