fix(rome_js_analyze): False positives for interactive elements in `useKeyWithClickEvents`
MODIFY
crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs
crates/rome_js_analyze/src/analyzers/a11y/use_key_with_click_events.rs
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();
-
- 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()
- {
- return None;
- }
+ let element = ctx.query();
- 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();
+ match element.name() {
+ Ok(JsxAnyElementName::JsxName(name)) => {
+ let element_name = name.value_token().ok()?.text_trimmed().to_lowercase();
- if Self::REQUIRED_PROPS.contains(&name.as_str()) {
+ // 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;
}
-
- 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()
- {
+
+ let attributes = element.attributes();
+ let on_click_attribute = attributes.find_by_name("onClick").ok()?;
+
+ #[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()) {
+ if matches!(
+ name_token.text_trimmed(),
+ "onKeyDown" | "onKeyUp" | "onKeyPress"
+ ) {
return None;
}
}
+ JsxAnyAttribute::JsxSpreadAttribute(_) => {
+ return None;
}
-
- Some(())
}
}
+
+ Some(())
}
fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {

MODIFY
crates/rome_js_analyze/src/utils/tests.rs
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()

MODIFY
crates/rome_js_syntax/src/jsx_ext.rs
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].
+ /// 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.
///
- /// 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,13 +284,9 @@ 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| {
+ 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())?;
@@ -302,13 +300,10 @@ pub fn find_attribute_by_name(
Ok(attribute)
}
-pub fn has_trailing_spread_prop(
- attributes: JsxAttributeList,
- current_attribute: impl Into<JsxAnyAttribute>,
-) -> bool {
+ 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 attributes {
+ for attribute in self {
if attribute == current_attribute {
current_attribute_found = true;
continue;
@@ -319,3 +314,4 @@ pub fn has_trailing_spread_prop(
}
false
}
+}

MODIFY
website/docs/src/lint/rules/index.md
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">

MODIFY
website/docs/src/lint/rules/useKeyWithClickEvents.md
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>
+```
+

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