fix: clear text selection on card drag start (#515)#557
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request addresses issue #515 by clearing active text selections when the card drag handle is clicked, ensuring the browser initiates a card drag instead of a text-drag. It introduces a handleDragHandleMouseDown function and applies user-select: none to the drag handle. The review feedback suggests adding an automated test to verify the selection clearing behavior and recommends including additional vendor prefixes for the user-select CSS property to improve cross-browser compatibility.
| function handleDragHandleMouseDown() { | ||
| // Clear any active text selection so the browser initiates a card drag | ||
| // instead of treating the interaction as a text-drag. Without this, a prior | ||
| // text selection inside the card prevents the drag handle from working until | ||
| // the selection is cleared by clicking elsewhere. Fixes #515. | ||
| window.getSelection()?.removeAllRanges() | ||
| } |
There was a problem hiding this comment.
To prevent future regressions, it would be beneficial to add an automated test case that covers this scenario. You can simulate text selection within the test and then verify that the selection is cleared when mousedown is triggered on the drag handle.
Here's an example of how you could write such a test using vitest and @vue/test-utils:
it('clears text selection on drag handle mousedown', async () => {
const wrapper = mount(CardItem, {
props: { card: createCard() },
});
const selection = window.getSelection();
if (!selection) {
// JSDOM supports getSelection, but good practice to check.
return;
}
// 1. Simulate text selection
const title = wrapper.get('.td-board-card__title');
const range = document.createRange();
range.selectNodeContents(title.element);
selection.removeAllRanges();
selection.addRange(range);
expect(selection.toString()).toBe(wrapper.props().card.title);
// 2. Trigger mousedown on the drag handle
await wrapper.get('[data-action=\"drag-card-handle\"]').trigger('mousedown');
// 3. Assert that the selection was cleared
expect(selection.toString()).toBe('');
});This would ensure the fix remains effective as the codebase evolves.
| user-select: none; | ||
| -webkit-user-select: none; |
There was a problem hiding this comment.
For improved cross-browser compatibility, it's best practice to include other vendor prefixes for user-select and list the standard property last. This ensures that browsers use the standard implementation if available, falling back to the prefixed version otherwise.
-webkit-user-select: none;
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
#515) Covers handleDragHandleMouseDown — verifies removeAllRanges is called on mousedown and that null getSelection does not throw.
Adversarial review findingsReviewed against the full checklist. Six items are clean; one genuine gap was found and fixed. Items that pass
Genuine issue found and fixed
Fix applied in commit
All 5 tests (3 pre-existing + 2 new) pass. |
Summary
handleDragHandleMouseDownthat callswindow.getSelection()?.removeAllRanges()on the drag handle'smousedowneventuser-select: none/-webkit-user-select: noneCSS to.td-card-drag-handleto prevent the handle itself from being selectedRoot cause
When a user selects text inside a card, the browser prioritises a text-drag over the card's HTML drag. Because the drag handle never cleared the selection, the card became effectively non-draggable until the user manually deselected the text.
Fix
@mousedownon the drag handle callswindow.getSelection()?.removeAllRanges()immediately, before the browser decides whether to start a text-drag or an element-drag.user-select: noneon the handle element prevents the handle icon itself from being selected.Test plan
npm run typecheckpasses (verified: clean exit)