-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix drag gesture triggering on interactive elements inside draggables #3448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Prevents drag from starting when clicking on input, textarea, select, button, anchor, or contenteditable elements inside a draggable component. This allows normal interaction with form elements (focus, text selection) without triggering the parent's drag gesture. Fixes #1674 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code ReviewThank you for this PR! This is a well-thought-out fix for issue #1674. Here's my detailed review: ✅ Strengths
🔍 Potential Issues & Suggestions1. Event Target vs Current Target (Medium Priority)The code checks \!isElementKeyboardAccessible(event.target as Element)However, this may not handle all edge cases. Consider:
Recommendation: Consider traversing up the DOM tree to check if any ancestor (up to the draggable element) is keyboard accessible: function hasKeyboardAccessibleAncestor(
target: Element,
stopAt: Element
): boolean {
let current: Element | null = target
while (current && current \!== stopAt) {
if (isElementKeyboardAccessible(current)) {
return true
}
current = current.parentElement
}
return false
}Then use: Test Case: Add a test with an input that has child elements (e.g., 2. Select Element Menu (Low Priority)The fix prevents drag on 3. Custom Interactive Elements (Low Priority)Elements with 4. Type Safety (Minor)The cast const target = event.target
if (\!(target instanceof Element)) return
if (drag && dragListener && \!isElementKeyboardAccessible(target)) {
this.start(event)
}📝 Code Quality
🧪 Test SuggestionsConsider adding these edge case tests:
🎯 Performance & Security
📖 DocumentationConsider adding a JSDoc comment to the SummaryThis is a solid PR that solves a real usability issue. The main concern is the potential edge case with nested elements. I'd recommend:
Great work following the test-first approach from CLAUDE.md! 🎉 |
Consolidates interactive element detection logic between drag and press gestures. Adds isContentEditable check to the shared utility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fec30d8 to
bdd34d8
Compare
Code Review SummaryI've reviewed PR #3448 and overall this is a well-implemented fix for issue #1674. The solution is elegant, focused, and follows the repository's best practices. Great work! ✅ Strengths
🔍 Observations & Suggestions1. Consider
|
Tests verify that drag gestures do not trigger when interacting with: - Button - Link (anchor) - Select dropdown - Checkbox inside label - ContentEditable element 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request ReviewSummaryThis PR fixes issue #1674 by preventing drag gestures from starting when users interact with interactive elements (inputs, textareas, buttons, links, selects, contenteditable) inside draggable components. The solution is elegant and well-tested. ✅ Strengths1. Excellent Test Coverage
2. Clean Implementation
3. Good Developer Documentation
🔍 Code Quality ObservationsMinor Issues1. Export Addition May Be Unnecessary// packages/motion-dom/src/index.ts
+export * from "./gestures/press/utils/is-keyboard-accessible"Issue: This exports Recommendation: Consider whether this utility should be part of the public API. If it's only for internal use across packages, you might want to:
2. Missing Edge Case: Nested Interactive ElementsThe current implementation checks only <motion.div drag>
<label>
<span>Click me</span>
<input />
</label>
</motion.div>If a user clicks on the Test: The test file includes a checkbox inside a label, which is good! However, the test clicks directly on the checkbox ( Recommendation: Add a test case that clicks on the label text itself (not the input/checkbox) to verify this edge case is handled. You may need to check 3. Test Robustness: Hard-coded Coordinatescy.get("[data-testid='draggable']")
.trigger("pointerdown", 5, 5) // Hard-coded coordinatesIssue: The tests use hard-coded coordinates (5, 5) which assume the element layout. If the test component layout changes, tests could break or pass incorrectly. Recommendation: While this matches existing test patterns in the codebase (see 🎯 Functionality ReviewCorrect Behavior ✅The implementation correctly:
Potential Concerns1. Custom Interactive ComponentsElements with Example: <motion.div drag>
<div role="button" onClick={handleClick}>Custom Button</div>
</motion.div>Impact: Low - Most properly implemented custom interactive components should have Recommendation: Consider adding a comment documenting this limitation, or extend the check to include common ARIA roles. 2. Performance Consideration
Verdict: No action needed, but worth monitoring in performance-critical applications with many draggable elements. 🔒 Security ReviewNo security concerns identified. The changes are defensive in nature:
📋 Additional Recommendations1. Add JSDoc Comments/**
* Prevents drag from initiating when interacting with keyboard-accessible
* elements like inputs, buttons, and links inside a draggable container.
* This allows users to interact normally with form controls without
* accidentally triggering drag gestures.
*
* @see https://github.com/motiondivision/motion/issues/1674
*/
const stopPointerListener = addPointerEvent(
element,
"pointerdown",
(event) => {
// ...
}
)2. Consider Adding a Prop for Opt-OutWhile the current behavior is sensible as the default, some users might want the old behavior. Consider adding a prop like 3. Test with Real User Interaction (Manual Testing)Ensure manual testing covers:
The PR description mentions manual testing, which is great! ✅ 🏁 ConclusionThis is a high-quality PR that solves a real UX problem with a clean, minimal implementation. The test coverage is excellent and the code follows the repository's conventions. Approval Recommendation: ✅ Approve with Minor SuggestionsThe suggestions above are minor enhancements that could be addressed in follow-up PRs if needed. The current implementation is production-ready. Priority Suggestions:
Great work on fixing this long-standing issue! 🎉 |
PR Review: Fix drag gesture triggering on interactive elementsSummaryThis PR addresses issue #1674 by preventing drag gestures from starting when users interact with focusable/interactive elements (inputs, textareas, buttons, links, etc.) inside draggable components. The fix is elegant and well-tested. ✅ Strengths
🔍 Code Quality Review
|
Pull Request ReviewThis PR effectively fixes issue #1674 by preventing drag gestures from starting when users interact with focusable/interactive elements inside draggable containers. The implementation is clean and well-tested. ✅ Strengths1. Comprehensive Test CoverageThe E2E tests cover all major interactive elements:
2. Clean Implementation
3. Good Documentation
🐛 Potential Issues1. Missing contenteditable Check (CRITICAL)The // packages/motion-dom/src/gestures/press/utils/is-keyboard-accessible.ts:9
export function isElementKeyboardAccessible(element: Element) {
return (
focusableElements.has(element.tagName) ||
(element as HTMLElement).tabIndex \!== -1
)
}While the test includes a contenteditable element ( Recommendation: Add explicit contenteditable check: export function isElementKeyboardAccessible(element: Element) {
const htmlElement = element as HTMLElement
return (
focusableElements.has(element.tagName) ||
htmlElement.tabIndex \!== -1 ||
htmlElement.isContentEditable
)
}2. Edge Case: Labels Without Interactive ChildrenThe implementation blocks drag on 3. Disabled ElementsA disabled button/input should arguably allow drag-through since it's not interactive. Consider: const isDisabled = (element as HTMLButtonElement | HTMLInputElement).disabled
if (\!isDisabled && isElementKeyboardAccessible(event.target as Element))Though this might be over-engineering - current behavior is reasonable. 📊 Code QualityPositive
Minor Suggestions
🧪 TestingCoverage: Excellent
Suggestions
🎯 RecommendationApprove with minor fix required: Fix the contenteditable detection issue before merging. The current implementation may work in some browsers but is not guaranteed to be reliable. The overall approach is sound and this is a valuable fix for a long-standing issue. Great work on the comprehensive test coverage! Review generated using repository conventions from CLAUDE.md |
…with onTap The previous tabIndex !== -1 check was too broad - it blocked drag for any element with tabIndex, including motion.div elements that automatically get tabIndex=0 when onTap is set. Now only blocks drag for actual interactive form elements (INPUT, TEXTAREA, SELECT, BUTTON, A) and contentEditable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review - PR #3448This PR fixes a long-standing issue (#1674) where interactive elements inside draggable components would trigger drag gestures, preventing normal interaction. Overall, this is a well-implemented fix with excellent test coverage. Here's my detailed review: ✅ Strengths1. Test-Driven Development
2. Minimal, Focused Implementation
3. Proper Scope
🔍 Code Quality Observations1. Function Name vs Implementation Mismatch The function // packages/motion-dom/src/gestures/press/utils/is-keyboard-accessible.ts:17-22
export function isElementKeyboardAccessible(element: Element) {
return (
interactiveElements.has(element.tagName) ||
(element as HTMLElement).isContentEditable === true
)
}Suggestion: Consider renaming to 2. Missing Edge Cases The implementation doesn't handle:
3. Type Safety packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:650: !isElementKeyboardAccessible(event.target as Element)The !isElementKeyboardAccessible(event.target as Element | null ?? event.currentTarget as Element)🎯 Test CoverageExcellent coverage for the happy path, but consider adding:
|
Summary
Fixes #1674
Test plan
🤖 Generated with Claude Code