Skip to content

fix: warn on click handlers for table row and cell elements#17990

Open
RazinShafayet2007 wants to merge 3 commits into
sveltejs:mainfrom
RazinShafayet2007:fix/a11y-click-events-table-row
Open

fix: warn on click handlers for table row and cell elements#17990
RazinShafayet2007 wants to merge 3 commits into
sveltejs:mainfrom
RazinShafayet2007:fix/a11y-click-events-table-row

Conversation

@RazinShafayet2007
Copy link
Copy Markdown
Contributor

@RazinShafayet2007 RazinShafayet2007 commented Mar 23, 2026

Fixes #17963

This fixes a gap in the a11y_click_events_have_key_events compiler warning for table elements with click handlers. Previously, visible non-interactive elements like div, span, and section correctly warned, but tr did not because table row and cell elements were treated as interactive enough to bypass this warning path. This change narrowly updates the warning check so that tr, td, and th are included.

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 23, 2026

🦋 Changeset detected

Latest commit: d81342a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Copy Markdown
Member

Interesting — any idea why these elements are being treated as interactive in the first place? Is that a (possibly upstream) bug?

@afurm
Copy link
Copy Markdown

afurm commented Mar 23, 2026

The logic (!is_interactive || is_table_cell_or_row) correctly overrides the interactive bypass for these elements. One edge case: if someone legitimately assigns role="button" to a td, this will still warn. Might be worth checking whether is_non_presentation_role already handles that, or if an explicit carve-out is needed for non-table roles on these elements.

@paoloricciuti
Copy link
Copy Markdown
Member

I think we should rather fix the fact that those were considered interactive (or figure out if they are indeed interactive) and fix that inside packages/svelte/src/compiler/phases/2-analyze/visitors/shared/a11y/index.js rather that with an extra check.

@RazinShafayet2007
Copy link
Copy Markdown
Contributor Author

Interesting — any idea why these elements are being treated as interactive in the first place? Is that a (possibly upstream) bug?

I dug into this more, and it does look like a classification issue rather than an intentional exception for table elements.
tr, td, and th are currently ending up as interactive through element_interactivity(...):

  • tr maps to row, and aria-query gives row a widget superclass
  • td matches both cell and the constrained gridcell schema because match_schema(...) ignores constraints; axobject-query also maps bare td to CellRole, which is typed as a widget
  • th maps to ColumnHeaderRole / RowHeaderRole in axobject-query, which are also typed as widget
    So this looks like a classification mismatch, possibly influenced by upstream role/object data, plus how Svelte currently derives interactivity from it.

The logic (!is_interactive || is_table_cell_or_row) correctly overrides the interactive bypass for these elements. One edge case: if someone legitimately assigns role="button" to a td, this will still warn. Might be worth checking whether is_non_presentation_role already handles that, or if an explicit carve-out is needed for non-table roles on these elements.

Good catch — I checked that edge case.
With my current patch:

  • <td role="button" onclick={...}> still warns for a11y_click_events_have_key_events
  • <td role="button" tabindex="0" onclick={...} onkeydown={...}> ends up with no warnings
    For comparison:
  • <div role="button" onclick={...} onkeydown={...}> still gets a11y_interactive_supports_focus
    So I agree this is a sign that the local bypass fixes the reported warning, but not the underlying classification.

I think we should rather fix the fact that those were considered interactive (or figure out if they are indeed interactive) and fix that inside packages/svelte/src/compiler/phases/2-analyze/visitors/shared/a11y/index.js rather that with an extra check.

Agreed. After digging into it more, I think the better fix is to correct why these elements are considered interactive in the first place rather than keep the extra is_table_cell_or_row bypass.

@RazinShafayet2007 RazinShafayet2007 force-pushed the fix/a11y-click-events-table-row branch from e9d45ed to 29b0f48 Compare March 23, 2026 18:11
@RazinShafayet2007
Copy link
Copy Markdown
Contributor Author

RazinShafayet2007 commented Mar 23, 2026

I chose to fix this in index.js because it gives a narrower, behavior-level correction in the analyzer flow and directly resolves the reported issue. I did also consider constants.js, since that would change the broader interactivity model for these elements across the a11y system. That approach could potentially address more related cases, but it also seemed riskier because it would have a wider effect on existing classification logic. Do you think I should explore the broader constants.js route instead?

@RazinShafayet2007 RazinShafayet2007 force-pushed the fix/a11y-click-events-table-row branch from 187d353 to e7b28ba Compare March 25, 2026 05:43
@RazinShafayet2007 RazinShafayet2007 force-pushed the fix/a11y-click-events-table-row branch from e7b28ba to d81342a Compare March 25, 2026 18:03
Copy link
Copy Markdown

@codeCraft-Ritik codeCraft-Ritik left a comment

Choose a reason for hiding this comment

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

The Explanation: Excellent. Explaining that tr, td, and th were previously treated as "interactive enough" to bypass the warning is great context.

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.

Table elements missing a11y_click_events_have_key_events compiler warnings

5 participants