Skip to content

JS-1465 Fix S6819 false positive: SVG role="img" with accessible name#6856

Open
sonar-nigel[bot] wants to merge 7 commits intomasterfrom
fix/JS-1465-fix-fp-on-s6819-inline-svg-with-roleimg-is-semantically-correct-sonnet
Open

JS-1465 Fix S6819 false positive: SVG role="img" with accessible name#6856
sonar-nigel[bot] wants to merge 7 commits intomasterfrom
fix/JS-1465-fix-fp-on-s6819-inline-svg-with-roleimg-is-semantically-correct-sonnet

Conversation

@sonar-nigel
Copy link
Copy Markdown
Contributor

@sonar-nigel sonar-nigel bot commented Apr 17, 2026

Relates to JS-1465

S6819 was incorrectly flagging <svg role="img"> elements that carry a proper accessible name — a <title> child, aria-label, or aria-labelledby attribute. These are valid WCAG-compliant patterns for inline SVG icons that need CSS control or animation that a native <img> cannot provide. The fix adds a tightly scoped isSemanticSvgImg() guard in the decorator that suppresses the report only when both role="img" and a non-empty accessible name are present; SVGs with role="img" but no accessible name remain true positives.

  • Empty-string values for aria-label/aria-labelledby are correctly treated as missing (no suppression), matching how isDecorativeSvg already handles attribute values.
  • <title> suppression requires at least one non-whitespace text child or a dynamic expression child — an empty <title> does not suppress.
  • The ruling files were synced after the refinement; the net change is a reduction in reported issues on the Peach corpus.

Vibe Bot added 3 commits April 17, 2026 09:54
Tests cover the scenario where inline SVG elements with role="img" and
a proper accessible name (via <title> child, aria-label, or aria-labelledby)
are incorrectly flagged as violations suggesting <img> replacement. The tests
verify that the decorator suppresses reports for these WCAG-compliant patterns
while still flagging SVG elements with role="img" but no accessible name.

Relates to JS-1465
The rule was incorrectly flagging inline SVG elements with role="img" that
have proper accessible names via <title> child elements, aria-label, or
aria-labelledby attributes. This is a WCAG-compliant pattern for icon
components that require CSS class control, animation, or programmatic
styling — capabilities that native <img> tags cannot provide.

Extends decorator.ts with a new isSemanticSvgImg() helper that suppresses
reports for SVG elements with role="img" when they have an accessible name.
SVG elements with role="img" but no accessible name continue to be flagged
as true positives. Also adds a compliant code example to the rule spec.

Implementation follows the approved proposal guidelines from JS-1465.

Relates to JS-1465
@sonar-nigel
Copy link
Copy Markdown
Contributor Author

sonar-nigel bot commented Apr 17, 2026

Rule Profile

Field Value
Rule S6819 — Prefer tag over ARIA role
Severity / type Minor / CODE_SMELL
Sonar Way Yes
Scope Main

A genuine violation — an SVG with role="img" but no accessible name:

<svg role="img" viewBox="0 0 24 24">
  <path d="M5 12h14"/>
</svg>
// ^ S6819: Use <img alt=...> instead of the "img" role

False Positive Pattern

<svg role="img"> combined with a proper accessible name is the WCAG-recommended pattern for inline SVG icons that must remain in markup to support CSS class manipulation, animation, or programmatic styling — capabilities that a native <img> element cannot provide. The upstream prefer-tag-over-role rule has no awareness of this distinction and flags all such SVGs unconditionally.

// FP 1: accessible name via <title> child
<svg role="img" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
  <title>Nx</title>
  <path d="M11.987 14.138l-3.132 4.923..." />
</svg>

// FP 2: accessible name via aria-label
<svg role="img" aria-label="GitHub" viewBox="0 0 24 24">
  <path d="..." />
</svg>

// FP 3: accessible name via aria-labelledby
<svg role="img" aria-labelledby="icon-title" viewBox="0 0 24 24">
  <title id="icon-title">Download</title>
  <path d="..." />
</svg>

False Negative Risk

The guard is conservative: suppression only fires when role="img" and a substantive accessible name are both present. The two main edge cases where a real issue could be masked are:

  • Empty aria-label="": getLiteralPropValue is checked against '', so an explicit empty string does not suppress — the issue is still reported.
  • Empty <title></title>: hasTitleChild requires at least one non-whitespace text child or a dynamic expression child (JSXExpressionContainer), so a bare <title/> or <title></title> does not suppress.

The one truly conservative choice is dynamic expressions — aria-label={someVar} or <title>{someVar}</title> suppresses because the value cannot be statically evaluated. If someVar is an empty string at runtime the issue would be silently missed, but this is an inherent limitation of static analysis and is consistent with how every other attribute guard in this decorator behaves.


Fix Summary

The implementation adds two pure functions to decorator.ts and wires the new guard into the existing isValidAriaPattern chain:

  • isSemanticSvgImg() — early-returns false for any element that is not <svg role="img">, then checks for a non-empty aria-label, a non-empty aria-labelledby, or a <title> child with content.
  • hasTitleChild() — walks node.parent.children looking for a JSXElement whose opening tag is named title and that has at least one JSXText child with non-whitespace content or any JSXExpressionContainer child.
  • Test suite extended with an explicit aria-label="" invalid case to document and guard the empty-string boundary.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 17, 2026

Ruling Report

No changes to ruling expected issues in this PR

@sonar-nigel sonar-nigel bot marked this pull request as ready for review April 17, 2026 10:28
@sonar-review-alpha
Copy link
Copy Markdown
Contributor

sonar-review-alpha bot commented Apr 17, 2026

Summary

This PR fixes a false positive in S6819 (prefer-tag-over-role) by recognizing inline SVG with role="img" as a valid WCAG pattern when coupled with an accessible name. The fix adds a scoped isSemanticSvgImg() guard to the decorator that suppresses the rule only when both conditions are met: the element is an SVG with role="img" AND has an accessible name from <title>, aria-label, or aria-labelledby. SVGs with role="img" but no accessible name remain correctly flagged.

What reviewers should know

Where to start: Look at isSemanticSvgImg() and hasTitleChild() functions in decorator.ts — these contain the core suppression logic. The guard is inserted at line 117 in isValidAriaPattern().

Key implementation details:

  • Empty strings for aria-label/aria-labelledby are correctly treated as missing (no suppression)
  • <title> suppression requires at least one non-whitespace text child or a dynamic expression — empty <title> tags don't suppress
  • The check integrates cleanly with the existing isDecorativeSvg() pattern without duplicating logic

Test coverage: The unit test comprehensively covers both the decorator's suppression (valid SVGs that should not be flagged) and the upstream rule's behavior (which still raises errors on these cases, proving the decorator suppression is working). Test cases include edge cases like empty <title> and empty aria-label to ensure they don't suppress.


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as resolved.

Vibe Bot added 2 commits April 17, 2026 10:36
…rc/jsts/rules/S6819/decorator.ts

Comment: `getProp` returns the prop AST node whenever the attribute is present, regardless of its value. This means `<svg role="img" aria-label="">` (explicit empty string — no accessible name) would still satisfy the guard and suppress the report, producing a false negative.

Contrast with `isDecorativeSvg` just above, which correctly uses `getLiteralPropValue` to inspect the actual value before accepting the prop.

For dynamic expressions (`aria-label={someVar}`), `getLiteralPropValue` returns `null`, which is fine — we can't statically determine the value so suppressing is the safe choice. Only the literal empty-string case is broken.

```suggestion
  const ariaLabelProp = getProp(attributes, 'aria-label');
  if (ariaLabelProp && getLiteralPropValue(ariaLabelProp) !== '') {
    return true;
  }
  const ariaLabelledbyProp = getProp(attributes, 'aria-labelledby');
  if (ariaLabelledbyProp && getLiteralPropValue(ariaLabelledbyProp) !== '') {
    return true;
  }
```

- [ ] Mark as noise
…rc/jsts/rules/S6819/unit.test.ts

Comment: The `invalid` block only tests SVG with no accessible-name attributes at all. There is no test for `<svg role="img" aria-label="">` (explicit empty string), which is the edge case the guard currently mishandles. Adding it here would both document the expected behaviour and catch a regression if the guard is ever simplified back to a bare `getProp` check.

```suggestion
      invalid: [
        // svg role="img" without an accessible name is still a true positive
        {
          code: `<svg role="img" viewBox="0 0 24 24"><path d="M5 12h14"/></svg>`,
          errors: 1,
        },
        // svg role="img" with an explicit empty aria-label provides no accessible name
        {
          code: `<svg role="img" aria-label=""><path d="M5 12h14"/></svg>`,
          errors: 1,
        },
      ],
```

- [ ] Mark as noise
sonar-review-alpha[bot]

This comment was marked as resolved.

@sonarqube-next
Copy link
Copy Markdown

…rc/jsts/rules/S6819/decorator.ts

Comment: `hasTitleChild` checks for the *presence* of a `<title>` child but not whether it has any content. `<svg role="img"><title></title></svg>` passes the guard and suppresses the report even though an empty `<title>` provides no accessible name — the exact same false-negative class as the `aria-label=""` issue fixed in this commit.

The fix should match the treatment of `aria-label`: accept a `<title>` only when it has at least one non-whitespace text child or a dynamic expression (which we can't statically evaluate, so we conservatively suppress).

```suggestion
  return parent.children.some(
    child =>
      child.type === 'JSXElement' &&
      child.openingElement.name.type === 'JSXIdentifier' &&
      child.openingElement.name.name === 'title' &&
      child.children.some(
        c =>
          (c.type === 'JSXText' && c.value.trim() !== '') ||
          c.type === 'JSXExpressionContainer',
      ),
  );
```

- [ ] Mark as noise
sonar-review-alpha[bot]

This comment was marked as outdated.

s6819-inline-svg-with-roleimg-is-semantically-correct-sonnet
Copy link
Copy Markdown
Contributor

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

The fix is well-structured and the core logic is sound. One subtle behavior around getLiteralPropValue returning null for non-literal props is worth confirming before merge — see the inline comment on isSemanticSvgImg.

🗣️ Give feedback

Comment on lines +159 to +164
const ariaLabelProp = getProp(attributes, 'aria-label');
if (ariaLabelProp && getLiteralPropValue(ariaLabelProp) !== '') {
return true;
}
const ariaLabelledbyProp = getProp(attributes, 'aria-labelledby');
if (ariaLabelledbyProp && getLiteralPropValue(ariaLabelledbyProp) !== '') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getLiteralPropValue returns null for any non-literal prop value — dynamic expressions like aria-label={someVar}, aria-label={null}, or aria-label={undefined}. Because null !== '' is true, those cases all fall through and suppress the report.

This is intentionally more permissive than isDecorativeSvg, which uses === true || === 'true' (so a dynamic aria-hidden does not suppress). For aria-label, accepting unknowable dynamic values is a reasonable static-analysis trade-off — but aria-label={null} is a real pattern in conditional rendering (e.g. aria-label={active ? 'Close' : null}) and React silently drops the attribute at runtime, leaving the SVG with no accessible name.

Two things worth confirming:

  1. Is suppressing on any dynamic aria-label/aria-labelledby intentional, or should it require a string literal (mirroring how isDecorativeSvg handles aria-hidden)?
  2. If dynamic suppression is intentional, a test case documenting it (aria-label={someVar} in the valid block) would pin the behaviour and serve as an explicit notice that it differs from the decorative-SVG guard.
Suggested change
const ariaLabelProp = getProp(attributes, 'aria-label');
if (ariaLabelProp && getLiteralPropValue(ariaLabelProp) !== '') {
return true;
}
const ariaLabelledbyProp = getProp(attributes, 'aria-labelledby');
if (ariaLabelledbyProp && getLiteralPropValue(ariaLabelledbyProp) !== '') {
const ariaLabelProp = getProp(attributes, 'aria-label');
const ariaLabelValue = ariaLabelProp ? getLiteralPropValue(ariaLabelProp) : null;
if (typeof ariaLabelValue === 'string' && ariaLabelValue !== '') {
return true;
}
const ariaLabelledbyProp = getProp(attributes, 'aria-labelledby');
const ariaLabelledbyValue = ariaLabelledbyProp ? getLiteralPropValue(ariaLabelledbyProp) : null;
if (typeof ariaLabelledbyValue === 'string' && ariaLabelledbyValue !== '') {
return true;
}
  • Mark as noise

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.

1 participant