Skip to content

fix(table): decouple item drop target highlight from cell measurement state - #10472

Open
starboyvarun wants to merge 1 commit into
adobe:mainfrom
starboyvarun:fix/table-item-drop-target-highlight
Open

fix(table): decouple item drop target highlight from cell measurement state#10472
starboyvarun wants to merge 1 commit into
adobe:mainfrom
starboyvarun:fix/table-item-drop-target-highlight

Conversation

@starboyvarun

Copy link
Copy Markdown

Re: #5404

Intent

#5404 reports that a TableView item drop target shows no highlight while the root drop target does. I set out to fix it, but the first thing I did was try to reproduce it on main — and I can't. I believe #5404 is already fixed, incidentally, by the Virtualizer refactor in #6451.

So this PR does two things instead:

  1. Adds the regression coverage that was missing (which is why this regressed silently and the issue sat open).
  2. Fixes a separate, still-live defect in the same rule that reintroduces the exact same symptom under different conditions.

I'd rather bring you evidence than a speculative fix. If you disagree with the analysis below, I'm happy to keep digging — and if you agree, #5404 can be closed once this lands.

Why I think #5404 is already fixed

The original diagnosis in the issue thread was @reidbarber's: dropState is stale when used in renderWrapper.

The mechanism, for the record: TableView splits the drop highlight across two elements, because cell wrappers have opaque backgrounds (they have to, so sticky cells don't show other cells through them). The row carries the inset border (react-spectrum-Table-row--dropTarget), the cell wrappers carry the background tint (react-spectrum-Table-cellWrapper--dropTarget). At the time, the cell wrapper's isDropTarget was computed in a closure inside renderWrapper, which was handed to useVirtualizerState and whose output the old Virtualizer cached per ReusableView. Hovering a new drop target invalidates nothing — same collection, no scroll, no layout change — so the cached wrappers replayed a stale dropState. TableRow, being a real component reading TableContext, updated normally. The border was applied and then painted over by unchanged opaque white. That also explains why "the computed styles look right": the row was correct; the elements covering it were not. Root drops were unaffected because that highlight lives on .react-spectrum-Table-body, outside the cached tree.

#6451 removed both halves of that: TableCellWrapper is now a real component reading dropState from TableContext, and TableVirtualizer builds the wrapper tree inline on every render rather than consuming cached views.

I verified rather than assumed — a probe test firing a real dragover onto a folder row, dumping the classes on the row and its wrappers across four configurations:

[default]           row= … react-spectrum-Table-row--dropTarget
[default]       wrapper= … react-spectrum-Table-cellWrapper--dropTarget
[quiet]              … same
[overflowMode=wrap]  … same
[density=spacious]   … same

Both levels update in all of them.

The defect this actually fixes

While confirming the above I found a live problem in the same CSS rule:

.react-spectrum-Table-cellWrapper.react-spectrum-Table-cellWrapper--dropTarget {
  background-color: var(--spectrum-table-droptarget-background-color);
}

The chained base class is there purely for specificity — #4483 added it so the rule beats .spectrum-Table--quiet .spectrum-Table-row .spectrum-Table-cellWrapper at (0,3,0). But react-spectrum-Table-cellWrapper is not a static hook. It's toggled by layout state:

// TableViewBase.tsx
'react-spectrum-Table-cellWrapper': !layoutInfo.estimatedSize,
'react-spectrum-Table-cellWrapper--dropTarget': isDropTarget || isRootDroptarget

estimatedSize is true whenever a cell height is a guess rather than a measurement — initial layout, and again on any re-layout where the column width changed (TableLayout.ts#L332). In those windows the modifier class is present, the JS is entirely correct, and the selector just stops matching. The item highlight disappears while the root highlight (on .react-spectrum-Table-body) keeps working — the same confusing signature as #5404, with nothing in the DOM to explain it.

The fix takes the specificity from an ancestor that can't be toggled off:

.react-spectrum-Table .react-spectrum-Table-row .react-spectrum-Table-cellWrapper--dropTarget {
  background-color: var(--spectrum-table-droptarget-background-color);
}

Still (0,3,0), and table.css is imported after @adobe/spectrum-css-temp in TableViewBase.tsx, so it still wins the tie against the quiet variant.

Happy to be told the right call is instead to make the base class unconditional and give the "height is measured" case its own class — that's the other way to break the coupling, it's just a wider change.

Tests

Nothing anywhere asserted these classes, which is how this regressed unnoticed. Two tests in TableDnd.test.js, pointer and keyboard, asserting the row and every one of its cell wrappers:

  • the targeted row and all its wrappers get the drop target classes
  • the highlight follows the target rather than being left behind on a stale one
  • it clears when the drag ends

I mutation-tested them: stubbing the cell wrapper's isDropTarget to false fails both, so they would have caught the original #5404 bug.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests). — tests added; no new story, the existing DragOntoRowExample already covers this visual state and no new state is introduced.
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component). — no docs change; no API surface change.
  • Looked at the Accessibility Practices for this feature - Aria Practices
  • I understand every change in this PR and can explain why it's there.

📝 Test Instructions:

Storybook → TableView/Drag and DropDrag onto row.

  1. Drag a row over "Folder 1" or "Folder 2". The row should show a blue inset border and a blue tint across all of its cells.
  2. Drag between a folder row and a non-folder row — the highlight should move/clear rather than sticking to the previous target.
  3. Repeat with isQuiet, and with overflowMode="wrap" (the case the CSS fix targets — resize a column mid-drag to force a re-estimate).
  4. Keyboard: focus a row, to the drag handle, Enter to start a drag, then arrow through drop targets.

Tested: mouse + keyboard, light + dark, LTR, default/quiet/wrap/spacious. Not tested: RTL, high-contrast (the forced-colors block sets the drop target background to transparent, so this rule is a no-op there), screen reader, zoom.

yarn jest packages/@adobe/react-spectrum/test/table/TableDnd.test.js — 80/80 pass locally. I was not able to run the React 16/17 matrix locally (unrelated local node_modules corruption), so I'm relying on CI for those.

🧢 Your Project:

Personal / open source contribution

…t state

The drop target background for a TableView row is painted on its cell wrappers,
because those are opaque and sit on top of the row. The rule selected them with
`.react-spectrum-Table-cellWrapper.react-spectrum-Table-cellWrapper--dropTarget`,
chaining the base class purely for specificity. But that base class is applied
conditionally on `!layoutInfo.estimatedSize`, so any time a cell height is still
an estimate the selector stops matching and the row silently loses its
highlight, even though the modifier class is present. Select on the modifier
alone and take the specificity from a `.react-spectrum-Table` ancestor instead,
so it still wins over the quiet variant's cell wrapper background.

Also adds the regression coverage this area was missing (see adobe#5404): pointer and
keyboard drags now assert that the targeted row and each of its cell wrappers
get the drop target classes, that the highlight follows the target rather than
being left behind on a stale one, and that it clears when the drag ends.
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