feat(html_analyze): implement noStaticElementInteractions#10069
Conversation
🦋 Changeset detectedLatest commit: 6a7427d The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
f32df0c to
fff0434
Compare
Merging this PR will degrade performance by 69.55%
Performance Changes
Comparing 
|
WalkthroughAdds a new HTML accessibility lint rule Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_html_analyze/src/lint/a11y/no_static_element_interactions.rs`:
- Around line 104-128: The INTERACTIVE_HANDLERS constant is missing "onmouseup",
causing static-element interaction checks (and Vue directive resolution for
`@mouseup` / v-on:mouseup) to miss those cases; update the INTERACTIVE_HANDLERS
array to include "onmouseup" alongside "onmousedown" so the lint in
no_static_element_interactions (which relies on INTERACTIVE_HANDLERS) correctly
flags mouseup handlers in HTML and Vue fixtures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fec3251b-9d2a-4c67-8549-32f5d74afae0
⛔ Files ignored due to path filters (8)
crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/astro/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/svelte/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/vue/valid.vue.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (10)
.changeset/common-toes-trade.mdcrates/biome_html_analyze/src/lint/a11y/no_static_element_interactions.rscrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/astro/valid.astrocrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/svelte/valid.sveltecrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/valid.htmlcrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/vue/valid.vue
dyc3
left a comment
There was a problem hiding this comment.
Lets try to reduce the perf regressions if possible.
| fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { | ||
| let node = ctx.query(); | ||
| Some(RuleDiagnostic::new( | ||
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| "Static Elements should not be interactive." | ||
| }, | ||
| ).note( | ||
| markup! { | ||
| "To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value." | ||
| } | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
"Static Elements should not be interactive." doesn't really illustrate why. remember the rule pillars. A good rule of thumb is if you don't have 3 markup! notes, then you are missing something.
There was a problem hiding this comment.
Copied it over from the JS rule, included the improvement in this PR. Both rules I touched in this PR
There was a problem hiding this comment.
The other rules are old, and rule pillars weren't a thing. There's a good opportunity to improve things
| "onkeyup", | ||
| "onfocus", | ||
| "onblur", | ||
| ]; |
There was a problem hiding this comment.
Add a unit test to ensure this is sorted, then use binary search. to do lookups in this list. That's likely the cause of the perf regression.
There was a problem hiding this comment.
Added and abstracted the event handlers bit to the aria crate (which I first planned in a later PR)
| fn has_handler_props(element: &AnyHtmlTagElement) -> bool { | ||
| INTERACTIVE_HANDLERS.iter().any(|handler| { | ||
| element.find_attribute_by_name(handler).is_some() | ||
| || handler.strip_prefix("on").is_some_and(|handler_name| { | ||
| element | ||
| .find_vue_event_handling_directive(handler_name) | ||
| .is_some() | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
iterate on the attributes in the outer loop, do lookups in the list on the inside.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_html_analyze/src/lint/a11y/no_noninteractive_element_interactions.rs`:
- Around line 129-130: The EVENT_HANDLER_TYPES constant currently includes
"image", which causes the rule in no_noninteractive_element_interactions
(constant EVENT_HANDLER_TYPES) to treat passive image handlers like
onload/onerror as interaction events and trigger false positives; remove "image"
from the slice so the array only contains "focus", "keyboard", and "mouse"
(i.e., update EVENT_HANDLER_TYPES to &["focus", "keyboard", "mouse"]) and
run/update any tests or references that assume the former set.
In
`@crates/biome_js_analyze/src/lint/a11y/no_noninteractive_element_interactions.rs`:
- Around line 135-136: The EVENT_HANDLER_TYPES constant currently includes
"image", which causes lifecycle handlers like onLoad/onError to be treated as
interactive; update the constant in no_noninteractive_element_interactions.rs to
remove the "image" entry so it only contains "focus", "keyboard", and "mouse"
(i.e., change EVENT_HANDLER_TYPES to &[&str] = &["focus", "keyboard", "mouse"])
to align the rule with its mouse/key handler intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28437db2-38fe-4193-a60d-55356e694d37
⛔ Files ignored due to path filters (11)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveElementInteractions/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveElementInteractions/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveElementInteractions/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noNoninteractiveElementInteractions/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noStaticElementInteractions/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/a11y/noNoninteractiveElementInteractions/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/a11y/noStaticElementInteractions/invalid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (10)
crates/biome_aria/Cargo.tomlcrates/biome_aria/src/event_handlers.rscrates/biome_aria/src/lib.rscrates/biome_html_analyze/src/a11y.rscrates/biome_html_analyze/src/lint/a11y/no_noninteractive_element_interactions.rscrates/biome_html_analyze/src/lint/a11y/no_static_element_interactions.rscrates/biome_html_syntax/src/element_ext.rscrates/biome_js_analyze/src/a11y.rscrates/biome_js_analyze/src/lint/a11y/no_noninteractive_element_interactions.rscrates/biome_js_analyze/src/lint/a11y/no_static_element_interactions.rs
💤 Files with no reviewable changes (1)
- crates/biome_html_syntax/src/element_ext.rs
✅ Files skipped from review due to trivial changes (2)
- crates/biome_aria/Cargo.toml
- crates/biome_aria/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_html_analyze/src/lint/a11y/no_static_element_interactions.rs
Perf regression are false positives. I doubt this PR has anything to do with CSS |
| } | ||
|
|
||
| // Only check the focus, keyboard and mouse event handler types. | ||
| const EVENT_HANDLER_TYPES: &[&str] = &["focus", "keyboard", "mouse"]; |
There was a problem hiding this comment.
its not worth a binary search if there's so few elements. binary_search only starts to help performance at ~20 items, give or take
There was a problem hiding this comment.
I don't do a binary search on these, only on the keys & within the values array of the EVENT_HANDLERS
| .is_some_and(|t| { | ||
| matches_event_handler( | ||
| handler_types, | ||
| format!("on{}", t.text_trimmed()).as_str(), |
There was a problem hiding this comment.
format! results in string allocations.
| .is_some_and(|t| { | ||
| matches_event_handler( | ||
| handler_types, | ||
| format!("on{}", t.text_trimmed()).as_str(), |
| // https://github.com/jsx-eslint/jsx-ast-utils/blob/v3.3.5/src/eventHandlers.js | ||
| const EVENT_HANDLERS: &[(&str, &[&str])] = &[ | ||
| ( | ||
| "animation", | ||
| &["onanimationend", "onanimationiteration", "onanimationstart"], | ||
| ), | ||
| ("clipboard", &["oncopy", "oncut", "onpaste"]), | ||
| ( | ||
| "composition", | ||
| &[ | ||
| "oncompositionend", | ||
| "oncompositionstart", | ||
| "oncompositionupdate", | ||
| ], | ||
| ), | ||
| ("focus", &["onblur", "onfocus"]), |
There was a problem hiding this comment.
On one hand, I can see why you did this refactor. But I don't think its going to perform very well. Now, instead of doing one lookup into a list, you are doing 2 (look up the key, then check the values) multiplied by how many categories you are looking at.
There was a problem hiding this comment.
On one hand, I can see why you did this refactor.
Mainly dragging it to the aria crate, the map was already used in noStaticElementInteractions, so dragged it over. If you have any suggestion, I am open to change :)
There was a problem hiding this comment.
In the short term, I would rather the rules just have one list they binary search. You could strip the on prefixes from them and do something like text.starts_with("on") && text[2..].binary_search() to be able to deal with the vue directives cases, and avoid the format!s.
There was a problem hiding this comment.
But that would mean, 4-6 rules with an array to maintain, instead of 1 single source of truth. It's just that some require a few extra event handlers than others. Doesn't that also increase unnecessarily the binary?
There was a problem hiding this comment.
Yes that's true, but a better refactor would be a macro or something so we can filter the lists at compile time instead of at run time. Or maybe we could use bitflags in some way to apply filters. It's something I'd rather have in a different PR so it doesn't block this rule getting merged, and to better see what the benchmarks say about it.
And regarding binary size, best case the compiler sees the same strings and deduplicates them, worst case we increase the binary size by a kilobyte tops. Probably doesn't matter all that much.
There was a problem hiding this comment.
text[2..].binary_search()to be able to deal with the vue directives cases, and avoid the format!s.
Huh? But wouldn't you then binary search through the string? (if that's even possible) Instead of searching through an array for the value
Refactoring the list stuff in another PR sounds good
There was a problem hiding this comment.
sorry i meant list.binary_search(text[2..]). i was trying to illustrate how you could deal with the vue directives.
|
That's rule @Netail , so I think you update the description in |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_aria/src/event_handlers.rs`:
- Around line 98-105: Add behavioral matcher tests alongside the existing
ordering checks to assert actual matching behavior for different casings and
prefix forms; create a new test (or extend test_order) that looks up handlers in
EVENT_HANDLERS (the EVENT_HANDLERS map/vec and its matcher logic) for strings
like "onclick", "onClick", "ONCLICK", and "click" and assert the expected
handler is returned (or that matching succeeds/fails as intended) for each
variant; reference the EVENT_HANDLERS symbol and any matching function used by
the runtime so the test exercises the same lookup/matching code path and
prevents regressions in prefix/casing handling.
- Around line 82-85: The code strips an "on" prefix using starts_with("on")
which misses HTML-style attributes like "OnClick"/"ONCLICK"; change the prefix
check to be case-insensitive (e.g. check the first two bytes/chars with
eq_ignore_ascii_case("on") or use a case-insensitive substring compare) before
slicing event_handler, then compute event_handler_lower =
event_handler.to_lowercase_cow() as before; update the logic around the
event_handler variable in event_handlers.rs (the starts_with("on") check and
subsequent slicing) so any case variant of "on" is removed correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e64672ac-9631-4c57-9e4d-0a337cd7b292
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lockand included by**
📒 Files selected for processing (2)
crates/biome_aria/src/event_handlers.rscrates/biome_html_analyze/src/a11y.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_html_analyze/src/a11y.rs
| #[test] | ||
| fn test_order() { | ||
| assert!(EVENT_HANDLERS.is_sorted_by_key(|(k, _)| *k)); | ||
|
|
||
| for (_, v) in EVENT_HANDLERS.iter() { | ||
| assert!(v.is_sorted()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add behavioural matcher tests, not only ordering checks.
Nice guard on sorting. Add direct assertions for prefix/casing paths (onclick, onClick, ONCLICK, click) so this doesn’t regress again.
Suggested test shape
#[test]
fn test_order() {
assert!(EVENT_HANDLERS.is_sorted_by_key(|(k, _)| *k));
for (_, v) in EVENT_HANDLERS.iter() {
assert!(v.is_sorted());
}
}
+
+#[test]
+fn test_matches_event_handler_normalisation() {
+ let mouse = &["mouse"];
+
+ assert!(matches_event_handler(mouse, "click"));
+ assert!(matches_event_handler(mouse, "onclick"));
+ assert!(matches_event_handler(mouse, "onClick"));
+ assert!(matches_event_handler(mouse, "ONCLICK"));
+ assert!(!matches_event_handler(mouse, "onnotarealevent"));
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_aria/src/event_handlers.rs` around lines 98 - 105, Add
behavioral matcher tests alongside the existing ordering checks to assert actual
matching behavior for different casings and prefix forms; create a new test (or
extend test_order) that looks up handlers in EVENT_HANDLERS (the EVENT_HANDLERS
map/vec and its matcher logic) for strings like "onclick", "onClick", "ONCLICK",
and "click" and assert the expected handler is returned (or that matching
succeeds/fails as intended) for each variant; reference the EVENT_HANDLERS
symbol and any matching function used by the runtime so the test exercises the
same lookup/matching code path and prevents regressions in prefix/casing
handling.
54636c8 to
461e748
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_aria/src/event_handlers.rs (1)
99-106:⚠️ Potential issue | 🟡 MinorPlease add behavioural tests, not only ordering checks.
Line 99-Line 106 only proves sortedness; it doesn’t protect actual matching behaviour. Please add direct assertions for
click,onclick,onClick,ONCLICK, and a negative case so normalisation doesn’t regress again.Proposed test addition
#[test] fn test_order() { assert!(EVENT_HANDLERS.is_sorted_by_key(|(k, _)| *k)); for (_, v) in EVENT_HANDLERS.iter() { assert!(v.is_sorted()); } } + +#[test] +fn test_matches_event_handler_normalisation() { + let mouse = &["mouse"]; + + assert!(matches_event_handler(mouse, "click")); + assert!(matches_event_handler(mouse, "onclick")); + assert!(matches_event_handler(mouse, "onClick")); + assert!(matches_event_handler(mouse, "ONCLICK")); + assert!(!matches_event_handler(mouse, "onnotarealevent")); +}As per coding guidelines, “All code changes MUST include appropriate tests: lint rules require snapshot tests in
tests/specs/{group}/{rule}/... and bug fixes require tests that reproduce and validate the fix.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_aria/src/event_handlers.rs` around lines 99 - 106, The current test_order only checks sortedness; add a behavioural test (e.g., new fn test_event_normalisation or extend test_order) that asserts EVENT_HANDLERS contains a handler for the normalized event key and that different casings map to the same entry: check lookups for "click", "onclick", "onClick", and "ONCLICK" all resolve to the same handler value from EVENT_HANDLERS (use the same key-normalisation used by insertion/lookup code), and include a negative assertion that a non-matching variant (e.g., "on-click" or "clicker") is not present. Reference EVENT_HANDLERS and the test function name (test_order) so the behaviour checks live alongside the ordering assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_aria/src/event_handlers.rs`:
- Around line 99-106: The current test_order only checks sortedness; add a
behavioural test (e.g., new fn test_event_normalisation or extend test_order)
that asserts EVENT_HANDLERS contains a handler for the normalized event key and
that different casings map to the same entry: check lookups for "click",
"onclick", "onClick", and "ONCLICK" all resolve to the same handler value from
EVENT_HANDLERS (use the same key-normalisation used by insertion/lookup code),
and include a negative assertion that a non-matching variant (e.g., "on-click"
or "clicker") is not present. Reference EVENT_HANDLERS and the test function
name (test_order) so the behaviour checks live alongside the ordering
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f0f2e57-dd54-4eca-9572-576b917ecac9
📒 Files selected for processing (2)
crates/biome_aria/src/event_handlers.rscrates/biome_html_analyze/src/a11y.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_html_analyze/src/a11y.rs
Summary
Port noStaticElementInteractions to HTML
Closes #8155
Test Plan
unit tests
Docs