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): new rule useValidAriaRole #553

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Conversation

vasucp1207
Copy link
Member

@vasucp1207 vasucp1207 commented Oct 19, 2023

Summary

Closes #526

Configuration  
Recommended
Fixable
Fix Kind 🔓

Tasks

  • Add options
  • Check for custom components

Test Plan

  • Snapshots

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Oct 19, 2023
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.

Thank you! I am going to block for now while we focus only on bug fixes :)

@Conaclos Conaclos requested a review from nissy-dev October 21, 2023 13:49
Copy link
Contributor

@nissy-dev nissy-dev left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions! I added some comments.

@@ -0,0 +1,6 @@
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,6 @@
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

@vasucp1207
Copy link
Member Author

Now it also checks for attr val like "row range" by splitting the string. Also I think some roles are missing roles.rs like tabpanel, treegrid etc, there are 78 in total in wai-aria?

@vasucp1207 vasucp1207 requested a review from nissy-dev October 22, 2023 18:52
rule_category!(),
node.range(),
markup! {
"Enforce that elements with ARIA roles must use a valid, non-abstract ARIA role."
Copy link
Member

@ematipico ematipico Oct 23, 2023

Choose a reason for hiding this comment

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

This is the phrase you want in the documentation, not here. Here, you want to have something like "The role <ROLE_NAME> doesn't exist", or `"The role <ROLE_NAME> is abstract and can't be applied.*

},
)
.note(markup! {
"Check "<Hyperlink href="https://www.w3.org/TR/wai-aria/#namefromauthor">"wai-aria"</Hyperlink>" for valid roles or provide options accordingly."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Check "<Hyperlink href="https://www.w3.org/TR/wai-aria/#namefromauthor">"wai-aria"</Hyperlink>" for valid roles or provide options accordingly."
"Check "<Hyperlink href="https://www.w3.org/TR/wai-aria/#namefromauthor">"WAI-ARIA"</Hyperlink>" for valid roles or provide options accordingly."

pub(crate) UseValidAriaRole {
version: "next",
name: "useValidAriaRole",
recommended: 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 should be recommended, I think. ARIA rules are very important.

Comment on lines 60 to 90
pub struct UseValidAriaRoleOptions {
allowed_invalid_roles: Vec<String>,
ignore_non_dom: bool,
}
Copy link
Member

Choose a reason for hiding this comment

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

You created an option, but it seems we are not using it. In order to "wire" an option to Biome, you have to:

  1. Add it to the list of rules that have options:
    pub enum PossibleOptions {
    /// Options for `noExcessiveComplexity` rule
    Complexity(#[bpaf(external(complexity_options), hide)] ComplexityOptions),
    /// Options for `useExhaustiveDependencies` and `useHookAtTopLevel` rule
    Hooks(#[bpaf(external(hooks_options), hide)] HooksOptions),
    /// Options for `useNamingConvention` rule
    NamingConvention(#[bpaf(external(naming_convention_options), hide)] NamingConventionOptions),
    /// Options for `noRestrictedGlobals` rule
    RestrictedGlobals(#[bpaf(external(restricted_globals_options), hide)] RestrictedGlobalsOptions),
    /// No options available
    #[default]
    NoOptions,
    }

match rule_key.rule_name() {

"useExhaustiveDependencies" | "useHookAtTopLevel" => {

  1. You must apply all the correct macros, here's an example:

    #[derive(Default, Deserialize, Serialize, Eq, PartialEq, Debug, Clone, Bpaf)]
    #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
    #[serde(rename_all = "camelCase", deny_unknown_fields)]
    pub struct RestrictedGlobalsOptions {
    /// A list of names that should trigger the rule
    #[serde(skip_serializing_if = "Option::is_none")]
    #[bpaf(hide, argument::<String>("NUM"), many, optional)]
    denied_globals: Option<Vec<String>>,
    }

  2. Apply FromStr, with no fancy implementation

  3. And apply our internal deserialisation

    impl VisitNode<JsonLanguage> for RestrictedGlobalsOptions {
    fn visit_member_name(
    &mut self,
    node: &SyntaxNode<JsonLanguage>,
    diagnostics: &mut Vec<DeserializationDiagnostic>,
    ) -> Option<()> {
    has_only_known_keys(node, Self::KNOWN_KEYS, diagnostics)
    }
    fn visit_map(
    &mut self,
    key: &SyntaxNode<JsonLanguage>,
    value: &SyntaxNode<JsonLanguage>,
    diagnostics: &mut Vec<DeserializationDiagnostic>,
    ) -> Option<()> {
    let (name, value) = self.get_key_and_value(key, value, diagnostics)?;
    let name_text = name.text();
    if name_text == "deniedGlobals" {
    self.denied_globals = self.map_to_array_of_strings(&value, name_text, diagnostics);
    }
    Some(())
    }
    }

For testing, the guide should help you: https://github.com/biomejs/biome/blob/main/crates/biome_analyze/CONTRIBUTING.md#rule-configuration

Once you've done this, there are two missing things:

  • add documentation comments for allowed_invalid_roles and ignore_non_dom and explain what they do
  • update the documentation of this rule by adding the options and explaining them:
    /// ## Options
    ///
    /// Use the options to specify additional globals that you want to restrict in your
    /// source code.
    ///
    /// ```json
    /// {
    /// "//": "...",
    /// "options": {
    /// "deniedGlobals": ["$", "MooTools"]
    /// }
    /// }
    /// ```
    ///
    /// In the example above, the rule will emit a diagnostics if tried to use `$` or `MooTools` without
    /// creating a local variable.

@nissy-dev
Copy link
Contributor

Now it also checks for attr val like "row range" by splitting the string. Also I think some roles are missing roles.rs like tabpanel, treegrid etc, there are 78 in total in wai-aria?

I have created the PR (#629) about this.

Comment on lines 115 to 121
fn visit_member_name(
&mut self,
node: &SyntaxNode<JsonLanguage>,
diagnostics: &mut Vec<DeserializationDiagnostic>,
) -> Option<()> {
has_only_known_keys(node, Self::KNOWN_KEYS, diagnostics)
}
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to rebase your work on main. I pushed some changes related to config parsing. In the process I removed visit_member_name. I introduced report_unknown_map_key that you can use directly in visit_map.

@ematipico ematipico changed the title feat(lint): new useValidAriaRole feat(lint): new rule useValidAriaRole Oct 30, 2023
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.

I think we can merge, we should resolve the conflicts

@vasucp1207 vasucp1207 requested a review from ematipico November 9, 2023 17:53
@Conaclos Conaclos force-pushed the rule-role branch 2 times, most recently from eddb5a9 to 1a4c382 Compare November 13, 2023 13:35
Copy link
Member

@Conaclos Conaclos left a comment

Choose a reason for hiding this comment

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

Thanks!

@Conaclos Conaclos merged commit ea53c38 into biomejs:main Nov 13, 2023
14 checks passed
@Conaclos Conaclos added the A-Changelog Area: changelog label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement lint/useValidAriaRole - eslint-plugin-jsx-a11y/aria-role
4 participants