Skip to content

SONARHTML-395 Handle template-generated labels in S6853#715

Open
erwan-leforestier-sonarsource wants to merge 13 commits into
masterfrom
fix/sonarhtml-395-handle-template-generated-labels
Open

SONARHTML-395 Handle template-generated labels in S6853#715
erwan-leforestier-sonarsource wants to merge 13 commits into
masterfrom
fix/sonarhtml-395-handle-template-generated-labels

Conversation

@erwan-leforestier-sonarsource

@erwan-leforestier-sonarsource erwan-leforestier-sonarsource commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Handle S6853 false positives on template-generated labels by recognizing Razor and Thymeleaf patterns that only resolve at render time.

Changes

  • track whether a Razor asp-for label is truly empty before trusting its generated text
  • recognize Thymeleaf th:for and th:attr associations, plus template text attributes such as th:text, th:utext, v-text, and v-html
  • add regression coverage for empty Razor labels and template-generated label content

Functional Validation

Artifact: SONARHTML-395-fv.zip

Once the file is attached to the PR description, unzip and run:
./run.sh

Expected output:

******************* MASTER *******************
Analyzing "razor-empty-label.cshtml"...
Results:
    - Rule "S6853" -> L.1
Analyzing "template-generated-label.html"...
Results:
    - Rule "S6853" -> L.1
    - Rule "S6853" -> L.2

****** Branch "fix/sonarhtml-395-handle-template-generated-labels" ******
Analyzing "razor-empty-label.cshtml"...
Results:
    (none)
Analyzing "template-generated-label.html"...
Results:
    (none)

@erwan-leforestier-sonarsource erwan-leforestier-sonarsource force-pushed the fix/sonarhtml-395-handle-template-generated-labels branch from de1ce60 to e260469 Compare June 24, 2026 14:41
@github-actions

Copy link
Copy Markdown
Contributor

Ruling Report

No changes to ruling expected issues in this PR

@erwan-leforestier-sonarsource erwan-leforestier-sonarsource marked this pull request as ready for review June 25, 2026 08:28

@asya-vorobeva asya-vorobeva left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like corresponding RSpec description is very confusing: https://musical-adventure-r9qk65j.pages.github.io/rspec/#/rspec/S6853/html
It says that label elements should have a text label (which is obviously wrong for template-generated labels) and doesn't provide list of exceptions. Should we update it?

@erwan-leforestier-sonarsource

erwan-leforestier-sonarsource commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Hi @asya-vorobeva thanks for the review, it's true that the rspec rule description is bad, but it was improved in this PR: https://github.com/SonarSource/rspec/pull/7179

Though there is no exception listed in the rule description because I think that either you fall into one of the exception, in which case you won't see this isuse or you have this issue raised in your code, then I don't think having a list of exceptions will help the developer to fix it. If he thinks that it's a FP because we don't handle the framework it's using, then he can reach out to us and we will investigate the FP. If for example it's just plain HTML, having a list of exceptions for specific frameworks is just noise.

Be aware that the rule description has been synced in sonar-html as part of a separate PR and it has been merged into master. See commit a8ae2d3

@erwan-leforestier-sonarsource erwan-leforestier-sonarsource force-pushed the fix/sonarhtml-395-handle-template-generated-labels branch from a495abf to 4e89f8e Compare June 26, 2026 13:13
@gitar-bot

gitar-bot Bot commented Jun 26, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 2 resolved / 2 findings

Updates S6853 to correctly ignore template-generated labels in Razor and Thymeleaf, resolving false positives from incorrect fragment detection and unhandled attribute associations. No issues remain.

✅ 2 resolved
Edge Case: th:attr recognized for control association but not accessible text

📄 sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/accessibility/LabelHasAssociatedControlCheck.java:98-109 📄 sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/accessibility/LabelHasAssociatedControlCheck.java:138-141
The new code recognizes Thymeleaf th:attr assignments for the for association (hasThymeleafAssignment(label, "for")), and recognizes accessible text via the dedicated attributes th:text/th:utext/v-text/v-html. However, hasAccessibleTextHint does not inspect th:attr assignments for accessible-text attributes such as aria-label/aria-labelledby/alt.

As a result a label whose accessible text is generated entirely through th:attr, e.g. <label th:attr="aria-label=#{label}, for=${id}"></label>, will still be flagged as missing accessible text (false positive): control association is detected via th:attr for, but the aria-label provided in the same th:attr bundle is not. This is the same false-positive class this PR aims to eliminate, just via the th:attr form for accessible-text attributes.

Consider extending hasAccessibleTextHint to also check hasThymeleafAssignment(node, "aria-label") / aria-labelledby / alt, mirroring the association handling. If th:attr-generated accessible text is intentionally out of scope, a code comment would help clarify the limitation.

Edge Case: Razor fragment detection runs on non-Razor files in characters()

📄 sonar-html-plugin/src/main/java/org/sonar/plugins/html/checks/accessibility/LabelHasAssociatedControlCheck.java:144-148
In characters(TextNode), the new Helpers.containsRazorFragmentRendering(textNode.getCode()) call runs for every label text node regardless of file type (it is not gated by Helpers.isRazorFile(...)). On a plain .html file, label body text that happens to contain a substring like @RenderBody, @RenderSection, or @Html.Partial (e.g. an email address or escaped text) would match RAZOR_FRAGMENT_EXPRESSION and set both foundControl and foundAccessibleLabel to true, suppressing a legitimate S6853 violation.

This mirrors the pre-existing RAZOR_CONTROL_PATTERN usage on the line above, so the risk is low and contrived, but since this is a newly added call it is worth considering gating it on isRazorFile(...) (or otherwise restricting it to Razor-capable files) to avoid false negatives on non-Razor templates.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown

@asya-vorobeva

Copy link
Copy Markdown

Hi @asya-vorobeva thanks for the review, it's true that the rspec rule description is bad, but it was improved in this PR: SonarSource/rspec#7179

Though there is no exception listed in the rule description because I think that either you fall into one of the exception, in which case you won't see this isuse or you have this issue raised in your code, then I don't think having a list of exceptions will help the developer to fix it. If he thinks that it's a FP because we don't handle the framework it's using, then he can reach out to us and we will investigate the FP. If for example it's just plain HTML, having a list of exceptions for specific frameworks is just noise.

Be aware that the rule description has been synced in sonar-html as part of a separate PR and it has been merged into master. See commit a8ae2d3

If you check descriptions of other rules, you'll see that we usually provide exceptions in case they exist. I think that documentation about the rule should be full. It's not for users, it's for us developers (ordinary users even don't have an access to full RSpec descriptions).

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