SONARHTML-441 Restrict S1082 to native HTML elements - #777
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Ruling ReportThe following ruling changes are in this PR: Rule:
|
victor-peixoto-sonarsource
left a comment
victor-peixoto-sonarsource
left a comment
There was a problem hiding this comment.
Good job. I left some comments
| || hasAttribute(node, "V-ON:" + eventName); | ||
| // Vue long form v-on:eventname, plus the @eventname shorthand the parser strips to a bare name | ||
| || hasAttribute(node, "V-ON:" + eventName) | ||
| || hasAttribute(node, eventName); |
There was a problem hiding this comment.
This line makes bare event names count as handlers for every event, but modifiers are still only
understood by the two KEY_*_WITH_KEY_NAME patterns. That asymmetry cuts both ways — a modifier on
the satisfying side hides a real handler, and one on the triggering side hides the trigger:
| Case | Current behaviour |
|---|---|
<div @mouseover="a" @focus.capture="b"> |
raises Add a 'onFocus'… — @focus.capture not recognised |
<div @mouseout="a" @blur.once="b"> |
raises Add a 'onBlur'… — @blur.once not recognised |
<div @click="a" @keydown.arrow-up="b"> |
raises — \w{1,10} rejects Vue's kebab-case key modifiers |
<div @click.prevent="a"> |
raises nothing — @click.prevent not recognised as a click handler |
<div v-on:click.prevent="a"> |
raises nothing — same |
Generalising the modifier grammar across all events fixes all five. The validation in the existing
patterns has to be preserved though — key names capped at 10 characters and 5 combined modifiers is
what keeps (keydown.enter,shift) and (keydown.undefinedkeyname) correctly reported (fixture lines
68, 70, 71). A naive "strip everything after the first dot" accepts those and breaks detected().
// Event modifiers / key names: Angular's (keydown.enter) pseudo-events and Vue's @click.prevent,
// @keydown.arrow-up. Key names are capped at 10 characters and 5 combined modifiers so that malformed
// bindings — (keydown.enter,shift), (keydown.undefinedkeyname) — are not mistaken for real handlers.
private static final String MODIFIERS = "(?:\\.[\\w-]{1,10}){0,5}";
private static final Map<String, Pattern> EVENT_PATTERNS = new ConcurrentHashMap<>();
private static boolean hasEventHandlerAttribute(TagNode node, String eventName) {
return hasAttribute(node, "ON" + eventName)
|| hasAttribute(node, "ON-" + eventName)
|| hasAttribute(node, "NG-" + eventName)
// Angular (eventname), Vue long form v-on:eventname, and the @eventname shorthand the parser
// strips to a bare name — each optionally carrying modifiers (.prevent, .enter, .arrow-up)
|| hasEventBinding(node, eventName);
}
private static boolean hasEventBinding(TagNode node, String eventName) {
Pattern pattern = EVENT_PATTERNS.computeIfAbsent(eventName, name -> Pattern.compile(
"\\(" + name + MODIFIERS + "\\)"
+ "|(?:v-on:)?" + name + MODIFIERS,
Pattern.CASE_INSENSITIVE));
return node.getAttributes().stream().anyMatch(a -> pattern.matcher(a.getName()).matches());
}This subsumes KEY_DOWN_WITH_KEY_NAME, KEY_UP_WITH_KEY_NAME, hasKeyDownWithKeyName,
hasKeyUpWithKeyName and the (eventName) branch, so all five can go — net −13 lines.
Applied locally at this branch's head: all five rows above behave correctly, the malformed key-name
cases stay reported, <a href="#" @click.prevent="x"> stays exempt, and the full module passes
(657 tests, 0 failures, this check's four tests unchanged).
It also looks ruling-neutral: of the 52 @click.<modifier> sites in its/sources, 40 are on
<el-dropdown-item> (custom element, excluded by the new gate) and 12 on <a @click.prevent>
(exempt via isHyperlink — and that exemption only engages properly once the modifier form is
recognised). Worth a ruling run to confirm, but no churn expected on top of the +59.
Suggest adding those five rows as fixture cases either way; none are covered today.
There was a problem hiding this comment.
Handled in 33ee23f. Event bindings now accept valid Angular/Vue modifiers consistently for every event, while retaining the existing modifier count and key-name length limits. The fixture covers all five cases from the review.
| 130 | ||
| ], | ||
| "project:voten/resources/assets/js/components/passport/PersonalAccessTokens.vue": [ | ||
| 101 |
There was a problem hiding this comment.
This entry is a false positive. PersonalAccessTokens.vue:101 is:
<input type="checkbox" @click="toggleScope(scope.id)" :checked="scopeIsAssigned(scope.id)">A native checkbox is keyboard-operable by definition — Space activates it and the HTML activation
behaviour fires a synthetic click, so the @click handler is the keyboard path. Asking for
onKeyDown/onKeyUp here is wrong.
The cause is isAnInteractiveRole, which contains only "textbox", so <textarea> and text inputs
are exempted via their implicit role while checkbox, radio, combobox and listbox fall through:
| Input | Result |
|---|---|
<input type="checkbox" @click="x"> |
flagged (FP) |
<input type="checkbox" onclick="x"> |
flagged — pre-existing on master |
<input type="radio" @click="x"> |
flagged (FP) |
<select @click="x"> |
flagged (debatable) |
<textarea @click="x"> |
clean (textbox role) |
Since onclick= already behaves this way the gap predates this PR — the Vue support just gives it a
first corpus hit. Either widen isAnInteractiveRole to the natively-activatable roles, or split it
into its own ticket; what I'd avoid is accepting this line into the expectation file as though it
were correct.
For the record, the other 58 additions all check out — <i>, <div>, <li>, <span>, <h1>,
<img> with @click and no keyboard equivalent, no role, no tabindex. I reconstructed each
flagged tag from the pinned corpus commit ef27916 to confirm.
There was a problem hiding this comment.
Handled in 33ee23f. Native implicit checkbox, radio, and listbox controls are now exempted; explicit ARIA roles still require keyboard handlers. Added native-control regression cases and removed the PersonalAccessTokens false positive from the ruling expectation.
210d690 to
079ba21
Compare
victor-peixoto-sonarsource
left a comment
victor-peixoto-sonarsource
left a comment
There was a problem hiding this comment.
LGTM. I left one question.
| // combinations to five modifiers so malformed bindings are not mistaken for event handlers. | ||
| private static final String EVENT_MODIFIERS = "(?:\\.[\\w-]{1,10}){0,5}"; | ||
| private static final Map<String, Pattern> EVENT_PATTERNS = new ConcurrentHashMap<>(); | ||
| private static final Set<String> NATIVELY_ACTIVATABLE_ROLES = Set.of("textbox", "checkbox", "radio", "listbox"); |
There was a problem hiding this comment.
Do we also need combobox?
There was a problem hiding this comment.
Good catch. A native single-select has an implicit combobox role, while multiple or size > 1 selects have listbox. I corrected the implicit-role mapping, included combobox among the natively keyboard-activatable roles, and added coverage for both forms. I also updated the rule description in RSPEC PR #7862 to cover the implicit combobox role and its listbox variants.
There was a problem hiding this comment.
Follow-up: I also updated the rule description in RSPEC PR #7862 to cover the implicit combobox role and its listbox variants.
…nt shorthands Restore the whitelistedElements rule property so a native element can be opted out of the check, now with an empty default since the native-HTML gate already excludes custom elements. Recognize the bare @eventname Vue shorthand for click, mouseover, mouseout, focus and blur by handling it in the shared event-handler helper, fixing the asymmetry where only keydown/keyup accepted it.
🤖 Generated with GitHub Actions
🤖 Generated with GitHub Actions
91a2f72 to
d9ae36d
Compare
|
Code Review ✅ Approved 2 resolved / 2 findingsRestricts rule S1082 to native HTML elements, addressing the redundant local-name check and whitelistedElements documentation findings while adding support for Vue mouse-event shorthands. ✅ 2 resolved✅ Quality: Redundant local-name check before hasKnownHTMLTag gate
✅ Quality: whitelistedElements docs contradict its actual behavior
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
Summary
S1082 now evaluates mouse-event keyboard equivalence only for known native HTML elements, avoiding false positives on framework and design-system components. A native-element allowlist remains available for explicit opt-outs, and Vue mouse-event shorthands are recognized.
RSPEC PR: https://github.com/SonarSource/rspec/pull/7862
Changes
whitelistedElementsfor native-element opt-outs and document its full scope.Functional Validation
Artifact: SONARHTML-441-fv.zip
Once the file is attached to the PR description, unzip and run:
./run.sh
Expected output: