Skip to content

Commit 719b98c

Browse files
committed
fix(react): dropdown audit fixes — locked-row hover cancel, stale focus-key reset
Ports Solid's f93f9a3/ea1e0dd audit fixes to React, plus one React-specific fix surfaced while porting: - Sort dropdown's non-draggable "Group order" rows now cancel the hover background (a cue that dragging would do something there, which it wouldn't — the row's own section heading + hint text already explain why) while keeping the focus background, since the row is still clickable/keyboard-toggleable. ddRowHighlighted/ddRowHoverFocusHandlers both gained a hoverable option for this. - Filter dropdown's Escape-clear-search-term bug (checked only the currently active column, not where focus actually was) doesn't apply to React's architecture: its value-search Escape handler is already bound directly to that exact input's own onKeyDown, so it can only ever fire while that specific input has focus. No fix needed there. - React-specific: closing a dropdown panel (it unmounts entirely) doesn't reliably fire a blur event on whatever row was focused inside it, leaving a stale hoveredDdRowKey/focusedDdRowKey that could wrongly match a same-keyed row in a different dropdown opened right after (e.g. grouping a column then immediately opening Sort, whose "Group order" row shares that key). Fixed by resetting both keys whenever which dropdown is open changes, via the render-time "adjust state when a prop changes" pattern already used elsewhere in this file (not an effect, to avoid the react-hooks/set-state-in-effect lint error).
1 parent 58d3c17 commit 719b98c

2 files changed

Lines changed: 53 additions & 7 deletions

File tree

packages/react/src/DataTableView.tsx

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -900,13 +900,37 @@ export function DataTableView<TRow extends object>({
900900
// dropdown is ever open at a time, so a single pair of keys (not one per dropdown) is enough.
901901
const [hoveredDdRowKey, setHoveredDdRowKey] = useState<string | null>(null)
902902
const [focusedDdRowKey, setFocusedDdRowKey] = useState<string | null>(null)
903-
function ddRowHighlighted(key: string): boolean {
904-
return hoveredDdRowKey === key || focusedDdRowKey === key
903+
// A closed dropdown panel unmounts entirely (see Dropdown.tsx), but that doesn't reliably fire a
904+
// blur event on whatever row was focused inside it — so without this, closing a dropdown while
905+
// one of its rows is focused/hovered can leave a stale key here, which then wrongly matches a
906+
// same-keyed row in a *different* dropdown opened right after (e.g. grouping "dept" then
907+
// immediately opening Sort, whose "Group order" row shares that same key). Resetting whenever
908+
// which dropdown is open changes keeps this scoped to whichever one is actually open right now
909+
// — the same "adjust state when a prop changes" render-time pattern as `sameKeySet`'s
910+
// `prevColumns` comparison above, rather than an effect (avoiding both an extra render and the
911+
// react-hooks/set-state-in-effect lint error this project's config treats as one).
912+
const openDdTriple = `${openColsDD}:${openSortDD}:${openGroupDD}`
913+
const [prevOpenDdTriple, setPrevOpenDdTriple] = useState(openDdTriple)
914+
if (openDdTriple !== prevOpenDdTriple) {
915+
setPrevOpenDdTriple(openDdTriple)
916+
setHoveredDdRowKey(null)
917+
setFocusedDdRowKey(null)
905918
}
906-
function ddRowHoverFocusHandlers(key: string) {
919+
// `hoverable = false` is for Sort's non-draggable "Group order" rows: a hover highlight would
920+
// read as "dragging would do something here," which it wouldn't — its own section heading +
921+
// hint text already explain why, so this only cancels the hover cue, not the focus one (the row
922+
// is still clickable/keyboard-toggleable, so focus stays a meaningful cue).
923+
function ddRowHighlighted(key: string, { hoverable = true } = {}): boolean {
924+
return (hoverable && hoveredDdRowKey === key) || focusedDdRowKey === key
925+
}
926+
function ddRowHoverFocusHandlers(key: string, { hoverable = true } = {}) {
907927
return {
908-
onMouseEnter: () => setHoveredDdRowKey(key),
909-
onMouseLeave: () => setHoveredDdRowKey((k) => (k === key ? null : k)),
928+
...(hoverable
929+
? {
930+
onMouseEnter: () => setHoveredDdRowKey(key),
931+
onMouseLeave: () => setHoveredDdRowKey((k) => (k === key ? null : k)),
932+
}
933+
: {}),
910934
onFocus: () => setFocusedDdRowKey(key),
911935
onBlur: () => setFocusedDdRowKey((k) => (k === key ? null : k)),
912936
}
@@ -2145,11 +2169,11 @@ export function DataTableView<TRow extends object>({
21452169
removeSort(entry.key)
21462170
}
21472171
}}
2148-
{...ddRowHoverFocusHandlers(entry.key)}
2172+
{...ddRowHoverFocusHandlers(entry.key, { hoverable: false })}
21492173
style={{
21502174
...S.ddItem,
21512175
justifyContent: 'space-between',
2152-
background: ddRowHighlighted(entry.key)
2176+
background: ddRowHighlighted(entry.key, { hoverable: false })
21532177
? 'var(--color-background-secondary)'
21542178
: undefined,
21552179
}}

packages/react/src/__tests__/dropdownSearchNav.test.tsx

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,28 @@ describe('DataTable — Sort/Group/Columns active-row hover/focus background', (
577577
fireEvent.focus(colRow)
578578
expect(colRow.style.background).toBe('var(--color-background-secondary)')
579579
})
580+
581+
// A "Group order" row is still clickable to toggle direction, just not draggable/Alt+Arrow-
582+
// reorderable — its own section heading + hint text already explain why, so it cancels only the
583+
// hover cue (which would read as "dragging would do something here," which it wouldn't), not the
584+
// focus one (the row is still keyboard-toggleable, so focus stays a meaningful cue).
585+
it('a "Group order" row gets the focus background but not the hover one', () => {
586+
const { getByText, getAllByText, container } = render(
587+
<DataTable data={ROWS} columns={COLS} rowKey="id" />,
588+
)
589+
fireEvent.click(getByText('Group'))
590+
fireEvent.click(ddCopyOf(getAllByText, 'Dept'))
591+
fireEvent.click(getByText('Group')) // close
592+
fireEvent.click(getByText('Sort'))
593+
const groupOrderRow = [...container.querySelectorAll<HTMLElement>('[tabindex="0"]')].find(
594+
(el) => el.textContent?.includes('Dept') && !el.hasAttribute('data-sort-key'),
595+
)!
596+
fireEvent.mouseEnter(groupOrderRow)
597+
expect(groupOrderRow.style.background).toBe('')
598+
fireEvent.mouseLeave(groupOrderRow)
599+
fireEvent.focus(groupOrderRow)
600+
expect(groupOrderRow.style.background).toBe('var(--color-background-secondary)')
601+
})
580602
})
581603

582604
const CATEGORIZED_COLS: ColumnDef<Row>[] = [

0 commit comments

Comments
 (0)