fix(table): decouple item drop target highlight from cell measurement state - #10472
Open
starboyvarun wants to merge 1 commit into
Open
fix(table): decouple item drop target highlight from cell measurement state#10472starboyvarun wants to merge 1 commit into
starboyvarun wants to merge 1 commit into
Conversation
…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.
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
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.
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:
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:
dropStateis stale when used inrenderWrapper.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'sisDropTargetwas computed in a closure insiderenderWrapper, which was handed touseVirtualizerStateand whose output the old Virtualizer cached perReusableView. Hovering a new drop target invalidates nothing — same collection, no scroll, no layout change — so the cached wrappers replayed a staledropState.TableRow, being a real component readingTableContext, 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:
TableCellWrapperis now a real component readingdropStatefromTableContext, andTableVirtualizerbuilds the wrapper tree inline on every render rather than consuming cached views.I verified rather than assumed — a probe test firing a real
dragoveronto a folder row, dumping the classes on the row and its wrappers across four configurations: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:
The chained base class is there purely for specificity — #4483 added it so the rule beats
.spectrum-Table--quiet .spectrum-Table-row .spectrum-Table-cellWrapperat(0,3,0). Butreact-spectrum-Table-cellWrapperis not a static hook. It's toggled by layout state:estimatedSizeis 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:
Still
(0,3,0), andtable.cssis imported after@adobe/spectrum-css-tempinTableViewBase.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:I mutation-tested them: stubbing the cell wrapper's
isDropTargettofalsefails both, so they would have caught the original #5404 bug.✅ Pull Request Checklist:
DragOntoRowExamplealready covers this visual state and no new state is introduced.📝 Test Instructions:
Storybook →
TableView/Drag and Drop→ Drag onto row.isQuiet, and withoverflowMode="wrap"(the case the CSS fix targets — resize a column mid-drag to force a re-estimate).Tested: mouse + keyboard, light + dark, LTR, default/quiet/wrap/spacious. Not tested: RTL, high-contrast (the
forced-colorsblock 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