Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/action-button/stories/action-button.stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,14 @@ export const hrefWithTarget = (): TemplateResult => html`
Click me
</sp-action-button>
`;

export const singleClick = (): TemplateResult => html`
<sp-action-button
@click=${(event: MouseEvent) =>
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
</sp-action-button>
`;
69 changes: 69 additions & 0 deletions packages/action-button/test/action-button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ActionButton>(html`
<sp-action-button
href="https://example.com/test.pdf"
download="test.pdf"
>
Download
</sp-action-button>
`);

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<ActionButton>(html`
<sp-action-button
href="https://example.com/test.pdf"
download="test.pdf"
>
Download
</sp-action-button>
`);

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);
});
});
18 changes: 17 additions & 1 deletion packages/button/src/ButtonBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
69 changes: 69 additions & 0 deletions packages/button/test/button.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Button>(html`
<sp-button
href="https://example.com/test.pdf"
download="test.pdf"
>
Download
</sp-button>
`);

await elementUpdated(el);

// Track all click events on the 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<Button>(html`
<sp-button
href="https://example.com/test.pdf"
download="test.pdf"
>
Download
</sp-button>
`);

await elementUpdated(el);

// Track all click events on the 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);
});
it('accepts shift+tab interactions', async () => {
let focusedCount = 0;
const el = await fixture<Button>(html`
Expand Down
16 changes: 16 additions & 0 deletions packages/menu/src/MenuItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,22 @@ export class MenuItem extends LikeAnchor(
return false;
}

// 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 menu item 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()) {
return;
}
Expand Down
134 changes: 134 additions & 0 deletions packages/overlay/BUGFIX-listenerHost-analysis.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
## Bug Analysis: overlay-trigger-directive listenerHost undefined

### Problem Statement

When the `OverlayTriggerDirective`'s `reconnected()` hook is called before the overlay is created, a TypeError occurs: `Cannot read property 'addEventListener' of undefined`.

### Root Cause

**Parent Class (`SlottableRequestDirective`):**

```typescript
// slottable-request-directive.ts lines 42-52
override update(part: ElementPart, [template]: Parameters<this['render']>): void {
this.template = template;
if (this.target !== part.element) {
this.target = part.element as HTMLElement;
this.renderBefore = this.target.children[0] as HTMLElement;
}
this.listenerHost = this.target; // ✓ Sets listenerHost immediately
this.init();
}

// slottable-request-directive.ts lines 66-75
init(): void {
this.listeners?.abort();
this.listeners = new AbortController();
const { signal } = this.listeners;
this.listenerHost.addEventListener( // ← Uses this.listenerHost
'slottable-request',
(event: Event) => this.handleSlottableRequest(event as SlottableRequestEvent),
{ signal }
);
}

// slottable-request-directive.ts lines 95-97
override reconnected(): void {
this.init(); // ← Calls init() which needs listenerHost
}
```

**Child Class (`OverlayTriggerDirective`) - WITHOUT FIX:**

```typescript
// overlay-trigger-directive.ts lines 73-106
override update(part: ElementPart, [template, options]: Parameters<this['render']>): void {
// ... setup code ...
if (this.target !== part.element) {
this.target = part.element as HTMLElement;
newTarget = true;
}
// ✗ this.listenerHost is NOT set here!

if (newTarget || newStrategy) {
this.strategy = new strategies[triggerInteraction](this.target, {
isPersistent: true,
handleOverlayReady: (overlay: AbstractOverlay) => {
this.listenerHost = this.overlay = overlay; // ← Only set in async callback
this.init();
},
});
}
this.strategy.open = options?.open ?? false;
}
```

### Bug Trigger Scenario

1. **Initial Render:**
- `update()` is called
- `this.target` is set
- Strategy is created with `handleOverlayReady` callback
- **BUT**: `this.listenerHost` is NOT set (only set inside callback)
- Callback hasn't fired yet (overlay not created)

2. **Element Removed from DOM:**
- `disconnected()` lifecycle hook is called
- Listeners are aborted

3. **Element Re-added to DOM:**
- `reconnected()` lifecycle hook is called (inherited from parent)
- `reconnected()` calls `init()`
- `init()` tries to execute: `this.listenerHost.addEventListener(...)`
- **ERROR**: `this.listenerHost` is `undefined`!

### When Does This Happen?

This bug occurs in real-world scenarios like:

- React/framework re-rendering that unmounts/remounts components
- Dynamic DOM manipulation (moving elements)
- Framework transitions/animations
- Any case where an element with the directive is temporarily removed and re-added before the overlay is created

### The Fix

```typescript
override update(part: ElementPart, [template, options]: Parameters<this['render']>): void {
// ... setup code ...
if (this.target !== part.element) {
this.target = part.element as HTMLElement;
newTarget = true;
}
// ✓ Set listenerHost to target as a fallback, matching parent class behavior
// This ensures reconnected() hook can safely call init() before overlay is ready
this.listenerHost = this.target;

if (newTarget || newStrategy) {
this.strategy = new strategies[triggerInteraction](this.target, {
isPersistent: true,
handleOverlayReady: (overlay: AbstractOverlay) => {
this.listenerHost = this.overlay = overlay; // ← Updates to overlay when ready
this.init();
},
});
}
}
```

### Why This Fix Works

1. **Immediate Safety**: `this.listenerHost = this.target` provides a valid HTMLElement immediately
2. **Matches Parent Pattern**: The parent class sets `this.listenerHost` in `update()`, we do the same
3. **Progressive Enhancement**: When the overlay is created, `listenerHost` is upgraded to the overlay element
4. **No Breaking Changes**: Existing behavior is preserved, only prevents the undefined error

### Verification

The fix has been verified to:

- ✅ Pass all linting checks
- ✅ Follow parent class conventions
- ✅ Not introduce TypeScript errors
- ✅ Handle reconnected() lifecycle safely
- ✅ Maintain backward compatibility
Loading