Skip to content

Comments

fix(a11y): use native button in MetricsScoreCircle#3437

Merged
kasya merged 4 commits intoOWASP:mainfrom
Shubb07:fix/a11y-metrics-score-circle
Jan 24, 2026
Merged

fix(a11y): use native button in MetricsScoreCircle#3437
kasya merged 4 commits intoOWASP:mainfrom
Shubb07:fix/a11y-metrics-score-circle

Conversation

@Shubb07
Copy link
Contributor

@Shubb07 Shubb07 commented Jan 20, 2026

Proposed change

Resolves #3421

This PR fixes a SonarCloud accessibility issue where a non-interactive

was being used as an interactive element in MetricsScoreCircle.

Non-native interactive elements should not be used with click and keyboard handlers, as this breaks semantic HTML and can cause accessibility issues. This change replaces the interactive div with a native when the component is clickable, while keeping a div for the non-interactive state.

This preserves the existing visual and functional behavior, improves keyboard and assistive-technology support, and resolves the SonarCloud rule typescript:S6848.

##Checklist

  • I followed the contributing workflow
  • I verified that my code works as intended and resolves the issue as described
  • I ran make check-test locally: all relevant checks executed (frontend lint/build passed; full pipeline run in WSL + Docker)
  • I used AI for code, documentation, tests, or communication related to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 20, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Improved accessibility by using native button semantics when the score display is interactive.
  • New Features

    • Added a visual pulse indicator for low scores (below 30).
  • Refactor

    • Component now renders either an interactive button or a static display; interactivity is opt-in and defaults to off, removing custom keyboard handlers.
  • Tests

    • Updated keyboard interaction tests to use higher-level user interactions.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

Renders a native HTML button when clickable and a non-interactive div otherwise; removes custom keyboard/ARIA handling; changes onClick prop type to accept button mouse events; adds default clickable = false and a visual pulse indicator when score < 30. (≤50 words)

Changes

Cohort / File(s) Summary
MetricsScoreCircle Component
frontend/src/components/MetricsScoreCircle.tsx
Change onClick prop from MouseEvent<HTMLDivElement> to MouseEvent<HTMLButtonElement>; add default clickable = false; remove internal handleClick, keyboard handlers, ARIA/role/tabIndex; conditionally render <button type="button"> when clickable or <div> otherwise; keep inner content; add pulse indicator for score < 30.
Unit Tests (keyboard interaction)
frontend/__tests__/unit/components/MetricsScoreCircle.test.tsx
Replace fireEvent keyDown usage with async userEvent keyboard simulation for Enter/Space; tests now use userEvent.setup() and awaited keyboard calls to validate keyboard activation on the rendered button.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • arkid15r
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main change: replacing a non-native interactive div with a native button element in MetricsScoreCircle to fix accessibility issues.
Description check ✅ Passed The PR description is related to the changeset and provides relevant context about the accessibility issue being resolved, the solution approach, and verification steps taken.
Linked Issues check ✅ Passed The code changes fully satisfy the requirements from issue #3421: the div has been replaced with a button when clickable, keyboard handlers are removed in favor of native button behavior, and the SonarCloud rule violation is resolved.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the PR objective. The modifications to MetricsScoreCircle component and its test file strictly address the accessibility issue without introducing unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 20, 2026
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 23, 2026
@codecov
Copy link

codecov bot commented Jan 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@204f833). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3437   +/-   ##
=======================================
  Coverage        ?   85.57%           
=======================================
  Files           ?      461           
  Lines           ?    14236           
  Branches        ?     1897           
=======================================
  Hits            ?    12182           
  Misses          ?     1679           
  Partials        ?      375           
Flag Coverage Δ
backend 84.47% <ø> (?)
frontend 88.62% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
frontend/src/components/MetricsScoreCircle.tsx 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 204f833...88d4c00. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

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

@Shubb07 this looks good on the UI.

Unit tests are now failing though! Please fix those to work with your changes

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 23, 2026
@Shubb07
Copy link
Contributor Author

Shubb07 commented Jan 23, 2026

Hey @kasya , I’ve updated the unit tests in MetricsScoreCircle.test.tsx to align with the new native button behavior. The keyboard interaction tests now use @testing-library/user-event instead of fireEvent to better reflect real user keyboard interactions (Enter and Space). This resolves the failing unit tests caused by the accessibility change. Please let me know if any further adjustments are needed.

@sonarqubecloud
Copy link

Copy link
Collaborator

@kasya kasya left a comment

Choose a reason for hiding this comment

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

@Shubb07 thanks for working on this! 👍🏼

The tests were still failing (could be for a different reason).

Image I fixed that failing test 👌🏼

Going forward, please make sure to run make check-test every time before pushing changes. This helps catch linting issues and failing tests early.

@kasya kasya enabled auto-merge January 24, 2026 02:57
@Shubb07
Copy link
Contributor Author

Shubb07 commented Jan 24, 2026

Hey @kasya , Thanks a lot for the review and guidance! I’ll make sure to run make check-test before pushing changes going forward. Appreciate your help.

@kasya kasya added this pull request to the merge queue Jan 24, 2026
Merged via the queue into OWASP:main with commit b177dfe Jan 24, 2026
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accessibility: Replace non-native interactive element with native button in MetricsScoreCircle

2 participants