spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard
verified publisher icon adobe

fix(button): handles modifier keys correctly

Open Rajdeepc opened this issue 6 months ago • 5 comments

Prevent double tab opening for action buttons with href

Problem

When using sp-action-button with href and target="_blank", clicking the button would open two tabs instead of one. This happened because:

  1. Command + Click: Only browser's default link behavior (1 tab) ✅
  2. Regular Click: Both browser's default link behavior AND component's click handler (2 tabs) ❌

Root Cause

The ButtonBase component was proxying clicks to the anchor element even when href was present, causing the browser's native link behavior to be duplicated by the component's click handler.

Solution

Added a check in shouldProxyClick() to return early when href is present, allowing the browser to handle link behavior naturally:

// If this is a link (has href), let the browser handle it naturally
if (this.href && this.href.length > 0) {
    return false; // Don't interfere with browser's link handling
}

Changes Made

packages/button/src/ButtonBase.ts

  • Added early return in shouldProxyClick() when href is present
  • Prevents component from interfering with browser's link handling

packages/button/test/button.test.ts

  • Updated test to verify new behavior for buttons with href
  • Ensures clicks are NOT proxied to anchor when href is present
  • Maintains existing behavior for modifier key clicks

Example Usage

<sp-action-button
  href="https://github.com/adobe/spectrum-web-components"
  target="_blank"
>
    Click me
</sp-action-button>

Now opens exactly one tab regardless of how the user clicks the button.

Breaking Changes

None. This is a bug fix that improves expected behavior without changing the API.

Related issue(s)

  • fixes SWC-921

Screenshots (if appropriate)

Author's checklist

  • [x] 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.
  • [x] 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

  • [x] 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
  • [x] Automated tests cover all use cases and follow best practices for writing
  • [x] Validated on all supported browsers
  • [ ] All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • [ ] Href Button in Chrome + Safari + Firefox

    1. Go https://rajdeep-proxy-click-modifier-keys--spectrum-wc.netlify.app/storybook/?path=/story/action-button--href-with-target
    2. Click on the button
    3. Check it opens one tab
    4. Command + Click
    5. Check it also opens one tab

  • [ ] Voice Over Test

    1. Go here
    2. Once focus is on the button, voice over will instruct you to use CTRL+Option+Space to click the link.
    3. Press CTRL+Option+Space and click the link
    4. See one tab opens

Device review

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

Rajdeepc avatar Jun 17 '25 08:06 Rajdeepc

🦋 Changeset detected

Latest commit: 4c04d1518cd6abb46039230196e81cc50e066778

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

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

changeset-bot[bot] avatar Jun 17 '25 08:06 changeset-bot[bot]

Branch preview

Review the following VRT differences

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

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.

github-actions[bot] avatar Jun 17 '25 08:06 github-actions[bot]

Tachometer results

Chrome

button permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 501 kB 50.24ms - 51.49ms - faster ✔
6% - 10%
3.39ms - 5.73ms
branch 478 kB 54.44ms - 56.42ms slower ❌
7% - 11%
3.39ms - 5.73ms		 -
Firefox

button permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 501 kB 119.43ms - 126.33ms - unsure 🔍
-7% - -0%
-8.70ms - +0.02ms
branch 478 kB 124.55ms - 129.89ms unsure 🔍
-0% - +7%
-0.02ms - +8.70ms		 -

github-actions[bot] avatar Jun 17 '25 08:06 github-actions[bot]

I know @nikkimk may have looked at this shouldProxyClick functionality before - I can't remember if there is something A11Y-related that we need to consider with this fix?

rubencarvalho avatar Jun 17 '25 20:06 rubencarvalho

I know @nikkimk may have looked at this shouldProxyClick functionality before - I can't remember if there is something A11Y-related that we need to consider with this fix?

Its been taken care of. You can refer to this PR. I have also updated the test suits. Refer to the Voice Over test

Rajdeepc avatar Jun 18 '25 06:06 Rajdeepc