Skip to content

fix: prevent trailing interactive controls being swallowed by EntityR…#819

Open
mikefreno wants to merge 1 commit intopaperclipai:masterfrom
mikefreno:fix/entityrow-click-area
Open

fix: prevent trailing interactive controls being swallowed by EntityR…#819
mikefreno wants to merge 1 commit intopaperclipai:masterfrom
mikefreno:fix/entityrow-click-area

Conversation

@mikefreno
Copy link

When a row has both a navigation target (to=) and trailing content (buttons, menus), the Link element previously covered the entire row, making trailing controls unclickable. Split the layout so a decorative full-bleed Link sits behind the content (z-index / aria-hidden) while trailing children render at a higher z-index in normal document flow.

…ow link

When a row has both a navigation target (to=) and trailing content
(buttons, menus), the Link element previously covered the entire row,
making trailing controls unclickable. Split the layout so a decorative
full-bleed Link sits behind the content (z-index / aria-hidden) while
trailing children render at a higher z-index in normal document flow.
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR fixes a real UX bug where an all-encompassing <Link> element in EntityRow was preventing trailing interactive controls (buttons, menus) from receiving click events when both to and trailing props were supplied. The fix introduces a new rendering branch that places a full-bleed absolutely-positioned Link behind the row content while rendering trailing children at z-index: 10, which is the correct structural approach for this "card with link + action" pattern.

Key observations:

  • The core z-index layering strategy is sound and the visual/pointer-events behaviour is correct for mouse users.
  • The absolute <Link> is given both aria-hidden="true" and tabIndex={-1}, which fully removes it from the accessibility tree and the tab order. Keyboard-only and screen-reader users lose the ability to navigate to the to destination in all rows that use both props. Removing aria-hidden (keeping tabIndex={-1}) and adding aria-label={title} would restore accessibility while preserving the desired tab flow.
  • The inline comment describing the layout refers to the Link covering "the leading + body (left side)", but the Link is actually inset-0 and covers the entire row.
  • Existing callers (e.g. Projects.tsx, GoalDetail.tsx, MyIssues.tsx) currently pass only non-interactive trailing content (status badges, date labels), so the functional regression is limited today — but the accessibility gap will affect any future caller that passes interactive trailing elements.

Confidence Score: 3/5

  • Safe to merge for sighted mouse users, but introduces an accessibility regression that should be addressed before shipping.
  • The pointer-events fix is correct and the change is isolated to a single component. However, aria-hidden="true" + tabIndex={-1} on the navigation Link is a meaningful WCAG regression — keyboard and screen-reader users can no longer reach the link target in any row that uses both to and trailing.
  • ui/src/components/EntityRow.tsx — specifically the to && trailing rendering branch (lines 64-81).

Important Files Changed

Filename Overview
ui/src/components/EntityRow.tsx Introduces a new rendering branch for the to + trailing case using an absolutely-positioned full-bleed Link behind the row content. Correctly fixes trailing interactive controls being swallowed, but the Link is marked aria-hidden="true" + tabIndex={-1}, completely removing keyboard and screen-reader access to the navigation target.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: ui/src/components/EntityRow.tsx
Line: 67-72

Comment:
**Keyboard & screen-reader navigation is broken for the `to + trailing` case**

`aria-hidden="true"` combined with `tabIndex={-1}` completely removes the Link from both the tab order *and* from the accessibility tree. This means:
- Keyboard-only users who navigate with Tab can never reach the row's navigation target — neither the outer `<div>` nor the `<Link>` are focusable.
- Screen-reader users who rely on the accessibility tree can't discover or activate the link at all.

Removing `aria-hidden` (while keeping `tabIndex={-1}`) is the minimal fix: the link stays out of the Tab sequence (so focus jumps to trailing controls naturally) but remains discoverable via arrow-key navigation in screen readers. Adding an `aria-label` that mirrors the row title would further improve the experience:

```suggestion
        <Link
          to={to}
          className="absolute inset-0 no-underline"
          onClick={onClick}
          tabIndex={-1}
          aria-label={title}
        />
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: ui/src/components/EntityRow.tsx
Line: 60-62

Comment:
**Comment describes layout inaccurately**

The comment says "a Link covers the leading + body (left side)" but the Link actually uses `absolute inset-0`, which means it covers the *entire* row (including the trailing area). The trailing wrapper renders above it via `z-10`. Consider updating the comment to avoid future confusion:

```suggestion
  // When there is both a nav target and trailing interactive content, split the
  // row: a full-bleed absolute Link sits behind all content (z-index: auto),
  // while trailing children render at z-10 so their buttons/menus are never
  // swallowed by the link's click area.
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: fcc0977

Comment on lines +67 to +72
<Link
to={to}
className="absolute inset-0 no-underline"
onClick={onClick}
tabIndex={-1}
aria-hidden="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Keyboard & screen-reader navigation is broken for the to + trailing case

aria-hidden="true" combined with tabIndex={-1} completely removes the Link from both the tab order and from the accessibility tree. This means:

  • Keyboard-only users who navigate with Tab can never reach the row's navigation target — neither the outer <div> nor the <Link> are focusable.
  • Screen-reader users who rely on the accessibility tree can't discover or activate the link at all.

Removing aria-hidden (while keeping tabIndex={-1}) is the minimal fix: the link stays out of the Tab sequence (so focus jumps to trailing controls naturally) but remains discoverable via arrow-key navigation in screen readers. Adding an aria-label that mirrors the row title would further improve the experience:

Suggested change
<Link
to={to}
className="absolute inset-0 no-underline"
onClick={onClick}
tabIndex={-1}
aria-hidden="true"
<Link
to={to}
className="absolute inset-0 no-underline"
onClick={onClick}
tabIndex={-1}
aria-label={title}
/>
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/EntityRow.tsx
Line: 67-72

Comment:
**Keyboard & screen-reader navigation is broken for the `to + trailing` case**

`aria-hidden="true"` combined with `tabIndex={-1}` completely removes the Link from both the tab order *and* from the accessibility tree. This means:
- Keyboard-only users who navigate with Tab can never reach the row's navigation target — neither the outer `<div>` nor the `<Link>` are focusable.
- Screen-reader users who rely on the accessibility tree can't discover or activate the link at all.

Removing `aria-hidden` (while keeping `tabIndex={-1}`) is the minimal fix: the link stays out of the Tab sequence (so focus jumps to trailing controls naturally) but remains discoverable via arrow-key navigation in screen readers. Adding an `aria-label` that mirrors the row title would further improve the experience:

```suggestion
        <Link
          to={to}
          className="absolute inset-0 no-underline"
          onClick={onClick}
          tabIndex={-1}
          aria-label={title}
        />
```

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +60 to +62
// When there is both a nav target and trailing interactive content, split the
// row: a Link covers the leading + body (left side), while trailing sits in
// normal flow at a higher z-index so its buttons/menus are never swallowed
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment describes layout inaccurately

The comment says "a Link covers the leading + body (left side)" but the Link actually uses absolute inset-0, which means it covers the entire row (including the trailing area). The trailing wrapper renders above it via z-10. Consider updating the comment to avoid future confusion:

Suggested change
// When there is both a nav target and trailing interactive content, split the
// row: a Link covers the leading + body (left side), while trailing sits in
// normal flow at a higher z-index so its buttons/menus are never swallowed
// When there is both a nav target and trailing interactive content, split the
// row: a full-bleed absolute Link sits behind all content (z-index: auto),
// while trailing children render at z-10 so their buttons/menus are never
// swallowed by the link's click area.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/components/EntityRow.tsx
Line: 60-62

Comment:
**Comment describes layout inaccurately**

The comment says "a Link covers the leading + body (left side)" but the Link actually uses `absolute inset-0`, which means it covers the *entire* row (including the trailing area). The trailing wrapper renders above it via `z-10`. Consider updating the comment to avoid future confusion:

```suggestion
  // When there is both a nav target and trailing interactive content, split the
  // row: a full-bleed absolute Link sits behind all content (z-index: auto),
  // while trailing children render at z-10 so their buttons/menus are never
  // swallowed by the link's click area.
```

How can I resolve this? If you propose a fix, please make it concise.

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.

1 participant