Skip to content

fix(ui): make LinkWithArrow behave like accessible button for modal open #8051

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.

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Mohit5Upadhyay
Copy link
Contributor

Description

This PR solve the issue: #8048 by improving accessibility using LinkWithArrow as a button to trigger the modal with correct role, tabIndex, aria-label, and keyboard support.

Validation

details-btn.mp4

Related Issues

Closes #8048

Check List

  • [✅] I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • [✅] I have run pnpm format to ensure the code follows the style guide.
  • [✅] I have run pnpm test to check if all tests are passing.
  • [✅] I have run pnpm build to check if the website builds without errors.
  • [✅] I've covered new added functionality with unit tests if necessary.

@Copilot Copilot AI review requested due to automatic review settings July 30, 2025 08:50
@Mohit5Upadhyay Mohit5Upadhyay requested a review from a team as a code owner July 30, 2025 08:50
Copy link

vercel bot commented Jul 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ❌ Failed (Inspect) Aug 1, 2025 9:29pm

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves accessibility of the details button in the download releases table by making the LinkWithArrow component behave like a proper accessible button for modal opening. It addresses accessibility concerns by adding appropriate ARIA attributes and keyboard navigation support.

Key changes:

  • Added proper button semantics with role="button" and tabIndex={0}
  • Implemented keyboard support for Enter and Space key activation
  • Enhanced screen reader support with aria-label

@avivkeller
Copy link
Member

There should be a way to do this without adding the additional listener, right?

According to https://developer.mozilla.org/en-US/docs/Web/API/Element/click_event, the "click" event is emitted on key press.

@Mohit5Upadhyay
Copy link
Contributor Author

Thank you for the suggestion, @avivkeller! 🙌

According to https://developer.mozilla.org/en-US/docs/Web/API/Element/click_event, the "click" event is emitted on key press.

You're absolutely right — a manual listener isn’t needed if the element were a native <button> or <a href>.

Since LinkWithArrow renders a non-interactive element under the hood (<span>) keyboard events like Enter and Space won't trigger the on click natively, even with role="button" and tabIndex={0}.

To solve this while preserving the current structure, I added a onKeyDown handler that listens for Enter and Space and invokes the modal.

Alternatively, I'm also exploring replacing the underlying element with a native <button> using asChild for improved semantics and cleaner handling if you suggest.

Really appreciate your input — Please, Let me know 🙏
Thanks!

@avivkeller
Copy link
Member

I'd rather convert it to an anchor or button, instead of adding redundant listeners

@MattIPv4
Copy link
Member

+1 this should be an actual semantic button

@Mohit5Upadhyay
Copy link
Contributor Author

Hi @avivkeller @MattIPv4 👋

I've updated the implementation to use a semantic <button> element instead of a non-interactive LinkWithArrow element for triggering the release details modal, as per your suggestion.

This change improves accessibility and removes the redundant event listener.

Let me know if anything else needs adjustment — happy to iterate!

@avivkeller
Copy link
Member

What if we, instead, change the LinkWithArrow component itself to be an a, regardless of whether there is a link, meaning this change will work on all cases?

@Mohit5Upadhyay
Copy link
Contributor Author

Thanks for the suggestion!, @avivkeller 🙌

change the LinkWithArrow component itself to be an a, regardless of whether there is a link, meaning this change will work on all cases?

Changing LinkWithArrow to always render an <a> tag — even when there's no href — won't fully or properly address accessibility concerns.

  • An <a> without an href is not keyboard-focusable by default
  • Requires manual handling of tabIndex, role="button", and keyboard events to behave like a button

That’s why it’s more robust to use the appropriate semantic element (<a> for links, <button> for actions).
Let me know what you think!

@avivkeller
Copy link
Member

Okay, so why don't we have it be a button where there isn't a link, and an a (anchor) otherwise? Regardless, this should be changed on the LinkWithArrow level

@Mohit5Upadhyay
Copy link
Contributor Author

Hi @avivkeller 👋

I've updated the LinkWithArrow component as suggested:

  • It now renders an <a> when href is provided
  • Falls back to a <button> otherwise
  • Handles both accessibility and semantics properly at the component level

Let me know if any further refinements are needed — happy to iterate! 🙌

Copy link
Member

@avivkeller avivkeller left a comment

Choose a reason for hiding this comment

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

Almost there!

@Mohit5Upadhyay
Copy link
Contributor Author

Hi @avivkeller ,

Can we do a props of "Link" vs props of "HTMLButtonType"?

Yes, this can be done like this

LinkWithArrowProps =
  | { kind: 'link'; props: AnchorHTMLAttributes<HTMLAnchorElement> }
  | { kind: 'button'; props: ButtonHTMLAttributes<HTMLButtonElement> };

but for that we have to refractor some files like DownloadLink.tsx , DetailsButton.tsx , ChangelogLink.tsx ,ReleaseCodeBox.tsx accordingly

//previous
<LinkWithArrow href={downloadLink}>{children}</LinkWithArrow>;

//updated
<LinkWithArrow
      kind="link"
      props={{ href: downloadLink }}
    >
      {children}
    </LinkWithArrow>

@avivkeller
Copy link
Member

That's not what I meant, I meant the type definition itself. If it's not possible, forget it

@Mohit5Upadhyay
Copy link
Contributor Author

Hi @avivkeller 👋,

Thanks for the clarification!

I misunderstood your suggested approach.

That's not what I meant, I meant the type definition itself. If it's not possible, forget it

Since you meant it’s fine to skip if not possible— I’ve kept the simpler & skip it.

All other previous changes are applied as ✅ suggested above.

Let me know if anything else is needed!

@avivkeller
Copy link
Member

@Mohit5Upadhyay May I push a commit to resolve some last minute things, then we are good to go?

@avivkeller avivkeller requested a review from a team as a code owner August 1, 2025 20:59
@MattIPv4
Copy link
Member

MattIPv4 commented Aug 1, 2025

conflicts :P

Copy link

codecov bot commented Aug 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.01%. Comparing base (8194265) to head (ee060b8).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8051      +/-   ##
==========================================
+ Coverage   72.98%   73.01%   +0.03%     
==========================================
  Files          95       95              
  Lines        8324     8324              
  Branches      215      214       -1     
==========================================
+ Hits         6075     6078       +3     
+ Misses       2248     2245       -3     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Details Button not keyboard accessible
3 participants