Skip to content
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

feat(linter): add noNoninteractiveElementInteractions rule #4358

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

Conversation

tunamaguro
Copy link
Contributor

@tunamaguro tunamaguro commented Oct 21, 2024

Summary

Implement jsx-a11y/no-noninteractive-element-interactions

Close #3936

Test Plan

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Oct 21, 2024
Copy link

codspeed-hq bot commented Oct 21, 2024

CodSpeed Performance Report

Merging #4358 will not alter performance

Comparing tunamaguro:impl-noNoninteractiveElementInteractions (df9cbe0) with main (19abdcf)

Summary

✅ 99 untouched benchmarks

@tunamaguro tunamaguro force-pushed the impl-noNoninteractiveElementInteractions branch from 02a8329 to 5d53042 Compare October 22, 2024 12:26

/// List of `widget` role
/// https://www.w3.org/TR/wai-aria-1.2/#widget
const INTERACTIVE_ROLES: &[&str] = &[
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be coming from our biome_aria crate. Preferably generated from the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review!
I’ve updated the code to use biome_aria as suggested (df9cbe0).
However, I noticed a difference in the test results after this change.
For example, the radiogroup role, which inherits from select and should be interactive, is now being treated as non-interactive.

         54 │+# Diagnostics
         55 │+```
         56 │+valid.jsx:49:5 lint/nursery/noNoninteractiveElementInteractions ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
         57 │+
         58 │+  ! Non-interactive element should not have event handler.
         59 │+  
         60 │+  > 49 │     <div role="radiogroup" onClick={() => { }} />
         61 │+       │     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
         62 │+    50 │ </>
         63 │+  
         64 │+  i Consider replace semantically interactive element like <button/> or <a href/>.
         65 │+  
         66 │+

According to WAI-ARIA 1.1 and 1.2, radiogroup inherits from select, but role.rs indicates inheritance from range.

define_role! {
/// https://www.w3.org/TR/wai-aria-1.1/#radiogroup
RadiogroupRole {
PROPS: [("aria-readonly", false), ("aria-required", false)],
ROLES: ["range"],
}
}

https://www.w3.org/TR/wai-aria-1.1/#radiogroup
https://www.w3.org/TR/wai-aria-1.2/#radiogroup

I am not familiar with accessibility, so I apologize if there is any misunderstanding.
Could you please advise me on how to proceed?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably a bug, right? You could try to add the missing roles and see if that fixes the issue?

Copy link
Contributor Author

@tunamaguro tunamaguro Oct 23, 2024

Choose a reason for hiding this comment

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

I manually added the widget role and the radiogroup is no longer reported.

--- a/crates/biome_aria/src/roles.rs
+++ b/crates/biome_aria/src/roles.rs
@@ -488,7 +488,7 @@ define_role! {
     /// https://www.w3.org/TR/wai-aria-1.1/#radiogroup
     RadiogroupRole {
         PROPS: [("aria-readonly", false), ("aria-required", false)],
-        ROLES: ["range"],
+        ROLES: ["range","select","composite","widget","group"],
     }
 }
Expression: valid.jsx
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
  44    44 │     <div role="textbox" onClick={() => { }} />
  45    45 │ 
  46    46 │     {/* Presentation role */}
  47    47 │     <div role="presentation" onClick={() => { }} />
        48 │+
        49 │+
        50 │+    <div role="radiogroup" onClick={() => { }} />
  48    51 │ </>
  49    52 │ ```

If it's not my implementation error, I think it's a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement jsx-a11y/no-noninteractive-element-interactions
3 participants