feat(a11y): add click-events-have-key-events rule

[mcmxcdev]

Implements click-events-have-key-events lint rule from #820.

[mcmxcdev]

There are currently two pre-existing test cases called catch-declares-error-variable and event-modifiers-redundant which are failing the CI now, because they use on:click without a key event.

Should I adapt these test cases to include a noop key event to pass the CI?

[mcmxcdev]

@skippednote I pushed another commit where I fixed the test cases, It is ready for review.

[PatrickG]

I don't think button elements (and button-like input elements (with type="button|submit|reset|...?")) need a key handler, it works as expected with only a click handler.

I think all those input types are working correctly: https://svelte.dev/repl/fc2107f1684b40c4b2ffe535522f68e3?version=3.23.2 (only tested in chromium)

[mcmxcdev]

@PatrickG you are right of course.

@benmccann I think a contributor from the core team should help out here and specify how strict or loose this rule should be implemented.

[benmccann]

hey @mhatvan just fyi, I primarily work on Sapper, so you'd probably have to ask someone else about issues in svelte core since I don't know enough about it yet

[mcmxcdev]

oh my bad, thought you are doing both! keep up the good work in Sapper ;)

[mcmxcdev]

I understand. So should I close the PR as this rule will not be included in svelte or how should we proceed further?

[Conduitry]

This can't be merged in its current form, so whether it should be closed is going to depend on how strongly you feel about this, and whether you feel like working through the ESLint plugin's implementation and figuring out what the corresponding behavior ought to be.

[mcmxcdev]

@Conduitry I improved the implementation to respect following cases:

• rule bails out on aria-hidden, type="hidden" or role="presentation" set by user
• rule bails out on a, button and select tags, because they work for keyboard out of the box with on:click listener set only
• rule bails out on a couple input types that work for keyboard out of the box with on:click listener set only
• rule bails out on props spread, because we cannot know what is passed on

[stale[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mcmxcdev]

@Conduitry I updated this PR a while ago to conform to the requirements, is there a chance to get this reviewed/approved?

[mcmxcdev]

Are we waiting for an approval from @Conduitry here or do we have to wait another year for this PR to get merged? ;)

[benmccann]

I didn't look super closely, but it generally seems reasonable to me. The one thing that jumped out to me though is that the code from eslint-plugin-jsx-a11y encapsulates its logic in helpers like isPresentationRole, isInteractiveElement, and isHiddenFromScreenReader. That seems like a good way to structure this because I wouldn't be surprised to see those methods shared across different checks. I wonder if there's an opportunity here to see if some of these bail outs can be refactored into methods shared by multiple checks

[mcmxcdev]

I wonder if there's an opportunity here to see if some of these bail outs can be refactored into methods shared by multiple checks

I am sure there is a lot that can be simplified by moving code out into helper functions. After all, Element.ts is almost 1000 LOC at this point.

I would suggest this refactoring task be done in a separate PR after all a11y-related PRs are merged to avoid merge conflicts. Probably, a core member should do this then, since it will require some more in-depth knowledge of the codebase.

[mcmxcdev]

I see various a11y-related PRs were merged to master branch recently. What about this one?

[Conduitry]

This has been (finally!) released in 3.51.0 - sorry about the extremely long wait!

[smblee]

How are folks resolving this error on svelte:element that can both be an a and button depending on the prop definition? per my implementation, the tag will only be a button if on:click is specified, which would be correct according to the spec.