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

fix(js_analyze): skip noAutofocus check if eligible #4475

Merged
merged 3 commits into from
Nov 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 121 additions & 4 deletions crates/biome_js_analyze/src/lint/a11y/no_autofocus.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use crate::JsRuleAction;
use biome_analyze::{
context::RuleContext, declare_lint_rule, Ast, FixKind, Rule, RuleDiagnostic, RuleSource,
context::RuleContext, declare_lint_rule, FixKind, Phases, QueryMatch, Queryable, Rule,
RuleDiagnostic, RuleSource, Visitor,
};
use biome_console::markup;
use biome_js_syntax::{jsx_ext::AnyJsxElement, JsxAttribute};
use biome_rowan::{AstNode, BatchMutationExt};
use biome_js_syntax::{jsx_ext::AnyJsxElement, JsLanguage, JsSyntaxKind, JsxAttribute};
use biome_rowan::{AstNode, BatchMutationExt, WalkEvent};
use biome_string_case::StrOnlyExtension;

declare_lint_rule! {
/// Enforce that autoFocus prop is not used on elements.
Expand Down Expand Up @@ -50,6 +52,16 @@ declare_lint_rule! {
/// <MyComponent autoFocus={true} />
///```
///
/// ```jsx
/// // `autoFocus` prop in element has `popover` attribute is valid
seeintea marked this conversation as resolved.
Show resolved Hide resolved
/// <div popover><input autoFocus /></div>
/// ```
///
/// ```jsx
/// // `autoFocus` prop in `dialog` is valid
/// <dialog><input autoFocus /></dialog>
/// ```
///
/// ## Resources
///
/// - [WHATWG HTML Standard, The autofocus attribute](https://html.spec.whatwg.org/multipage/interaction.html#attr-fe-autofocus)
Expand All @@ -65,8 +77,113 @@ declare_lint_rule! {
}
}

fn find_kept_autofocus_mark(element: &AnyJsxElement) -> bool {
// the check for no_autofocus can be ignored
// 1. inside the `dialog` element
// 2. inside the element with the popover attribute

let is_dialog_element = match element.name_value_token() {
Some(syntax_token) => {
let tag_name = String::from(syntax_token.text_trimmed());
tag_name.to_lowercase_cow() == "dialog"
}
None => false,
};

let has_popover_attr = element.has_truthy_attribute("popover");

is_dialog_element || has_popover_attr
}

#[derive(Default)]
struct ValidAutofocusVisitor {
stack: Vec<(AnyJsxElement, bool)>,
}

impl Visitor for ValidAutofocusVisitor {
type Language = JsLanguage;

fn visit(
&mut self,
event: &biome_rowan::WalkEvent<biome_rowan::SyntaxNode<Self::Language>>,
mut ctx: biome_analyze::VisitorContext<Self::Language>,
) {
match event {
WalkEvent::Enter(node) => {
let kind = node.kind();

match kind {
JsSyntaxKind::JSX_OPENING_ELEMENT => {
let element = AnyJsxElement::unwrap_cast(node.clone());

let is_hold = match self.stack.last() {
None => false,
Some((_, value)) => *value,
};

if is_hold {
self.stack.push((element.clone(), true));
} else {
let next_hold = find_kept_autofocus_mark(&element);
self.stack.push((element.clone(), next_hold));
}

ctx.match_query(ValidAutofocus(element));
}
JsSyntaxKind::JSX_SELF_CLOSING_ELEMENT => {
let element = AnyJsxElement::unwrap_cast(node.clone());

let is_hold = match self.stack.last() {
None => false,
Some((_, value)) => *value,
};

if !is_hold {
ctx.match_query(ValidAutofocus(element));
}
}
JsSyntaxKind::JSX_CLOSING_ELEMENT => {
self.stack.pop();
}
_ => {}
}
}
WalkEvent::Leave(_) => {}
};
}
}

pub struct ValidAutofocus(AnyJsxElement);

impl QueryMatch for ValidAutofocus {
fn text_range(&self) -> biome_rowan::TextRange {
self.0.range()
}
}

impl Queryable for ValidAutofocus {
type Input = Self;

type Output = AnyJsxElement;

type Language = JsLanguage;

type Services = ();

fn build_visitor(
analyzer: &mut impl biome_analyze::AddVisitor<Self::Language>,
_: &<Self::Language as biome_rowan::Language>::Root,
) {
analyzer.add_visitor(Phases::Syntax, ValidAutofocusVisitor::default);
}

fn unwrap_match(_: &biome_analyze::ServiceBag, query: &Self::Input) -> Self::Output {
query.0.clone()
}
}

impl Rule for NoAutofocus {
type Query = Ast<AnyJsxElement>;
type Query = ValidAutofocus;
type State = JsxAttribute;
type Signals = Option<Self::State>;
type Options = ();
Expand Down
2 changes: 1 addition & 1 deletion crates/biome_js_analyze/tests/quick_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use std::{ffi::OsStr, fs::read_to_string, path::Path, slice};
#[ignore]
#[test]
fn quick_test() {
let input_file = Path::new("tests/specs/nursery/useSortedClasses/issue.jsx");
let input_file = Path::new("tests/specs/a11y/noAutofocus/invalid.jsx");
let file_name = input_file.file_name().and_then(OsStr::to_str).unwrap();

let (group, rule) = parse_test_path(input_file);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@
<div autoFocus />
<div autoFocus={true} />
<div autoFocus={false} />
</>
<div popover={false}><input autoFocus /></div>
</>
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ expression: invalid.jsx
<div autoFocus />
<div autoFocus={true} />
<div autoFocus={false} />
<div popover={false}><input autoFocus /></div>
</>

```

# Diagnostics
Expand Down Expand Up @@ -231,7 +231,7 @@ invalid.jsx:12:10 lint/a11y/noAutofocus FIXABLE ━━━━━━━━━━
> 12 │ <div autoFocus={true} />
│ ^^^^^^^^^^^^^^^^
13 │ <div autoFocus={false} />
14 │ </>
14 │ <div popover={false}><input autoFocus /></div>

i Unsafe fix: Remove the autoFocus attribute.

Expand All @@ -249,8 +249,8 @@ invalid.jsx:13:10 lint/a11y/noAutofocus FIXABLE ━━━━━━━━━━
12 │ <div autoFocus={true} />
> 13 │ <div autoFocus={false} />
│ ^^^^^^^^^^^^^^^^^
14 │ </>
15 │
14 │ <div popover={false}><input autoFocus /></div>
15 │ </>

i Unsafe fix: Remove the autoFocus attribute.

Expand All @@ -259,4 +259,20 @@ invalid.jsx:13:10 lint/a11y/noAutofocus FIXABLE ━━━━━━━━━━

```

```
invalid.jsx:14:33 lint/a11y/noAutofocus FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Avoid the autoFocus attribute.

12 │ <div autoFocus={true} />
13 │ <div autoFocus={false} />
> 14 │ <div popover={false}><input autoFocus /></div>
│ ^^^^^^^^^
15 │ </>

i Unsafe fix: Remove the autoFocus attribute.

14 │ ····<div·popover={false}><input·autoFocus·/></div>
│ ----------

```
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@
<div autofocus />
<input autofocus="true" />
<MyComponent autoFocus={true} />
</>
<div popover><input autoFocus />></div>
<dialog><input autoFocus />></dialog>
</>
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ expression: valid.jsx
<div autofocus />
<input autofocus="true" />
<MyComponent autoFocus={true} />
<div popover><input autoFocus />></div>
<dialog><input autoFocus />></dialog>
</>

```