fix(rome_js_analyze): False positives for interactive elements in useKeyWithClickEvents

crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs

@@ -1,10 +1,12 @@
 use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
 use rome_console::markup;
-use rome_js_syntax::{JsxAnyAttribute, JsxOpeningElement, JsxSelfClosingElement};
-use rome_rowan::{declare_node_union, AstNode, AstNodeList};
+use rome_js_syntax::{
+    JsxAnyAttribute, JsxAnyElementName, JsxAttributeList, JsxOpeningElement, JsxSelfClosingElement,
+};
+use rome_rowan::{declare_node_union, AstNode, SyntaxResult};
 
 declare_rule! {
-    /// Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `noKeyPress` keyboard event.
+    /// Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `onKeyPress` keyboard event.
     ///
     /// ## Examples
     ///
@@ -44,6 +46,10 @@ declare_rule! {
     /// ```jsx
     /// <div {...spread} onClick={() => {}} ></div>
     /// ```
+    ///
+    /// ```jsx
+    /// <button onClick={() => console.log("test")}>Submit</button>
+    /// ```
     pub(crate) UseKeyWithClickEvents {
         version: "10.0.0",
         name: "useKeyWithClickEvents",
@@ -56,22 +62,19 @@ declare_node_union! {
 }
 
 impl JsxAnyElement {
-    fn has_spread_attribute(&self) -> bool {
+    fn attributes(&self) -> JsxAttributeList {
         match self {
-            JsxAnyElement::JsxOpeningElement(element) => element
-                .attributes()
-                .iter()
-                .any(|attribute| matches!(attribute, JsxAnyAttribute::JsxSpreadAttribute(_))),
-            JsxAnyElement::JsxSelfClosingElement(element) => element
-                .attributes()
-                .iter()
-                .any(|attribute| matches!(attribute, JsxAnyAttribute::JsxSpreadAttribute(_))),
+            JsxAnyElement::JsxOpeningElement(element) => element.attributes(),
+            JsxAnyElement::JsxSelfClosingElement(element) => element.attributes(),
         }
     }
-}
 
-impl UseKeyWithClickEvents {
-    const REQUIRED_PROPS: [&'static str; 3] = ["onKeyDown", "onKeyUp", "onKeyPress"];
+    fn name(&self) -> SyntaxResult<JsxAnyElementName> {
+        match self {
+            JsxAnyElement::JsxOpeningElement(element) => element.name(),
+            JsxAnyElement::JsxSelfClosingElement(element) => element.name(),
+        }
+    }
 }
 
 impl Rule for UseKeyWithClickEvents {
@@ -81,64 +84,55 @@ impl Rule for UseKeyWithClickEvents {
     type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Self::Signals {
-        let node = ctx.query();
+        let element = ctx.query();
+
+        match element.name() {
+            Ok(JsxAnyElementName::JsxName(name)) => {
+                let element_name = name.value_token().ok()?.text_trimmed().to_lowercase();
 
-        match node {
-            JsxAnyElement::JsxOpeningElement(element) => {
-                let on_click_attribute = element.find_attribute_by_name("onClick").ok()?;
-                if element.name().ok()?.as_jsx_name().is_none()
-                    || on_click_attribute.is_none()
-                    || node.has_spread_attribute()
-                {
+                // Don't handle interactive roles
+                // TODO Support aria roles https://github.com/rome/tools/issues/3640
+                if matches!(
+                    element_name.as_str(),
+                    "button" | "checkbox" | "combobox" | "a" | "input"
+                ) {
                     return None;
                 }
+            }
+            _ => {
+                return None;
+            }
+        }
 
-                for attribute in element.attributes().into_iter() {
-                    if let JsxAnyAttribute::JsxAttribute(attribute) = attribute {
-                        let name = attribute
-                            .name()
-                            .ok()?
-                            .as_jsx_name()?
-                            .syntax()
-                            .text_trimmed()
-                            .to_string();
-
-                        if Self::REQUIRED_PROPS.contains(&name.as_str()) {
-                            return None;
-                        }
-                    }
-                }
+        let attributes = element.attributes();
+        let on_click_attribute = attributes.find_by_name("onClick").ok()?;
 
-                Some(())
-            }
-            JsxAnyElement::JsxSelfClosingElement(element) => {
-                let on_click_attribute = element.find_attribute_by_name("onClick").ok()?;
-                if element.name().ok()?.as_jsx_name().is_none()
-                    || on_click_attribute.is_none()
-                    || node.has_spread_attribute()
-                {
-                    return None;
-                }
+        #[allow(clippy::question_mark)]
+        if on_click_attribute.is_none() {
+            return None;
+        }
 
-                for attribute in element.attributes().into_iter() {
-                    if let JsxAnyAttribute::JsxAttribute(attribute) = attribute {
-                        let name = attribute
-                            .name()
-                            .ok()?
-                            .as_jsx_name()?
-                            .syntax()
-                            .text_trimmed()
-                            .to_string();
+        for attribute in attributes {
+            match attribute {
+                JsxAnyAttribute::JsxAttribute(attribute) => {
+                    let attribute_name = attribute.name().ok()?;
+                    let name = attribute_name.as_jsx_name()?;
+                    let name_token = name.value_token().ok()?;
 
-                        if Self::REQUIRED_PROPS.contains(&name.as_str()) {
-                            return None;
-                        }
+                    if matches!(
+                        name_token.text_trimmed(),
+                        "onKeyDown" | "onKeyUp" | "onKeyPress"
+                    ) {
+                        return None;
                     }
                 }
-
-                Some(())
+                JsxAnyAttribute::JsxSpreadAttribute(_) => {
+                    return None;
+                }
             }
         }
+
+        Some(())
     }
 
     fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {

crates/rome_js_analyze/src/utils/tests.rs

@@ -116,7 +116,7 @@ pub fn ok_find_attributes_by_name() {
         .filter_map(rome_js_syntax::JsxAttributeList::cast)
         .next()
         .unwrap();
-    let [a, c, d] = list.find_attributes_by_name(["a", "c", "d"]);
+    let [a, c, d] = list.find_by_names(["a", "c", "d"]);
     assert_eq!(
         a.unwrap()
             .initializer()

crates/rome_js_syntax/src/jsx_ext.rs

@@ -84,7 +84,7 @@ impl JsxOpeningElement {
         &self,
         name_to_lookup: &str,
     ) -> SyntaxResult<Option<JsxAttribute>> {
-        find_attribute_by_name(self.attributes(), name_to_lookup)
+        self.attributes().find_by_name(name_to_lookup)
     }
 
     /// It checks if current attribute has a trailing spread props
@@ -131,7 +131,8 @@ impl JsxOpeningElement {
     /// assert!(opening_element.has_trailing_spread_prop(div.clone()));
     /// ```
     pub fn has_trailing_spread_prop(&self, current_attribute: impl Into<JsxAnyAttribute>) -> bool {
-        has_trailing_spread_prop(self.attributes(), current_attribute)
+        self.attributes()
+            .has_trailing_spread_prop(current_attribute)
     }
 }
 
@@ -182,7 +183,7 @@ impl JsxSelfClosingElement {
         &self,
         name_to_lookup: &str,
     ) -> SyntaxResult<Option<JsxAttribute>> {
-        find_attribute_by_name(self.attributes(), name_to_lookup)
+        self.attributes().find_by_name(name_to_lookup)
     }
 
     /// It checks if current attribute has a trailing spread props
@@ -230,21 +231,22 @@ impl JsxSelfClosingElement {
     /// assert!(opening_element.has_trailing_spread_prop(div.clone()));
     /// ```
     pub fn has_trailing_spread_prop(&self, current_attribute: impl Into<JsxAnyAttribute>) -> bool {
-        has_trailing_spread_prop(self.attributes(), current_attribute)
+        self.attributes()
+            .has_trailing_spread_prop(current_attribute)
     }
 }
 
 impl JsxAttributeList {
-    /// Find and return the `JsxAttribute` that matches the given name like [find_attribute_by_name].  
-    /// Only attributes with name as [JsxName] can be returned.   
+    /// Finds and returns attributes `JsxAttribute` that matches the given names like [Self::find_by_name].
+    /// Only attributes with name as [JsxName] can be returned.
     ///
-    /// Each name of "names_to_lookup" should be unique.  
+    /// Each name of "names_to_lookup" should be unique.
     ///
-    /// Supports maximum of 16 names to avoid stack overflow. Eeach attribute will consume:
+    /// Supports maximum of 16 names to avoid stack overflow. Each attribute will consume:
     ///
     /// - 8 bytes for the [Option<JsxAttribute>] result;
     /// - plus 16 bytes for the [&str] argument.
-    pub fn find_attributes_by_name<const N: usize>(
+    pub fn find_by_names<const N: usize>(
         &self,
         names_to_lookup: [&str; N],
     ) -> [Option<JsxAttribute>; N] {
@@ -257,7 +259,7 @@ impl JsxAttributeList {
 
         let mut missing = N;
 
-        'attributes: for att in self.iter() {
+        'attributes: for att in self {
             if let Some(attribute) = att.as_jsx_attribute() {
                 if let Some(name) = attribute
                     .name()
@@ -282,40 +284,34 @@ impl JsxAttributeList {
 
         results
     }
-}
 
-pub fn find_attribute_by_name(
-    attributes: JsxAttributeList,
-    name_to_lookup: &str,
-) -> SyntaxResult<Option<JsxAttribute>> {
-    let attribute = attributes.iter().find_map(|attribute| {
-        let attribute = JsxAttribute::cast_ref(attribute.syntax())?;
-        let name = attribute.name().ok()?;
-        let name = JsxName::cast_ref(name.syntax())?;
-        if name.value_token().ok()?.text_trimmed() == name_to_lookup {
-            Some(attribute)
-        } else {
-            None
-        }
-    });
+    pub fn find_by_name(&self, name_to_lookup: &str) -> SyntaxResult<Option<JsxAttribute>> {
+        let attribute = self.iter().find_map(|attribute| {
+            let attribute = JsxAttribute::cast_ref(attribute.syntax())?;
+            let name = attribute.name().ok()?;
+            let name = JsxName::cast_ref(name.syntax())?;
+            if name.value_token().ok()?.text_trimmed() == name_to_lookup {
+                Some(attribute)
+            } else {
+                None
+            }
+        });
 
-    Ok(attribute)
-}
+        Ok(attribute)
+    }
 
-pub fn has_trailing_spread_prop(
-    attributes: JsxAttributeList,
-    current_attribute: impl Into<JsxAnyAttribute>,
-) -> bool {
-    let mut current_attribute_found = false;
-    let current_attribute = current_attribute.into();
-    for attribute in attributes {
-        if attribute == current_attribute {
-            current_attribute_found = true;
-            continue;
-        }
-        if current_attribute_found && attribute.as_jsx_spread_attribute().is_some() {
-            return true;
+    pub fn has_trailing_spread_prop(&self, current_attribute: impl Into<JsxAnyAttribute>) -> bool {
+        let mut current_attribute_found = false;
+        let current_attribute = current_attribute.into();
+        for attribute in self {
+            if attribute == current_attribute {
+                current_attribute_found = true;
+                continue;
+            }
+            if current_attribute_found && attribute.as_jsx_spread_attribute().is_some() {
+                return true;
+            }
         }
+        false
     }
-    false
 }

website/docs/src/lint/rules/index.md

@@ -57,7 +57,7 @@ Enforces the usage of the attribute <code>type</code> for the element <code>butt
 	<a href="/lint/rules/useKeyWithClickEvents">useKeyWithClickEvents</a>
 	<span class="recommended">recommended</span>
 </h3>
-Enforce to have the <code>onClick</code> mouse event with the <code>onKeyUp</code>, the <code>onKeyDown</code>, or the <code>noKeyPress</code> keyboard event.
+Enforce to have the <code>onClick</code> mouse event with the <code>onKeyUp</code>, the <code>onKeyDown</code>, or the <code>onKeyPress</code> keyboard event.
 </section>
 <section class="rule">
 <h3 data-toc-exclude id="useKeyWithMouseEvents">

website/docs/src/lint/rules/useKeyWithClickEvents.md

@@ -6,7 +6,7 @@ title: Lint Rule useKeyWithClickEvents
 
 > This rule is recommended by Rome.
 
-Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `noKeyPress` keyboard event.
+Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, or the `onKeyPress` keyboard event.
 
 ## Examples
 
@@ -71,3 +71,7 @@ Enforce to have the `onClick` mouse event with the `onKeyUp`, the `onKeyDown`, o
 <div {...spread} onClick={() => {}} ></div>
 ```
 
+```jsx
+<button onClick={() => console.log("test")}>Submit</button>
+```
+