From 4a579dec0eba35676fee287de3ad52c7971cc3c8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 10 Nov 2022 11:08:28 +0100 Subject: [PATCH] fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents` 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. --- .../a11y/use_key_with_click_events.rs | 117 +++++++++--------- crates/rome_js_syntax/src/jsx_ext.rs | 75 +++++------ .../src/lint/rules/useKeyWithClickEvents.md | 4 + 3 files changed, 98 insertions(+), 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 index e467993ac4d8..4f3c731f5212 100644 --- 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 ///
{}} >
/// ``` + /// + /// ```jsx + /// + /// ``` 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 { + 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::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::State) -> Option { diff --git a/crates/rome_js_syntax/src/jsx_ext.rs b/crates/rome_js_syntax/src/jsx_ext.rs index e7876397df35..476e4907e81d 100644 --- 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> { - 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) -> 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> { - 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) -> 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> { - 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> { + 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, -) -> 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) -> 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 index d6ce6fcd7928..c8e4d31186b6 100644 --- 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
{}} >
``` +```jsx + +``` +