Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: main
Are you sure you want to change the base?
feat(lint/noStaticElementInteractions): add rule #2981
Changes from 11 commits
d659393
4c71fa5
82626b0
f1ceb5e
494a4aa
aa03e21
ae05cca
9876632
679fbab
3a83fab
2281933
53745f3
93e07f2
f4f65cb
ae8d3c7
cc06ac5
7343ed2
74804bc
71c5004
a9aa880
b330131
546fa4a
f1d8524
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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
andmouse
.There was a problem hiding this comment.
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
andmouse
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.
There was a problem hiding this comment.
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
, andmouse
is enough.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'scomposition
? 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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "valid element" mean? Valid for what context? We should document it.
Plus, you would want to move this function inside
biome_aria
, as @unvalley suggested. Here's an example:biome/crates/biome_aria/src/roles.rs
Lines 1131 to 1138 in 9e8addd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ematipico cc @unvalley
Regarding this point, this method includes processing that considers elements generally deemed non-interactive as valid.
These elements were part of valid test cases in eslint-plugin-jsx-a11y.
When considering portability from ESLint, these test cases should be taken into account (meaning that if a project using ESLint transitions to Biome, many lint errors could occur). However, if we are overly concerned with the above point, we will never be able to lint according to Biome's rules.
Therefore, if we align with Biome's interactive criteria and adjust the rules accordingly, this method will become unnecessary. I would appreciate it if you could provide guidance on the direction we should take.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there was a misunderstanding; I'm sorry about it. I am not asking you to use the link I pasted. The link I pasted was an example. And I didn't ask to remove the logic you're porting from the plugin.
What I asked was to properly document the function. The phrase
"This function ables non-interactive elements."
doesn't provide any value to the function name, because it doesn't explain what is a valid element (or when it is valid).I understand that English is not your first language, so I am willing to help if you explain a bit the logic of this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this PR after addressing this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ematipico cc @unvalley
Okay, thank you.
Then I need to see why Eslint is enabling these elements, but we have not yet found a clear reason.
So it may take a little time, but I may ask for your help in documenting it. Thank you in advance for your help at that time.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.