fix(rome_js_analyze): False positives for interactive elements in `us…

…eKeyWithClickEvents` Interactive elements like `button`, `a`, `input` etc. don't need key events as they are supported by the browser. This is a simplified implementation and the rule should also consider if the element is hidden for screen readers but that's out of the scope of this fix. See #3640 ## Test Plan I added a new test that verifies that `button` no longer gets flagged.
rome · Nov 10, 2022 · 4a579de · 4a579de
1 parent 1c08b8f
commit 4a579de
Show file tree

Hide file tree

Showing 3 changed files with 98 additions and 98 deletions.
diff --git a/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs b/crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs
@@ -1,7 +1,9 @@
 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.
@@ -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,56 @@ 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 on_click_attribute = element
+            .attributes()
+            .find_attribute_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;
-                }
+        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 element.attributes().into_iter() {
+            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> {

diff --git a/crates/rome_js_syntax/src/jsx_ext.rs b/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_attribute_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_attribute_by_name(name_to_lookup)
     }
 
     /// It checks if current attribute has a trailing spread props
@@ -230,15 +231,16 @@ 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.   
+    /// Find and return the `JsxAttribute` that matches the given name like [find_attribute_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:
     ///
@@ -284,38 +286,37 @@ impl JsxAttributeList {
     }
 }
 
-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
-        }
-    });
+impl JsxAttributeList {
+    pub fn find_attribute_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
 }
diff --git a/website/docs/src/lint/rules/useKeyWithClickEvents.md b/website/docs/src/lint/rules/useKeyWithClickEvents.md
@@ -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>
+```
+