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

feat(lint/noStaticElementInteractions): add rule #2981

Open
wants to merge 35 commits into
base: main
Choose a base branch
from

Conversation

ryo-ebata
Copy link

@ryo-ebata ryo-ebata commented May 25, 2024

Summary

Enforce that non-interactive, visible elements (such as <div>) that have click handlers use the role attribute.

Implements #527

Test Plan

Added snaps for valid/invalid cases.

Basically, it conforms to the test cases of eslint-plugin-jsx-a11y/no-static-element-interactions, and test cases that cannot be handled by the resources within Biome are handled individually.

ref:
https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/main/__tests__/src/rules/no-static-element-interactions-test.js

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis A-CLI Area: CLI labels May 25, 2024
Copy link

codspeed-hq bot commented May 25, 2024

CodSpeed Performance Report

Merging #2981 will improve performances by 6.22%

Comparing ryo-ebata:ryo-ebata/no-static-element-interactions (6c17137) with main (1218c3a)

Summary

⚡ 1 improvements
✅ 107 untouched benchmarks

Benchmarks breakdown

Benchmark main ryo-ebata:ryo-ebata/no-static-element-interactions Change
dojo_11880045762646467684.js[cached] 8.5 ms 8 ms +6.22%

@ryo-ebata
Copy link
Author

ryo-ebata commented Jun 2, 2024

@unvalley

As this comment states, the following two methods were created to bridge the gap between eslint-plugin-jsx-a11y and Biome

  • is_valid_element
  • is_invalid_element

However, I thought it was necessary to optimize the Biome definition based on the official documentation, rather than discussing the two options of which way to go.

Is this an issue that should be addressed in this PR?
If necessary, we will address it in this PR, but I think there is an option to address it in another Issue, PR due to the number of elements.

@unvalley
Copy link
Member

unvalley commented Jun 3, 2024

@ryo-ebata

Is this an issue that should be addressed in this PR?
If necessary, we will address it in this PR, but I think there is an option to address it in another Issue, PR due to the number of elements.

Let's address that in another PR.

Comment on lines 51 to 74
m.insert("clipboard", vec!["onCopy", "onCut", "onPaste"]);
m.insert("composition", vec!["onCompositionEnd", "onCompositionStart", "onCompositionUpdate"]);
m.insert("keyboard", vec!["onKeyDown", "onKeyPress", "onKeyUp"]);
m.insert("focus", vec!["onFocus", "onBlur"]);
m.insert("form", vec!["onChange", "onInput", "onSubmit"]);
m.insert("mouse", vec![
"onClick", "onContextMenu", "onDblClick", "onDoubleClick", "onDrag", "onDragEnd",
"onDragEnter", "onDragExit", "onDragLeave", "onDragOver", "onDragStart", "onDrop",
"onMouseDown", "onMouseEnter", "onMouseLeave", "onMouseMove", "onMouseOut",
"onMouseOver", "onMouseUp"
]);
m.insert("selection", vec!["onSelect"]);
m.insert("touch", vec!["onTouchCancel", "onTouchEnd", "onTouchMove", "onTouchStart"]);
m.insert("ui", vec!["onScroll"]);
m.insert("wheel", vec!["onWheel"]);
m.insert("media", vec![
"onAbort", "onCanPlay", "onCanPlayThrough", "onDurationChange", "onEmptied",
"onEncrypted", "onEnded", "onError", "onLoadedData", "onLoadedMetadata", "onLoadStart",
"onPause", "onPlay", "onPlaying", "onProgress", "onRateChange", "onSeeked", "onSeeking",
"onStalled", "onSuspend", "onTimeUpdate", "onVolumeChange", "onWaiting"
]);
m.insert("image", vec!["onLoad", "onError"]);
m.insert("animation", vec!["onAnimationStart", "onAnimationEnd", "onAnimationIteration"]);
m.insert("transition", vec!["onTransitionEnd"]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need elements (and handlers) other than in CATEGORIES_TO_CHECK?
We only use focus, keyboard and mouse.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
In ESlint, interactions are managed in independent files named eventHandlersByType.
ref: https://github.com/jsx-eslint/jsx-ast-utils/blob/main/src/eventHandlers.js

Among them, ESlint uses focus, keyboard and mouse by default.
ref: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/974275353598e9407c76bd4a50c331a953755cee/src/rules/no-static-element-interactions.js#L33-L37

In other words, the EVENT_TO_HANDLERS created in this PR was originally intended to be used for other rules, etc., but since it is currently unclear where to place them, they are implemented this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. So, I guess it means we don't have any other rules that need this CATEGORIES_TO_CHECK yet.

Alright, we can locate the hashmap here, but I suggest commenting out elements that aren't used here. It may be a waste of overhead. Insertion for focus, keyboard, and mouse is enough.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
I see, I understand!
I'll fix this.

Copy link
Member

@ematipico ematipico Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to barge in during this review. I think this code would benefit from a refactor. I don't think it's a good custom to try to replicate JS code in Rust. For example, this code, while it's created once via lazy_static, still creates a bunch of vectors that are created at runtime (heap).

Instead, why not create simple arrays?

Also, I think we should explain what this code is. It's copied from another source, which is completely fine, but there's no comment that explains what are the keys and what are the values, how they are used and what they are meant to be.

For example composition -> [...]. What's composition? Where is it coming from? Is it an HTML element? Is it a role? Remember that in Biome, we want to aim for good standards, DX-wise and coding-wise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
fixed in this a9aa880

Copy link
Author

@ryo-ebata ryo-ebata Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
Sorry, I didn't address to additional comments, please leave them as they are.

Copy link
Author

@ryo-ebata ryo-ebata Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico @unvalley
Thank you for pointing this out.
As you said, it was causing overhead, so we defined it as a fixed-length array.

Also modified the comment to describe what is stored in this array and added the URL to the MDN reference describing each event handler

fixed in this commit b330131

@unvalley unvalley self-assigned this Jun 7, 2024
@ryo-ebata
Copy link
Author

@unvalley
Sorry for the delay in correcting the code in response to your comment...
Please check back as I have responded to your comments and corrected the points you raised!

@ematipico ematipico force-pushed the ryo-ebata/no-static-element-interactions branch from 8b225b2 to 6394998 Compare July 4, 2024 12:26
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just needs to remove the commented code, then we can merge it :)

Comment on lines 61 to 62
.text()
.split(' ')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico
Thanks your advice, I have incorporated the changes you suggested!
2dcbc5e

Comment on lines 73 to 99
{/* <summary> is inherently an interactive element, but in eslint-plugin-jsx-a11y,
it was made non-interactive due to the influence of an external library. */}
{/* ref: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/0be7ea95f560c6afc6817d381054d914ebd0b2ca/src/util/isInteractiveElement.js#L86-L89 */}
{/* <summary onClick={() => {}} /> */}

{/* This element is rejected by HTML Standard */}
{/* ref: https://lists.w3.org/Archives/Public/public-whatwg-archive/2012Aug/0298.html */}
{/* <content onClick={() => {}} /> */}

{/* This element is rejected by HTML Standard */}
{/* ref: https://html.spec.whatwg.org/multipage/obsolete.html */}
{/* <acronym onClick={() => {}} /> */}
{/* <applet onClick={() => {}} /> */}
{/* <frame onClick={() => { }} /> */}
{/* <frameset onClick={() => { }} /> */}
{/* <center onClick={() => {}} /> */}
{/* <font onClick={() => {}} /> */}
{/* <big onClick={() => {}} /> */}
{/* <blink onClick={() => {}} /> */}
{/* <rtc onClick={() => {}} /> */}
{/* <xmp onClick={() => {}} /> */}
{/* <strike onClick={() => {}} /> */}
{/* <param onClick={() => {}} /> */}
{/* <keygen onClick={() => {}} /> */}
{/* <noembed onClick={() => {}} /> */}
{/* <spacer onClick={() => {}} /> */}
{/* <tt onClick={() => {}} /> */}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these commented on purpose? If so, we should add a comment saying why, if not, we should remove them or uncomment them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico
I removed those.
7851727

Comment on lines 57 to 80
// (
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/ClipboardEvent
// "clipboard",
// &[
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/copy_event
// "onCopy",
// //ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/cut_event
// "onCut",
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/paste_event
// "onPaste",
// ],
// ),
// (
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/CompositionEvent
// "composition",
// &[
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/compositionstart_event
// "onCompositionStart",
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/compositionend_event
// "onCompositionEnd",
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/compositionupdate_event
// "onCompositionUpdate",
// ],
// ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico
I addressed this commit!
7851727

Comment on lines 103 to 113
// (
// "form",
// &[
// /// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event
// "onChange",
// // ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/input_event
// "onInput",
// // https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/submit_event
// "onSubmit",
// ],
// ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ematipico
I addressed this commit!
7851727

@ryo-ebata
Copy link
Author

@ematipico @unvalley
Thank you very much for your very extensive review!
I am personally interested in web a11y and would like you to let us continue to develop Biome's a11y rules in the future, may I?

Comment on lines 54 to 119
const EVENT_TO_HANDLERS: &[(&str, &[&str])] = &[
(
// ref https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
"keyboard",
&[
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/keydown_event
"onKeyDown",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/keyup_event
"onKeyUp",
//ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/keypress_event
"onKeyPress",
],
),
(
// ref: https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent
"focus",
&[
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/focus_event
"onFocus",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/blur_event
"onBlur",
],
),
(
// ref: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent
"mouse",
&[
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/click_event
"onClick",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/contextmenu_event
"onContextMenu",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/dblclick_event
"onDblClick",
"onDoubleClick",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/drag_event
"onDrag",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dragend_event
"onDragEnd",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dragenter_event
"onDragEnter",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dragleave_event
"onDragLeave",
"onDragExit",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dragover_event
"onDragOver",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dragstart_event
"onDragStart",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/drop_event
"onDrop",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mousedown_event
"onMouseDown",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mouseenter_event
"onMouseEnter",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mouseleave_event
"onMouseLeave",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mousemove_event
"onMouseMove",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mouseout_event
"onMouseOut",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mouseover_event
"onMouseOver",
// ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/mouseup_event
"onMouseUp",
],
),
];
Copy link
Member

@unvalley unvalley Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: i think we don't need the ref comments, because they are obvious and redundant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
thank you,
i fixed in this commit!
d09548f

@unvalley
Copy link
Member

unvalley commented Jul 4, 2024

I am personally interested in web a11y and would like you to let us continue to develop Biome's a11y rules in the future, may I?

Of course, helps are always welcome 😄

@unvalley
Copy link
Member

unvalley commented Jul 4, 2024

@ryo-ebata could you apply codegen to pass the CI?

Comment on lines 1337 to 1356
let role_name = if let Some(role) = attributes.get("role") {
if let Some(r) = role.first() {
self.get_role(r)
} else {
None
}
} else {
self.get_implicit_role(element_name, attributes)
};

if let Some(role) = role_name {
match role.type_name() {
"biome_aria::roles::PresentationRole" | "biome_aria::roles::GenericRole" => {
return false
}
_ => return true,
}
}

false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be more simple (if this works)

Suggested change
let role_name = if let Some(role) = attributes.get("role") {
if let Some(r) = role.first() {
self.get_role(r)
} else {
None
}
} else {
self.get_implicit_role(element_name, attributes)
};
if let Some(role) = role_name {
match role.type_name() {
"biome_aria::roles::PresentationRole" | "biome_aria::roles::GenericRole" => {
return false
}
_ => return true,
}
}
false
let role_name = attributes
.get("role")
.and_then(|role| role.first())
.map(|r| self.get_role(r))
.or_else(|| self.get_implicit_role(element_name, attributes));
match role_name.map(|role| role.type_name()) {
Some("biome_aria::roles::PresentationRole" | "biome_aria::roles::GenericRole") => false,
Some(_) => true,
None => false,
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unvalley
thanks,
a part of your suggestion code do not work, so i fixed!

7063f9b

Copy link
Member

@unvalley unvalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some suggestions but they are nits or for refactor. Thank you!

@ryo-ebata
Copy link
Author

ryo-ebata commented Jul 7, 2024

@unvalley
Thank you for your suggestion!
I have corrected your point and run the just gen-lint and just gen-bindings commands.

Also, I have an existing nursery rule that fails the snapshot test with my changes, so I'm going to address that with another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants