refactor: add "button" role to clickable UI elements for improved accessibility

[eulloa10]

SUMMARY

Clickable UI elements lacking <a> or <button> tags, or the role="button" attribute, weren't semantically clear as clickable elements when using a screen reader.

To improve accessibility, I reviewed the React codebase for onClick handlers. When encountering HTML elements with onClick handlers AND without <a> or <button> tags, I added the role="button" attribute to identify them as clickable.

This approach targets the most immediate and straightforward cases, ensuring that elements triggered by onClick handlers are now clearly identified as clickable for users relying on screen readers.

TESTING INSTRUCTIONS

Testing options include:

• Using a screen reader: identify and navigate through the clickable components and ensure they are announced correctly
• Using browser devtools: inspect clickable elements and verify that elements without <a> or <button> tags have the role="button" attribute.

ADDITIONAL INFORMATION

• [x] Has associated issue: #26190
• [ ] Required feature flags:
• [ ] Changes UI
• [ ] Includes DB Migration (follow approval process in SIP-59)
  • [ ] Migration is atomic, supports rollback & is backwards-compatible
  • [ ] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [ ] Introduces new feature or API
• [ ] Removes existing feature or API

[john-bodley]

Thank @eulloa10 for the change. I've kicked off CI, though it seems you have a couple of conflicts you need to resolve.

[eulloa10]

Thanks @john-bodley. I resolved the conflicts, hope it's successful this time around.

[eschutho]

/testenv up

[github-actions[bot]]

@eschutho Ephemeral environment spinning up at http://35.162.104.150:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[eulloa10]

Thank you for reviewing @justinpark. I made the requested changes and they are ready to be reviewed.

[eschutho]

/testenv up

[github-actions[bot]]

@eschutho Ephemeral environment spinning up at http://34.222.59.163:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[eulloa10]

I am looking into the failing test cases.

[geido]

Thanks for this @eulloa10. Let me know if I can help with the failing test cases

[eulloa10]

Thanks @geido. I looked through the failing unit tests (17 tests) to start, but I am confused on how the changes I made are contributing to the failing tests. What is the best way to approach debugging these?

[geido]

@eulloa10 both Cypress tests and unit tests are failing as you changed some of the roles associated to elements. Please check the errors individually and update the tests according to the new roles. If you need more help you can find me in Apache Superset Slack

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.

[mistercrunch]

Is there an opportunity as part of this PR to improve accessibility by reusing (or introducing if they don't exist already) low-level reusable components in src/components and make those accessible?

The original idea with src/components was [amongst other things] to offer a subset of antd (and native React like <a> or button) components that are more opinionated on properties (offer only a subset of properties) and used/reused consistently across Superset.

Say if we always use src.components.Button and that one always has a role, then no need to sprinkle role in a hundred places (?) Similar with antd.MenuItem, if we have a nice little abstraction where you can't get it wrong, then no need to go low level across the codebase, simply in src/components/

---

If this PR gets bit and large and merge-conflict-heavy, a different approach would be to do one PR by component-type (or a group of them, say all menu-related ones) and introduce the same src/components/Menu, prevent accessibility mis-use at that level, and reuse it across the codebase.

[mistercrunch]

Similarly, looking at all the many <Menu /> and <MenuItem /> entries here, you could really just force role="button" if/when onClick is defined centrally in src/components/Menu/, and not have to sprinkle role="button" anywhere else. We should have a hard rule to ALWAYS use antd component through the wrappers in src/components/ meaning we always do this lower level stuff from that central point.

[eulloa10]

@mistercrunch thank you, appreciate the additional context here and feedback. I am going to use your approach and also split the changes into multiple PRs.

[mistercrunch]

The beauty is that a single PR handling both Menu and MenuItem might be fairly simple and tackle a lot of the changes here in a very simple way.

[rusackas]

Agreed with all the comments... I appreciate this PR and all moves toward accessibility. We just need to fix it as close to the root as we can.

[mistercrunch]

[eulloa10]

@geido I pushed my latest changes, but there are still more to come. Additionally, some of the tests are failing, so I am going to look into those and update.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 67.49%. Comparing base (ad7bd09) to head (312d3ad). Report is 29 commits behind head on master.

Files	Patch %	Lines
superset-frontend/src/components/Icons/Icon.tsx	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #26602      +/-   ##
==========================================
+ Coverage   67.46%   67.49%   +0.02%     
==========================================
  Files        1910     1910              
  Lines       74802    74821      +19     
  Branches     8345     8358      +13     
==========================================
+ Hits        50468    50498      +30     
+ Misses      22283    22273      -10     
+ Partials     2051     2050       -1     

Flag	Coverage Δ
javascript	57.44% <83.33%> (+0.04%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[geido]

/testenv up

[github-actions[bot]]

@geido Ephemeral environment spinning up at http://34.218.74.195:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.