feat(linter): adds jsx-a11y/no-static-element-interactions rule

[colin-harrison]

What's New

• Implementation for jsx-a11y/no-static-element-interactions - ☂️ eslint-plugin-jsx-a11y #1141
• A new crate / pnpm workspace in tasks/a11y_data
  • Extracts and transforms accessibility data from the npm registry into Rust data structures
• A new crate in oxc/oxc_aria_query
  • Home of aforementioned Rust data structures
• Util functions for parsing role attribute values

Data Needs

This rule relies on data from aria-query and axobject-query npm packages. These data are not available in any well-maintained Rust crates from my search.

There's a comment in oxc_linter/src/utils/react.rs that suggests a oxc-project/aria-query repo is a W.I.P. but that repo is labeled as "under construction" and now appears to be archived.

I found no source for axobject-query data in Rust.

I followed the lead of tasks/compat_data and crates/oxc_compat which is similarly converting data from an npm package into Rust code.

I opted to grab the minimum set of data required for this rule, but even this set should be enough to help with other unimplemented jsx-a11y plugin rules, like interactive-supports-focus, that rely on the same data.

Known Differences from ESLint Implementation

I found two main issues with the ESLint implementation of this rule.

1. There was some no-op code that seemed to imply the only expression value rules allowed by the allowExpressionValues rule were ternaries where both sides were string literals. I looked at the GH issue that led to the no-op code being added and I believe the no-op code was added due to some miscommunication. I suspect the issue reporter was using a version of the jsx-a11y plugin that didn't have the allowExpressionValues option at all. A year and a half later the ticket was picked up and that option would've existed on main and I believe the contributor assumed that the user was on the latest code so they added some new code to handle that case which the latest code already permitted. Unsurprisingly their new test case passed as the "violation" had already been permitted by the latest code.
2. There are some inconsistencies in the ESLint plugin code about how they classify role attributes. Sometimes they use the entire role attribute value to classify roles and other times they separate the string on whitespace and use the first valid role. This difference in behavior didn't seem intentional so I've made this more consistent by creating a first_valid_role() function in react.rs that uses the latter strategy.

[graphite-app]

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[camchenry]

I think I'm in support of having our own version of aria-query in theory, but it's a big undertaking. I left some small suggestions in the PR, but bigger suggestions I have:

• Is there another simpler rule that requires a subset of this information? It would be simpler if we didn't need to migrate all of the data all at once.
• Maybe we could split the oxc_aria_query crate into its own separate PR? That would make this a bit simpler to review. It's also lacking tests, documentation and such that would be suitable for a published crate.

[camchenry]

Would you mind porting over the tests from the original rule? https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/a7d1a12a6198d546c4a06477b385b4fde03b762e/__tests__/src/rules/no-static-element-interactions-test.js#L4

There are a lot of tests currently missing.

[camchenry]

Instead of simply returning a string (which loses the info about what role was contained here), how about parsing out the role?

This is a slightly bigger refactoring, but it would save us from needing to re-check what list of roles contains this value.

Something like this would be a good start at least:

enum AriaRoleType {
  Interactive(String)
  Noninteractive(String)
  Abstract(String)
}

Then, instead of having is_interactive_role functions we could do:

impl AriaRoleType {
  fn is_interactive(&self) -> bool { }
  fn is_noninteractive(&self) -> bool { }
  // ...
}

And at that point, it shouldn't contain any linter-specific code, so we could move it into oxc_aria_query as well.

[camchenry]

This comment doesn't seem to match the actual behavior, perhaps it should be more like this?

Suggested change

	if is_interactive_element(element_type.as_ref(), jsx_opening_el)
	|| is_interactive_role(jsx_opening_el)
	|| is_noninteractive_element(element_type.as_ref(), jsx_opening_el)
	|| is_noninteractive_role(element_type.as_ref(), jsx_opening_el)
	|| is_abstract_role(element_type.as_ref(), jsx_opening_el)
	{
	// This rule has no opinion about abstract roles.
	return;
	}
	if is_abstract_role(element_type.as_ref(), jsx_opening_el)
	{
	// This rule has no opinion about abstract roles.
	return;
	}
	if is_interactive_element(element_type.as_ref(), jsx_opening_el)
	&& is_interactive_role(jsx_opening_el)
	&& is_noninteractive_element(element_type.as_ref(), jsx_opening_el)
	&& is_noninteractive_role(element_type.as_ref(), jsx_opening_el) {
	// actual rule logic here ...
	}

[camchenry]

Storing these tag names multiple times bloats the binary size. We should store these as static values and reference them instead.

Example:

static TAG_A: &'static str = "a";
static TAG_AREA: &'static str = "area";

ElementSchema {
  name: TAG_AREA,
  attributes: &[]
}

The same holds true for all property/attribute names too. If we could replace each of those with a reference to a static string, that would be a nice improvement.

[camchenry]

Is there any reason we need to commit these files? We don't use them except for generating the Rust code which should be equivalent, so these are just an intermediate artifact, right?

[camchenry]

These could be run in parallel with Promise.all, no? Unless there is a particular reason we need to run these all synchronously.

[camchenry]

I know this is more or less a direct port of aria-query, but it seems like these values are stored suboptimally. If we're only interested in input element schemas, we have to iterate over the entire array to find them. It seems like it would be better to store separate arrays for each element name, and iterate over those arrays instead. That would also allow us to easily check if a given tag is in the schemas list too.

Something more like this:

  "input" => &[
    ElementSchema {
      attributes: &[/* ... */]
    },
    ElementSchema {
      attributes: &[/* ... */]
    },
  ],

[connorshea]

This has been implemented via #17538, see no_static_element_interactions.rs.

Is there a concern with regards to the accuracy/data used for this rule that warrants pulling in the additional crate?

[mehm8128]

This PR includes some roles that are not included in the #17538 such as doc-backlink, graphics-document. These are part of dpub-aria and graphics-aria. Maybe we should consider these roles for accuracy regardless of whether we will adapt new oxc_aria_query crate or not.
In my opinion, we can't say that aria-query and axobject-query are maintained enough because these were last released and updated more than one year ago, so I'd like to propose to use roleInfo.js, which is maintained by W3C (even not now in the future). For now this provides the least information about ARIA roles but probably W3C will maintain more frequently and consider to include more information about ARIA in response to our demand such as ARIA properties and states (ref). To get ARIA information from the standardized data, Oxlint will be able to provide accurate and updated lint features.