bugfix(tablev2): keep row clicks, header tracks and header labels honest - #1192
Draft
JeanMarcMilletScality wants to merge 5 commits into
Draft
bugfix(tablev2): keep row clicks, header tracks and header labels honest#1192JeanMarcMilletScality wants to merge 5 commits into
JeanMarcMilletScality wants to merge 5 commits into
Conversation
Three defects in the same component, all measured rather than reasoned about. **A row's handler swallowed clicks meant for a control inside a cell.** The row `onClick` in both selectable contents fired for any click anywhere in the row with no target check, so clicking a button in a cell both activated the button and selected the row — and the selection re-render remounts the memoized row, unmounting whatever the button had just opened. The `+N` dropped-columns trigger is core-ui hitting itself with this. `onKeyDown` had it worse: `keydown` bubbles too, so Enter on a focused in-cell control selected the row *and* `preventDefault()`ed the control's own activation. Both handlers now bail on an interactive target, via one `isInteractiveTarget` in `TableCommon` so the two copies cannot drift. The multi-selectable selection cell no longer relies on the bubble the guard stops: it owns its click outright. That is behaviour-preserving — its two former paths, direct in single-row mode and bubbling to the row handler's else branch otherwise, ended in the same call with the same arguments. **A multi-selectable table's header tracks disagreed with its body tracks.** `TableRowMultiSelectable` never declared the `gap` that `HeadRow` and `TableRow` both do, so it computed `gap: normal`. Every column track has a grow factor and no basis, which turns the gap the header reserves and the body does not into free space the body redistributes — shifting each boundary by its grow share of the total gap, at any width. Measured on a three-column table with grow factors summing to 2.5: the header's three 14px gaps moved the columns by 25.20 / 8.41 / 8.40px, exactly 0.6 / 0.2 / 0.2 of 42px. One declaration; all deltas now zero. This also clears the two suspects that were on the list. The scrollbar compensation is fine — a single-selectable table with an identical 11px scrollbar aligned perfectly both before and after — and the header does receive `cellStyle`. **An ellipsized header gave no way to read the full label.** Body cells recover via `ConstrainedText`; the header was the one place a label became unreadable with no way back. `TruncatableHeaderLabel` measures the label and offers `title` only once it is actually cut off, re-measuring on resize because these columns size from grow factors. Wrapping in core-ui's `Tooltip` would have removed the truncation rather than explained it: `TooltipContainer` is an `inline-block` with no `min-width: 0`, so it replaces the ellipsizing flex item with one that cannot shrink below its min-content. The alignment story ships so the measurement can be repeated; jsdom has no layout, so it carries no jest assertion.
Contributor
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
The key names a private tracker project from a public repo. The test's own name already says what it covers.
The `gap` fix closed the header/body disagreement at wide widths only. Narrowing
the container reopened it: the reporter saw a 128px header column over a 104px
body column, and a width sweep put the onset at ~420px for a multi-selectable
table and ~340px for a single-selectable one.
`TableHeader` resets `min-width: 0` — "the header must never be the reason a
column is wider than its cells". The body cells never got the same reset, so they
sat at the flex default `min-width: auto`, whose automatic minimum is
content-based. Two symptoms follow from that one asymmetry:
- short body cells ("Attached", an action button) freeze at their content floor
while their headers keep shrinking, so those columns read wider in the body;
- flexbox then redistributes the frozen items' share of the negative free space
onto the row's only still-shrinkable cell, which over-shrinks *below* its own
header. That is the 128-vs-104 the reporter measured.
Both rows now shrink by the same rules. The reset goes before the `cellStyle`
spread, so a consumer's explicit `minWidth` still wins. Swept 736px down to
224px across all three selectable shapes: every header/body delta is now exactly
0, where before they diverged by up to 41px.
The alignment story becomes the verification surface for the whole change: four
panels, each in a draggable frame, and the two single-selectable ones now wire
`onRowSelected`/`selectedId` so row selection is actually visible — without it
the in-cell-click guard had nothing observable to demonstrate. Panel C adds
dropped columns with `revealDroppedColumns` so the `+N` trigger can be clicked on
an unselected row, which is the case that regressed.
Known limitation, unchanged in kind by this commit: a control that cannot shrink
overflows its column once the column is narrower than the control. With the floor
gone that becomes visible on the action column below ~260px. The remedy is
consumer-side — `iconOnly` on the action button — not a floor here, which would
put the columns back out of agreement.
…ected `TableRow` decided whether to paint the hover highlight and the pointer cursor from `$selectedId` — whether *something is currently selected* — which starts undefined. A selectable table therefore rendered with no affordance at all until after its first click, and nothing invited that click. It also made the fix for in-cell clicks impossible to demonstrate: the reveal stories pass no `onRowSelected`, so their rows were never selectable in the first place. The gate is now `$selectable`, meaning the content was given an `onRowSelected` — the same source it already uses for `tabIndex`. The selected-row highlight drops to `$isSelected` alone, since a selected row implies a selection exists. `Tablestyle` is not re-exported from `index.ts` or `next.ts`, so widening `TableRowType` is internal. Both responsive-column-drop stories now pass `onRowSelected`/`selectedId`, so the `+N` trigger can be exercised on a genuinely selectable row — verified: the popover opens, the row stays unselected, and the popover is still open after 500ms. The temporary header/body alignment story is removed now that the measurement it existed for is done; the two mechanisms it caught are covered by the commits before this one. Its prose, and the story-level prose the drop stories carried, moves into the Table guideline, where row selection, `dropAt`, `revealDroppedColumns` and the non-shrinking-control caveat are now documented — stories stay pure examples.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| target: EventTarget | null; | ||
| }): boolean => | ||
| event.target instanceof Element && | ||
| !!event.target.closest(INTERACTIVE_SELECTOR); |
Contributor
There was a problem hiding this comment.
.closest() walks the entire ancestor chain, not just within the row. If the table sits inside an element matching INTERACTIVE_SELECTOR (e.g. a [role="button"] container or a <label>), clicks on plain cell content would match the outer ancestor and silently suppress row selection.
Scoping to event.currentTarget avoids this:
Suggested change
| !!event.target.closest(INTERACTIVE_SELECTOR); | |
| !!event.target.closest(INTERACTIVE_SELECTOR)?.closest('[class*="tr"]') === null | |
| ? false | |
| : true; |
Actually, a cleaner fix — check the matched element is inside the handler's own row:
export const isInteractiveTarget = (event: {
target: EventTarget | null;
currentTarget: EventTarget | null;
}): boolean => {
if (!(event.target instanceof Element)) return false;
const hit = event.target.closest(INTERACTIVE_SELECTOR);
return !!hit && (event.currentTarget instanceof Element
? event.currentTarget.contains(hit)
: true);
};This way an ancestor <a> or [role="button"] wrapping the table won't disable row selection.
…dable Three things a review of the previous commits turned up, all verified in a browser rather than only in jsdom. **The row-click guard was unbounded in both directions.** It called `closest(INTERACTIVE_SELECTOR)` from the event target with no upper bound, so a table rendered inside a `<label>`, an `<a>` or a `[role="button"]` matched that ancestor for *every* cell and row selection stopped working entirely. In the other direction, a React portal leaves the row in the DOM but still bubbles to it through the React tree, so plain text in a portalled popover — the `revealDroppedColumns` panel — reached the row handler with nothing interactive in between and silently selected the row behind the open overlay. The search is now bounded to the row at both ends, and anything not contained by the row is not a click on the row. Renamed to `shouldIgnoreRowEvent`, since it answers a broader question than "is this a control". Both holes have a regression test, each confirmed to fail against the previous implementation. **An ellipsized header offered no way to read the full label.** Body cells recover through `ConstrainedText`; headers had nothing. They now show the label in a `Tooltip`, and only once it is actually cut off — a tooltip repeating a header that reads fine is noise. That needs a live measurement, because a column is sized by a grow factor and truncation onset moves with the table's width. The tooltip wrapper is mounted in every state and only `overlay` is conditional: mounting it on the flip would change the DOM under the element being measured and let the two states oscillate. A native `title` was tried first and rejected — the delay is drawn by the OS and cannot be configured, and react-table's `getSortByToggleProps()` already puts `title="Toggle SortBy"` on the header, so the two collided. **`HeaderLabel` relied on its parent for the ellipsis.** `overflow` and `text-overflow` are inert on an inline box, and a `span` is inline by default — it only worked because flex blockifies its children. Wrapping the label removed that and the truncation silently disappeared, with every test still green, because the tests stub `scrollWidth` and cannot see `display`. `display: block` is now stated on the label itself and asserted in a test that fails without it. Also, so the same rule is not written twice: the body cell's `min-width: 0` reset — the one that keeps header and body shrinking alike — moves into a shared `bodyCellStyle`, and the header-label story gains a label long enough to actually truncate, with the behaviour documented in the guideline rather than the story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR — Clicking a button inside a table row no longer also selects the row, a multi-selectable table's header columns now line up with its body columns, and a header label that gets cut off now offers its full text.
Context / Why
Three defects in
tablev2, grouped because they live in the same four files and would conflict as separate PRs. Two of them were found while narrowing tables down; the third — the row-click one — is core-ui hitting its own+Ndropped-columns trigger.🧩 Approach
Each one was measured, and in two cases the measurement contradicted the diagnosis it started from.
1. The row handler swallowed clicks meant for a cell's own controls
The row
onClickin both selectable contents fired for any click anywhere in the row, with no target check. Clicking a button in a cell activated the button and selected the row — and the selection re-render remounts the memoized row, so whatever the button just opened is unmounted immediately.onKeyDownwas worse:keydownbubbles too, so Enter on a focused in-cell control selected the row andpreventDefault()ed the control's own activation.Both handlers now bail on an interactive target, through a single
isInteractiveTargetinTableCommon— the two contents carry two copies of the same handler and must not drift.MultiSelectableContent's selection cell used to reach its behaviour two different ways: directly in single-row mode, and by bubbling to the row handler'selsebranch otherwise. Both ended in the same call with the same arguments, so it now owns its click outright rather than depending on the bubble the guard stops. Behaviour-preserving, and the three pre-existing selection tests confirm it.2. Multi-selectable header tracks disagreed with body tracks
TableRowMultiSelectablenever declared thegapthatHeadRowandTableRowboth do, so it computedgap: normal. Every column track has a grow factor and no basis, which turns a gap the header reserves and the body does not into free space the body redistributes — shifting every boundary by its grow share of the total gap, at any width.Measured on a three-column table with grow factors summing to 2.5 (
1.5 / 0.5 / 0.5), body-minus-header:deltaWidth42px is the header's three 14px gaps. One declaration closes it, and every delta is now exactly
0— not "under half a pixel", literally zero, from identical unrounded rects.This also clears the two suspects it started from. The scrollbar-compensation theory (
HeadRowsubtracts the scrollbar width,TableRowdoes not) is wrong: a single-selectable table with an identical 11px scrollbar aligned perfectly both before and after the fix. And header cells do receivecolumn.cellStyle, margins included.3. An ellipsized header gave no way to read the full label
Body cells recover via
ConstrainedText; the header was the one place a label became unreadable with nothing to get it back.TruncatableHeaderLabelmeasures the label and offerstitleonly once it is genuinely cut off — a tooltip on a header that reads fine is noise — re-measuring on resize, because these columns size from grow factors rather than fixed widths.titlerather than core-ui'sTooltipfor a structural reason worth stating:TooltipContaineris aninline-blockwith nomin-width: 0, so wrapping the label replaces the ellipsizing flex item with one that cannot shrink below its min-content — it would remove the truncation instead of explaining it. It also keeps the sort toggle onTableHeaderuntouched.📷 Screenshots
Multi-selectable header vs body, before and after the
gapdeclaration — the same story, same width. The header row is the one whose column boundaries move:🔍 Review focus
tablev2/SingleSelectableContent.tsx+MultiSelectableContent.tsx› rowonClick/onKeyDown— the one behavioural change here a consumer could depend on. Today, clicking a button inside a row also selects the row; after this, it does not. That is the bug, but it is a real behaviour change, and a consumer that has been relying on "a click anywhere in the row selects it" will notice. Consumers currently carrying astopPropagationon an interactive cell can drop it.tablev2/TableCommon.tsx › INTERACTIVE_SELECTOR— the guard is only as good as this list.labelis in it deliberately (clicking a label activates its control) and[role="button"]/[role="checkbox"]catch div-based triggers. Worth a look for anything a table cell renders that should be interactive and is not covered.tablev2/Tablestyle.tsx › TableRowMultiSelectable— the addedgapmust stay equal toHeadRow's. It is one line and it is now commented, but it is the kind of pairing that silently drifts.🧪 How to test
npm run storybook, open Components/Data Display/Table → Header Body Column Alignment. In all three panels, each header label should sit directly over its column. Panel C is the one that was broken.+Ntrigger appears, and click the trigger on an unselected row. The panel should open and stay open; previously the row selected itself, re-rendered, and closed it.🚧 Follow-up
organisms/attachments/AttachmentTable.tsxcarries amarginLeft: 'auto'that does nothing at a grow-sum above 1 but becomes live below it, and a<Box flex={0.5} />as aHeaderthat is a no-op. Neither causes the misalignment fixed here — the measurement cleared them — so both are left alone rather than widening this PR. Not yet ticketed.What changed
Tablestyle.tsxis the one-linegapfix.TableCommon.tsxgains the two shared pieces —isInteractiveTargetandTruncatableHeaderLabel— so both selectable contents use one implementation.SingleSelectableContent.tsxandMultiSelectableContent.tsxwire both in; the multi-selectable one also collapses its selection cell to a single path.stories/tablev2.stories.tsxaddsHeaderBodyColumnAlignment, reproducing a real three-column shape with and without a scrollbar and in both selectable modes. It ships so the measurement can be repeated — it carries no jest assertion, because jsdom has no layout and asserting a CSS declaration back out would prove nothing.