Skip to content

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 GitHub.

Project Deployment Preview Updated (UTC)
nodejs-org Ready Ready Preview Aug 24, 2025 1:40pm

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 75.88%. Comparing base (a9e90a2) to head (24bb7f7).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8051   +/-   ##
=======================================
  Coverage   75.88%   75.88%           
=======================================
  Files         112      112           
  Lines        9433     9433           
  Branches      303      303           
=======================================
  Hits         7158     7158           
  Misses       2274     2274           
  Partials        1        1           

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

@avivkeller avivkeller added the github_actions:pull-request Trigger Pull Request Checks label Aug 2, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Aug 2, 2025
Copy link
Contributor

github-actions bot commented Aug 2, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 96 🟢 100 🟢 100 🟢 100 🔗
/en/about 🟢 99 🟢 97 🟢 100 🟠 88 🔗
/en/about/previous-releases 🟢 99 🟢 93 🟢 100 🟢 100 🔗
/en/download 🟢 94 🟢 100 🟢 100 🟢 100 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 100 🔗

@ovflowd
Copy link
Member

ovflowd commented Aug 14, 2025

@avivkeller / @Mohit5Upadhyay there's a conflict, can any of you resolve it?

@Mohit5Upadhyay Mohit5Upadhyay force-pushed the fix/8048-keyboard-accessible-details-button branch from a4c05c8 to f185c88 Compare August 16, 2025 12:45
@Mohit5Upadhyay
Copy link
Contributor Author

Apologies, I think I may have broken something 😅.

@avivkeller
Copy link
Member

Apologies, I think I may have broken something 😅.

No worries! I can resolve the conflicts later today

@Mohit5Upadhyay
Copy link
Contributor Author

No worries! I can resolve the conflicts later today

Thanks @avivkeller for taking care of this 🙏. I apologize — I tried resolving the conflicts earlier but ended up breaking something and it got a bit messed up.
I’ll make sure to be more careful with conflict resolution next time.
Really appreciate your help in fixing it!

@avivkeller
Copy link
Member

Ack! I got swamped, ping me again if you need help, I'll be available.

@avivkeller avivkeller force-pushed the fix/8048-keyboard-accessible-details-button branch from f185c88 to d6954ef Compare August 24, 2025 13:25
@avivkeller avivkeller self-assigned this Aug 24, 2025
@avivkeller avivkeller force-pushed the fix/8048-keyboard-accessible-details-button branch from d6954ef to d7e7681 Compare August 24, 2025 13:33
@avivkeller avivkeller force-pushed the fix/8048-keyboard-accessible-details-button branch from d7e7681 to 37dd0bc Compare August 24, 2025 13:36
@avivkeller avivkeller added the github_actions:pull-request Trigger Pull Request Checks label Aug 24, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Aug 24, 2025
@avivkeller avivkeller added the github_actions:pull-request Trigger Pull Request Checks label Aug 24, 2025
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Aug 24, 2025
@avivkeller avivkeller enabled auto-merge August 24, 2025 13:43
@avivkeller avivkeller added this pull request to the merge queue Aug 24, 2025
Merged via the queue into nodejs:main with commit 2d2a01b Aug 24, 2025
15 checks passed
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
5 participants