fix(action-button): remove proxy click in capture phase (#5204)

* fix(action-button): remove proxy click in capture phase

* fix(action-button): remove proxy click in capture phase keeping the form action

* fix(action-button): added changeset

* fix(button): removed proxy click and add meta key support for voiceover

* fix(action-button): update changeset

* chore: updated golden image cache

---------

Co-authored-by: Rajdeep Chandra <[email protected]>
Co-authored-by: Casey Eickhoff <[email protected]>

Author: 3 people

.changeset/rich-bears-fold.md

@@ -0,0 +1,5 @@
+---
+'@spectrum-web-components/action-button': minor
+---
+
+-   **Fixed** : Action buttons with href attributes now properly detects modifier keys and skips the proxy click, allowing only native browser behavior to proceed.

.circleci/config.yml

@@ -14,7 +14,7 @@ parameters:
     # 3. Commit this change to the PR branch where the changes exist.
     current_golden_images_hash:
         type: string
-        default: a6fdd13f78f7931bbf15af5aef3c79f0423df3b5
+        default: c5b517432c05e1eeac576ad651233cef83957ce9
     wireit_cache_name:
         type: string
         default: wireit

packages/action-button/stories/action-button.stories.ts

@@ -47,3 +47,12 @@ href.args = {
     href: 'https://github.com/adobe/spectrum-web-components',
     icon: `<sp-icon-edit hidden slot="icon"></sp-icon-edit>`,
 };
+
+export const hrefWithTarget = (): TemplateResult => html`
+    <sp-action-button
+        href="https://github.com/adobe/spectrum-web-components"
+        target="_blank"
+    >
+        Click me
+    </sp-action-button>
+`;

packages/button/src/ButtonBase.ts

@@ -93,7 +93,7 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
             return false;
         }
 
-        if (this.shouldProxyClick()) {
+        if (this.shouldProxyClick(event as MouseEvent)) {
             return;
         }
     }
@@ -102,10 +102,19 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
         this.focus();
     }
 
-    private shouldProxyClick(): boolean {
+    private shouldProxyClick(event?: MouseEvent): boolean {
         let handled = false;
+
+        // Don't proxy clicks with modifier keys (Command/Meta, Ctrl, Shift, Alt)
+        if (
+            event &&
+            (event.metaKey || event.ctrlKey || event.shiftKey || event.altKey)
+        ) {
+            return false;
+        }
+
         if (this.anchorElement) {
-            // click HTML anchor element by proxy
+            // Click HTML anchor element by proxy, but only for non-modified clicks
             this.anchorElement.click();
             handled = true;
             // if the button type is `submit` or `reset`
@@ -127,7 +136,8 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
             ${super.renderAnchor({
                 id: 'button',
                 ariaHidden: true,
-                className: 'button anchor hidden',
+                className: 'button anchor',
+                tabindex: -1,
             })}
         `;
     }
@@ -234,8 +244,16 @@ export class ButtonBase extends ObserveSlotText(LikeAnchor(Focusable), '', [
         }
 
         if (this.anchorElement) {
-            this.anchorElement.addEventListener('focus', this.proxyFocus);
+            // Ensure the anchor element is not focusable directly via tab
             this.anchorElement.tabIndex = -1;
+
+            // Make sure it has proper ARIA attributes
+            if (!this.anchorElement.hasAttribute('aria-hidden')) {
+                this.anchorElement.setAttribute('aria-hidden', 'true');
+            }
+
+            // Set up focus delegation
+            this.anchorElement.addEventListener('focus', this.proxyFocus);
         }
     }
     protected override update(changes: PropertyValues): void {

packages/button/test/button.test.ts

@@ -719,28 +719,6 @@ describe('Button', () => {
             expect(submitSpy.callCount).to.equal(1);
             expect(resetSpy.callCount).to.equal(1);
         });
-        it('proxies click by [href]', async () => {
-            const clickSpy = spy();
-            const el = await fixture<Button>(html`
-                <sp-button href="test_url">With Href</sp-button>
-            `);
-
-            await elementUpdated(el);
-            (
-                el as unknown as {
-                    anchorElement: HTMLAnchorElement;
-                }
-            ).anchorElement.addEventListener('click', (event: Event): void => {
-                event.preventDefault();
-                event.stopPropagation();
-                clickSpy();
-            });
-            expect(clickSpy.callCount).to.equal(0);
-
-            el.click();
-            await elementUpdated(el);
-            expect(clickSpy.callCount).to.equal(1);
-        });
         it('manages "active" while focused', async () => {
             const el = await fixture<Button>(html`
                 <sp-button label="Button">
@@ -849,5 +827,89 @@ describe('Button', () => {
                 expect(el.hasAttribute('static-color')).to.be.false;
             });
         });
+        it('handles modifier key clicks correctly', async () => {
+            const el = await fixture<Button>(html`
+                <sp-button href="#test">Button with href</sp-button>
+            `);
+
+            await elementUpdated(el);
+
+            const anchorElement = el.shadowRoot?.querySelector(
+                '.anchor'
+            ) as HTMLAnchorElement;
+            expect(anchorElement).to.not.be.undefined;
+
+            // Set up spies instead of counters
+            const buttonClickSpy = spy();
+            const anchorClickSpy = spy();
+
+            // Prevent actual navigation but track anchor clicks with spy
+            anchorElement.addEventListener('click', (event) => {
+                event.preventDefault();
+                anchorClickSpy();
+            });
+
+            // Track button clicks with spy
+            el.addEventListener('click', buttonClickSpy);
+
+            // Test normal click - should proxy to anchor
+            const normalClick = new MouseEvent('click', {
+                bubbles: true,
+                cancelable: true,
+            });
+
+            buttonClickSpy.resetHistory();
+            anchorClickSpy.resetHistory();
+
+            el.dispatchEvent(normalClick);
+            await elementUpdated(el);
+
+            expect(
+                anchorClickSpy.called,
+                'Normal click should be proxied to the anchor'
+            ).to.be.true;
+
+            buttonClickSpy.resetHistory();
+            anchorClickSpy.resetHistory();
+
+            const metaClick = new MouseEvent('click', {
+                bubbles: true,
+                cancelable: true,
+                metaKey: true,
+            });
+
+            el.dispatchEvent(metaClick);
+            await elementUpdated(el);
+
+            expect(
+                buttonClickSpy.called,
+                'Meta+click should be received by the button'
+            ).to.be.true;
+            expect(
+                anchorClickSpy.called,
+                'Meta+click should NOT be proxied to the anchor'
+            ).to.be.false;
+
+            buttonClickSpy.resetHistory();
+            anchorClickSpy.resetHistory();
+
+            const ctrlClick = new MouseEvent('click', {
+                bubbles: true,
+                cancelable: true,
+                ctrlKey: true,
+            });
+
+            el.dispatchEvent(ctrlClick);
+            await elementUpdated(el);
+
+            expect(
+                buttonClickSpy.called,
+                'Ctrl+click should be received by the button'
+            ).to.be.true;
+            expect(
+                anchorClickSpy.called,
+                'Ctrl+click should NOT be proxied to the anchor'
+            ).to.be.false;
+        });
     });
 });