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

Merged

Conversation

ryo-ebata
Copy link
Contributor

@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 not alter performance

Comparing ryo-ebata:ryo-ebata/no-static-element-interactions (e99e251) with ryo-ebata:ryo-ebata/no-static-element-interactions (d4b6279)

Summary

✅ 108 untouched benchmarks

@ryo-ebata
Copy link
Contributor 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.

@unvalley unvalley self-assigned this Jun 7, 2024
@ryo-ebata
Copy link
Contributor 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!

@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?

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
Contributor 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.

@unvalley
Copy link
Member

unvalley commented Jul 8, 2024

@ryo-ebata There is a regression in useValidAutocomplete/valid.jsx.snap, could you take a look

@ryo-ebata
Copy link
Contributor Author

@unvalley

There is a regression in useValidAutocomplete/valid.jsx.snap, could you take a look

Okay! I'll address in this PR!

@ryo-ebata
Copy link
Contributor Author

ryo-ebata commented Jul 9, 2024

@unvalley
I addressed to fix regression in useValidAutocomplete/valid.jsx.snap.
08adc60

But one concern has arisen.
The extract_attributes() function can now handle dynamic values by my change. (Previously, if there were dynamic values, extract_attributes() would stop and terminate processing.)

If an attribute value is dynamic, the caller of extract_attributes() must now handle the processing differently. Therefore, we want to return a value from extract_attributes() that indicates whether it's a dynamic value, but since the return type is String, we're currently returning "[DYNAMIC]".

However, if a user actually sets "[DYNAMIC]" as an attribute value, the system will incorrectly interpret it as a dynamic value.

I'd like to consult with you on whether there might be a better approach to this issue....

@unvalley
Copy link
Member

unvalley commented Jul 10, 2024

@ryo-ebata
The extract_attrs function should correctly return each rule even when it returns function-like attributes.
Returning strings like "[DYNAMIC]" should be avoided unless it is defined in the specification somewhere, instead it should be changed returning enum can clarify the attribute kinds.

This PR is getting longer, so if this takes too long, update the test case and that change is fine in another PR.

@ryo-ebata ryo-ebata force-pushed the ryo-ebata/no-static-element-interactions branch from 08adc60 to 201af4f Compare July 11, 2024 12:14
@ryo-ebata
Copy link
Contributor Author

@unvalley

instead it should be changed returning enum can clarify the attribute kinds.

Fixed to return enum, and also fixed a rule that was causing regressions.
Thanks for a lot of reviews, I think this will be my last commit!
201af4f

@ematipico ematipico merged commit 4af61e1 into biomejs:main Jul 15, 2024
10 of 11 checks passed
@ryo-ebata ryo-ebata deleted the ryo-ebata/no-static-element-interactions branch July 15, 2024 11:11
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