fix(eslint-plugin-template): any valid DOM element with role button is interactive #2488
Conversation
|
View your CI Pipeline Execution ↗ for commit 6be30b4.
☁️ Nx Cloud last updated this comment at |
| @@ -547,6 +547,9 @@ interface Options { | |||
| <a href="#" (click)="onClick()"></a> | |||
| <a [attr.href]="href" class="anchor" (click)="onClick()"></a> | |||
| <a [routerLink]="'route'" (click)="onClick()"></a> | |||
| <div role="button" (click)="doSomething()"></div> | |||
| <span role="button" (click)="doSomething()"></span> | |||
| <p role="button" (click)="doSomething()"></p> | |||
There was a problem hiding this comment.
FYI @sandikbarr I am rushing this one a little bit to get the fix out in 19.x, I wasn't 100% if existing utils allowed me to figure this out so I put in a hardcoded check for role of button
There was a problem hiding this comment.
@JamesHenry those elements with role button won’t behave like a button that would have built in keyboard support.
There was a problem hiding this comment.
For sure, but for this rule, I think it makes sense not to report? It will be caught by other rules for not having a tabindex etc
There was a problem hiding this comment.
To fix this lint issue you would have to add those event handlers and then you would process the event multiple times
There was a problem hiding this comment.
As reported here: #1996 (comment)
There was a problem hiding this comment.
That’s a fair point. Interactive-supports-focus will catch it for not being focusable, but interactive-supports-focus won’t enforce keyboard support.
| @@ -44,7 +44,7 @@ function checkIsNonInteractiveRole(node: TmplAstElement): boolean { | |||
| * has a dynamic handler on it and we need to discern whether or not | |||
| * it's intention is to be interacted with on the DOM. | |||
| */ | |||
| export function isInteractiveElement(node: TmplAstElement): boolean { | |||
| export function isInherentlyInteractiveElement(node: TmplAstElement): boolean { | |||
There was a problem hiding this comment.
Renamed for clarity, I didn't understand it at first, and I added my role=button check in here which caused a bunch of regressions in other tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
==========================================
+ Coverage 92.85% 92.87% +0.02%
==========================================
Files 200 200
Lines 4169 4182 +13
Branches 973 977 +4
==========================================
+ Hits 3871 3884 +13
Misses 229 229
Partials 69 69
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
No description provided.