fix: allow drag to start from child button and link elements

[mattgperry]

Child buttons and links no longer block drag gestures from starting
since they don't have click-and-move actions of their own. Only form
controls where text selection or direct interaction is expected
(input, textarea, select) will still block drag.

https://claude.ai/code/session_01YKHLcB3ZviYXCsrWZzmyou

[claude]

Code Review

Summary

This PR fixes an issue where drag gestures couldn't start from child button and link elements. The change refines the logic for determining which elements should block drag gestures, limiting it to form controls with text selection/interaction (input, textarea, select) and excluding buttons and links.

---

✅ Strengths

1. Well-reasoned behavior change
The distinction between interactive elements with click-and-move actions (form controls) vs. click-only elements (buttons/links) is sound. This allows dragging card-like components with embedded buttons/links, which is a common UI pattern.

2. Excellent test coverage
The test changes demonstrate thorough coverage:

• Tests for motion.input and motion.a as draggable elements themselves ✅
• Tests for child input, textarea, select blocking drag ✅
• Tests for child button and a allowing drag ✅

3. Clear documentation
The updated JSDoc comment in is-keyboard-accessible.ts:3-13 clearly explains the rationale for excluding buttons and links.

4. Follows project conventions

• Uses interface for types (per CLAUDE.md)
• Includes comprehensive Jest tests
• Proper use of nextFrame() for async test timing

---

🔍 Observations & Considerations

1. Potential edge case: contentEditable
The function still checks for isContentEditable === true (line 17). This is correct, but worth noting that a contentEditable button or link would still block drag. This is likely the desired behavior, but consider if this edge case is documented/tested.

2. Accessibility implications
Buttons and links are keyboard-accessible elements, so allowing drag from them means:

• Users might accidentally start dragging when trying to activate the button/link
• Consider if this changes the expected keyboard interaction model

However, the PR description suggests this is intentional since buttons/links "don't have click-and-move actions of their own."

3. Impact on press gesture
The function is also used in press/index.ts:112 to determine if an element should get tabIndex=0. With this change:

• Buttons and links with press gestures will now get tabIndex=0 added (if not already present)
• This is likely fine since buttons already have tabIndex, but worth verifying behavior doesn't change unexpectedly

4. Test organization
The tests were reorganized significantly. The previous "drag gesture does not start when clicking a child button" test now expects the opposite behavior. Clear commit messages would help reviewers understand this intentional inversion.

---

🐛 Potential Issues

Minor: Test naming consistency
The test at line 1070 says "drag gesture does not start when clicking a child input" but the element is an <input> without a type. Consider if you want to test specific input types (text, range, etc.) since some might have different interaction models.

---

🎯 Recommendations

1. Consider E2E tests (Optional)
Per CLAUDE.md guidance: "For UI interaction bugs (like gesture handling), prefer E2E tests using Playwright or Cypress."

While the Jest tests are thorough, a Playwright test would verify real browser behavior, especially for edge cases like:

• Dragging from a link prevents the default navigation
• Form controls properly capture focus/selection

2. Verify all input types (Optional)
Input elements have many types (text, number, range, checkbox, etc.). Some like checkboxes might not need text selection. Consider if the behavior should vary by input type.

3. Document breaking change
This changes existing behavior where child buttons previously blocked drag. If any users rely on this, it's a breaking change. Consider:

• Adding to changelog/release notes
• Providing migration guidance if needed

---

🏁 Verdict

Approve with minor suggestions. The code quality is high, test coverage is comprehensive, and the behavioral change is well-reasoned for common UI patterns. The concerns raised are edge cases that may not warrant blocking this PR, but are worth considering for future refinement.

Security: ✅ No concerns
Performance: ✅ No concerns (same lookup complexity)
Code Quality: ✅ Clean, well-documented
Test Coverage: ✅ Comprehensive
Breaking Change: ⚠️ Yes, but likely desired

Great work! 🎉

[claude]

Code Review: Allow drag to start from child button and link elements

Summary

This PR changes the drag behavior to allow dragging when initiated from child <button> and <a> elements, while still blocking drag from form controls (<input>, <textarea>, <select>) and contentEditable elements. This is a sensible UX improvement since buttons and links don't have click-and-move interactions of their own.

---

✅ Strengths

1. Clear Rationale: The change is well-motivated - buttons and links only respond to clicks, not drag operations, so blocking drag gestures from them was unnecessarily restrictive.

2. Comprehensive Test Coverage: Excellent test additions covering all the different scenarios:

   • Tests for form controls that should block drag (input, textarea, select)
   • Tests for buttons and links that should allow drag
   • Tests for direct drag on motion.input and motion.a elements
   • Both Jest unit tests and Cypress E2E tests updated

3. Documentation: Good inline comments explaining the reasoning in both the implementation and test files.

4. Minimal Surface Area: The change is surgical - only modifying the isElementKeyboardAccessible function and its related tests.

5. Preserves contentEditable Handling: Correctly maintains blocking for contentEditable elements where text selection is important.

---

🔍 Potential Issues & Considerations

1. Edge Case: Elements with role="button" or role="link"

Severity: Medium

The current implementation only checks tag names (<button>, <a>), but doesn't consider ARIA roles. Elements with role="button" or role="link" would still block drag even though they have the same semantic meaning as native buttons/links.

Example scenario:

<motion.div drag>
  <div role="button" onClick={handleClick}>
    Click me
  </div>
</motion.div>

This would still block drag, which might be unexpected.

Suggestion:
Consider checking for ARIA roles as well:

export function isElementKeyboardAccessible(element: Element) {
    const role = element.getAttribute('role')
    
    // Allow drag from buttons and links (including ARIA roles)
    if (role === 'button' || role === 'link') {
        return false
    }
    
    return (
        interactiveElements.has(element.tagName) ||
        (element as HTMLElement).isContentEditable === true
    )
}

However, this might be intentional - perhaps you want to keep it conservative for now. Worth considering for a follow-up if users report issues.

---

2. Edge Case: Links Without href Attributes

Severity: Low

<a> tags without href attributes are not truly keyboard-accessible and behave differently than proper links. The current implementation treats them the same as regular links. This is probably fine since:

• They're uncommon in well-structured HTML
• The worst case is allowing drag when it might not be expected, which is less problematic than blocking it

---

3. Missing Test Coverage: Nested Contenteditable

Severity: Low

There's a test for contentEditable allowing drag when it's the drag target itself, but no test for contentEditable as a child element blocking parent drag. Consider adding:

test("drag gesture does not start when clicking a child contenteditable element", async () => {
    const onDragStart = jest.fn()
    const x = motionValue(0)
    const Component = () => (
        <MockDrag>
            <motion.div
                data-testid="draggable"
                drag
                onDragStart={onDragStart}
                style={{ x }}
            >
                <div data-testid="child-contenteditable" contentEditable>
                    Editable text
                </div>
            </motion.div>
        </MockDrag>
    )
    // ... rest of test
})

---

4. Function Name Might Be Misleading

Severity: Low

The function name isElementKeyboardAccessible no longer accurately describes what it does. It now returns true for elements where drag should be blocked (form controls + contentEditable), not all keyboard-accessible elements (since buttons and links are also keyboard-accessible).

Suggestion: Consider renaming to something more descriptive:

• shouldBlockDragGesture
• isTextInteractiveElement
• hasTextSelectionInteraction
• preventsDragStart

This would make the code more self-documenting:

const shouldBlockDrag = 
    target !== element && shouldBlockDragGesture(target)

---

5. Accessibility: Tab Index Behavior

Severity: Low - Observation

In packages/motion-dom/src/gestures/press/index.ts:112, the code adds tabIndex=0 to elements that aren't keyboard accessible and don't have a tabindex. With buttons and links removed from the check, this behavior is now correct - those elements are naturally keyboard accessible and don't need the automatic tabIndex.

This is working as intended, just noting it for completeness.

---

🎯 Best Practices

✅ Follows repository conventions (named exports, interfaces for types)
✅ Tests written before implementation (as per CLAUDE.md)
✅ E2E tests updated to match new behavior
✅ Clear documentation in comments

---

🔒 Security

No security concerns. This change only affects gesture handling logic and doesn't introduce any XSS, injection, or other vulnerabilities.

---

⚡ Performance

No performance concerns. The changes actually slightly improve performance by:

• Reducing the size of the interactiveElements Set from 5 to 3 elements
• Removing two unnecessary checks for "BUTTON" and "A"

---

📋 Recommendations

1. Consider ARIA roles (role="button", role="link") as mentioned in issue Initial API (WIP) #1
2. Rename function to better reflect its purpose (issue adding circleci config #4)
3. Add contentEditable child test for completeness (issue Add motionValue prop #3)

These are all minor polish items. The core implementation is solid and the test coverage is excellent.

---

✨ Final Verdict

LGTM with minor suggestions!

This is a well-thought-out change that improves UX without introducing bugs. The test coverage is thorough and the implementation is clean. The suggestions above are enhancements for future consideration, not blockers.

Great work on the comprehensive test suite - testing all the edge cases shows strong attention to detail. 🎉

---

Review generated via claude.ai/code

[claude]

Code Review

Summary

This PR improves the drag gesture behavior by allowing drag to start from child button and link elements, while still blocking drag from form controls (input, textarea, select, contenteditable). This is a sensible UX improvement since buttons and links don't have click-and-move interactions.

---

✅ Strengths

1. Well-justified behavioral change
The rationale is sound: buttons and links only respond to click events, so there's no conflict with dragging. Form controls need text selection and direct interaction, so blocking drag on those makes sense.

2. Excellent test coverage

• Updated Cypress E2E tests to reflect new behavior (lines 67-112 in drag-input-propagation.ts)
• Added comprehensive Jest unit tests covering all scenarios:
  • ✅ Drag from child button (should work)
  • ✅ Drag from child link (should work)
  • ✅ Drag from child input (should block)
  • ✅ Drag from child textarea (should block)
  • ✅ Drag from child select (should block)
  • ✅ motion.input with drag prop (should work)
  • ✅ motion.a with drag prop (should work)

3. Clear separation of concerns
The refactoring creates two distinct functions with well-defined purposes:

• isElementKeyboardAccessible(): For determining if tabIndex is needed (press gestures)
• isElementTextInput(): For determining if drag should be blocked (drag gestures)

4. Good documentation
Comments in both the utility functions and the drag controls clearly explain the reasoning behind the behavior.

---

🔍 Potential Issues

1. Missing contenteditable test coverage
While isElementTextInput() handles contenteditable elements, there are no tests for this case. Consider adding:

test("drag gesture does not start when clicking a child contenteditable", async () => {
  // ... test implementation
  <div data-testid="child-editable" contentEditable="true">
    Edit me
  </div>
})

2. Event propagation considerations
The implementation checks target !== element at packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:666. This works for direct children, but what about deeply nested elements? For example:

<motion.div drag>
  <div>
    <input /> {/* Will this be blocked? */}
  </div>
</motion.div>

The current implementation should handle this correctly since event.target will be the input regardless of nesting depth, but it might be worth adding a test for deeply nested elements to document this behavior.

3. Potential edge case: button/link inside input label
Consider this edge case:

<motion.div drag>
  <label>
    <input type="checkbox" />
    <button>Click me</button>
  </label>
</motion.div>

If users click the button, should drag start? The current implementation will allow it, which seems reasonable, but worth considering if this could lead to unexpected behavior.

---

🎯 Recommendations

Low priority:

1. Add contenteditable test case for completeness
2. Consider adding a test for deeply nested form controls to document expected behavior
3. Consider adding a comment in the code explaining that the check works for nested elements, not just direct children

Documentation:
The E2E test comments are excellent. The implementation comments clearly explain the rationale. Consider updating any user-facing documentation if this is a breaking change for existing apps.

---

🔒 Security & Performance

• ✅ No security concerns identified
• ✅ No performance regressions - just changing the blocking condition
• ✅ No XSS or injection vulnerabilities introduced

---

📊 Overall Assessment

This is a high-quality PR with:

• Clear motivation and sensible design decisions
• Comprehensive test coverage (both E2E and unit)
• Clean implementation with good separation of concerns
• Well-documented code

The suggestions above are minor improvements. The core change is solid and ready to merge pending any final discussion about the contenteditable test coverage.

Recommendation: Approve with minor suggestions ✅

---

Note: Tests were reviewed statically. Consider running the full test suite to verify all tests pass.