Skip to content

Handle logical expressions when getting attribute values - #1014

Closed
K-Schaeffer wants to merge 5 commits into
vue-a11y:mainfrom
K-Schaeffer:handle-logical-expressions
Closed

Handle logical expressions when getting attribute values#1014
K-Schaeffer wants to merge 5 commits into
vue-a11y:mainfrom
K-Schaeffer:handle-logical-expressions

Conversation

@K-Schaeffer

@K-Schaeffer K-Schaeffer commented Jan 16, 2024

Copy link
Copy Markdown

I noticed that logical expressions are not being resolved when getting attribute values, and thus simple bindings that should not raise a failed rule are raising, example:

<div role="textbox" :tabindex="disabled || 0" :aria-disabled="disabled"></div>

This should not fail the rule because if disabled is false then it actually has tabindex properly, but it currently fails with the "element should be focusable" rule.

The reason behind this is the fact that the own expression is being returned on all cases except for the Literal, that actually returns the value of the attribute.

  if (node.value.expression.type === "Literal") {
      return node.value.expression.value;
    }

   // TODO we're effectively using this as just a placeholder to let rules know
   // that a value has been passed in for this attribute. We should replace
   // this with a stronger API to either explicitly handle all of the different
   // types of values or just return a special symbol or something else.
   return node.value.expression;

I understand the TODO that was put on the return of the expression on those cases and I think that handling the LogicalExpression as I did on my PR might be a good addition to at least solve this simple case, even though we definitely have more cases that would require to be handled as the comment suggests.

Comment thread src/utils/getAttributeValue.ts Outdated
@K-Schaeffer
K-Schaeffer marked this pull request as draft January 16, 2024 21:45
@K-Schaeffer
K-Schaeffer marked this pull request as ready for review January 17, 2024 22:33
@K-Schaeffer

Copy link
Copy Markdown
Author

Any thoughts here? @vhoyer

@G-Rath

G-Rath commented Feb 12, 2026

Copy link
Copy Markdown
Contributor

@K-Schaeffer appreciate the effort, but ignoring the use of eval (which is evil and should never be used), I don't see what the actual value is since this will only work for literals which can always be simplified based on the operator which people usually have linting rules and IDEs to catch this - I don't see this as being a real world situation?

@K-Schaeffer

Copy link
Copy Markdown
Author

Hey @G-Rath, thanks for taking a look!

I'd push back a little on the eval concern since the inputs are fully constructed from AST literal values already validated by the ESLint parser, so there's no real injection risk here.

The fix does cover the proposed scenario: Dynamic components that use simple logical expressions for aria attributes can now do so without incorrectly breaking the linter. This is genuinely useful for component libraries where accessibility attributes are conditionally applied based on props.

That said, the approach is limited to logical expressions where both sides are literals. Expressions involving dynamic identifiers are still not resolved, and properly handling those would require static type inference or a deeper rework of getAttributeValue as the existing TODO already hints at.


I'm going to close this PR for now. In my own projects I'm still having to suppress the a11y rule on lines where dynamic identifiers are involved. Hopefully this discussion serves as useful context if someone tackles the broader improvement down the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants