Skip to content

fix(link): Emit click event on enter key when the component doesn't have href defined #12191

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.

Sign up for GitHub

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

Merged
jcfranco merged 4 commits into from
May 22, 2025
Merged

fix(link): Emit click event on enter key when the component doesn't have href defined #12191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63e64af
fix(link): Does not fire "click" event from keyboard if it doesn't ha…
driskull May 20, 2025
180b928
cleanup
driskull May 20, 2025
10e5547
add test
driskull May 20, 2025
2e311af
remove todo comment
driskull May 21, 2025
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
58 changes: 35 additions & 23 deletions packages/calcite-components/src/components/link/link.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,19 +65,19 @@ describe("calcite-link", () => {
expect(elementAsLink).not.toHaveAttribute("download");
});

it("renders as a span with default props", async () => {
it("renders as a button with default props", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link>Continue</calcite-link>`);

const element = await page.find("calcite-link");
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");

expect(element).not.toHaveAttribute("icon-flip-rtl");
expect(elementAsLink).toBeNull();
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(iconStart).toBeNull();
expect(iconEnd).toBeNull();
});
Expand All @@ -86,62 +86,62 @@ describe("calcite-link", () => {
const page = await newE2EPage({ html: `<calcite-link>Continue</calcite-link>` });
const link = await page.find("calcite-link");
let elementAsLink: E2EElement;
let elementAsSpan: E2EElement;
let elementAsButton: E2EElement;

elementAsSpan = await page.find("calcite-link >>> span");
elementAsButton = await page.find("calcite-link >>> button");
elementAsLink = await page.find("calcite-link >>> a");
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(elementAsLink).toBeNull();

link.setProperty("href", "/");
await page.waitForChanges();

elementAsSpan = await page.find("calcite-link >>> span");
elementAsButton = await page.find("calcite-link >>> button");
elementAsLink = await page.find("calcite-link >>> a");
expect(elementAsSpan).toBeNull();
expect(elementAsButton).toBeNull();
expect(elementAsLink).not.toBeNull();
});

it("renders as a link with default props", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link href="/">Continue</calcite-link>`);
const element = await page.find("calcite-link");
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");

expect(element).not.toHaveAttribute("icon-flip-rtl");
expect(elementAsLink).not.toBeNull();
expect(elementAsSpan).toBeNull();
expect(elementAsButton).toBeNull();
expect(iconStart).toBeNull();
expect(iconEnd).toBeNull();
});

it("renders as a span with requested props", async () => {
it("renders as a button with requested props", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link>Continue</calcite-link>`);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");

expect(elementAsLink).toBeNull();
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(iconStart).toBeNull();
expect(iconEnd).toBeNull();
});

it("renders as a link with requested props", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link href="/">Continue</calcite-link>`);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");

expect(elementAsLink).not.toBeNull();
expect(elementAsSpan).toBeNull();
expect(elementAsButton).toBeNull();
expect(iconStart).toBeNull();
expect(iconEnd).toBeNull();
});
Expand All @@ -151,13 +151,13 @@ describe("calcite-link", () => {
await page.setContent(
`<calcite-link rel="noopener noreferrer" target="_blank" class="my-custom-class" href="google.com">Continue</calcite-link>`,
);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");

expect(elementAsLink).not.toBeNull();
expect(elementAsSpan).toBeNull();
expect(elementAsButton).toBeNull();
expect(elementAsLink).not.toHaveClass("my-custom-class");
expect(elementAsLink).toEqualAttribute("href", "google.com");
expect(elementAsLink).toEqualAttribute("rel", "noopener noreferrer");
Expand All @@ -169,38 +169,38 @@ describe("calcite-link", () => {
it("renders with an icon-start", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link icon-start='plus'>Continue</calcite-link>`);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");
expect(elementAsLink).toBeNull();
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(iconStart).not.toBeNull();
expect(iconEnd).toBeNull();
});

it("renders with an icon-end", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link icon-end='plus'>Continue</calcite-link>`);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");
expect(elementAsLink).toBeNull();
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(iconStart).toBeNull();
expect(iconEnd).not.toBeNull();
});

it("renders with an icon-start and icon-end", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link icon-start='plus' icon-end='plus'>Continue</calcite-link>`);
const elementAsSpan = await page.find("calcite-link >>> span");
const elementAsButton = await page.find("calcite-link >>> button");
const elementAsLink = await page.find("calcite-link >>> a");
const iconStart = await page.find("calcite-link >>> .calcite-link--icon.icon-start");
const iconEnd = await page.find("calcite-link >>> .calcite-link--icon.icon-end");
expect(elementAsLink).toBeNull();
expect(elementAsSpan).not.toBeNull();
expect(elementAsButton).not.toBeNull();
expect(iconStart).not.toBeNull();
expect(iconEnd).not.toBeNull();
});
Expand Down Expand Up @@ -231,6 +231,18 @@ describe("calcite-link", () => {
expect(page.url()).toBe(targetUrl);
});

it("keyboard without href", async () => {
const element = await page.find("calcite-link");
element.setProperty("href", undefined);
const clickEvent = await element.spyOnEvent("click");

await element.callMethod("setFocus");
await page.waitForChanges();
await page.keyboard.press("Enter");
await page.waitForChanges();
expect(clickEvent).toHaveReceivedEventTimes(1);
});

it("mouse", async () => {
// workaround for https://github.com/puppeteer/puppeteer/issues/2977
await page.$eval("calcite-link", (link: HTMLElement): void => {
Expand Down
9 changes: 5 additions & 4 deletions packages/calcite-components/src/components/link/link.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

// link base
:host a,
:host span {
:host button {
@apply font-inherit
relative
flex
Expand All @@ -34,7 +34,7 @@

// focus styles
:host a,
:host span {
:host button {
@apply focus-base;
&:focus {
@apply focus-outset;
Expand Down Expand Up @@ -63,7 +63,7 @@ calcite-icon {
}

:host {
span,
button,
Copy link
Copy Markdown
Member

@jcfranco jcfranco May 21, 2025

Choose a reason for hiding this comment

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

Suggestion: styles and tests could be simplified if we use a link class and use it for targeting both button and a elements.

We can tackle this separately to keep the PR focused.

a {
@apply relative
inline
Expand All @@ -73,7 +73,8 @@ calcite-icon {
color: var(--calcite-link-text-color, var(--calcite-color-text-link));
line-height: inherit;
white-space: initial;
background-image: linear-gradient(currentColor, currentColor),
background-image:
linear-gradient(currentColor, currentColor),
linear-gradient(var(--calcite-color-brand-underline), var(--calcite-color-brand-underline));
background-position-x: 0%, 100%;
background-position-y: min(1.5em, 100%);
Expand Down
14 changes: 6 additions & 8 deletions packages/calcite-components/src/components/link/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ declare global {
/**
* Any attributes placed on <calcite-link> component will propagate to the rendered child
*
* Passing a 'href' will render an anchor link, instead of a span. Role will be set to link, or link, depending on this.
* Passing a 'href' will render an anchor link, instead of a button.
*
* It is the consumers responsibility to add aria information, rel, target, for links, and any link attributes for form submission
*
Expand All @@ -38,7 +38,7 @@ export class Link extends LitElement implements InteractiveComponent {
// #region Private Properties

/** the rendered child element */
private childEl: HTMLAnchorElement | HTMLSpanElement;
private childEl: HTMLAnchorElement | HTMLButtonElement;

// #endregion

Expand Down Expand Up @@ -131,7 +131,7 @@ export class Link extends LitElement implements InteractiveComponent {
override render(): JsxNode {
const { download, el } = this;
const dir = getElementDir(el);
const childElType = this.href ? "a" : "span";
const childElType = this.href ? "a" : "button";
const iconStartEl = (
<calcite-icon
class="calcite-link--icon icon-start"
Expand All @@ -151,11 +151,10 @@ export class Link extends LitElement implements InteractiveComponent {
);

const DynamicHtmlTag =
childElType === "span"
? (literal`span` as unknown as "span")
childElType === "button"
? (literal`button` as unknown as "button")
: (literal`a` as unknown as "a");
const role = childElType === "span" ? "link" : null;
const tabIndex = childElType === "span" ? 0 : null;
const tabIndex = childElType === "button" ? 0 : null;
/* TODO: [MIGRATION] This used <Host> before. In Stencil, <Host> props overwrite user-provided props. If you don't wish to overwrite user-values, replace "=" here with "??=" */
this.el.role = "presentation";

Expand All @@ -178,7 +177,6 @@ export class Link extends LitElement implements InteractiveComponent {
onClick={this.childElClickHandler}
ref={this.storeTagRef}
rel={childElType === "a" && this.rel}
role={role}
tabIndex={tabIndex}
target={childElType === "a" && this.target}
>
Expand Down
Loading