Allow drag within rotated parents

[mattgperry]

When a draggable element is inside a rotated parent container, the drag offset needs to be transformed from screen space to the element's local coordinate space.

This fix:

• Adds applyParentRotation() method that computes cumulative rotation from all ancestor motion elements in the projection path
• Applies inverse 2D rotation to both drag offset and velocity
• Uses minimal bundle impact: only Math.cos/sin, no new dependencies
• Leverages existing projection node data (path, latestValues)

Fixes #3320

[claude]

Pull Request Review: Allow drag within rotated parents

Great fix for issue #3320! The implementation correctly addresses the problem of drag direction being inverted when a parent container is rotated. Here's my detailed review:

✅ Strengths

1. Well-designed solution: The applyParentRotation() method correctly applies inverse 2D rotation transformations using standard rotation matrix math, transforming drag offsets from screen space to local coordinate space.

2. Comprehensive test coverage: The Cypress E2E tests cover the critical cases (0°, 90°, 180° rotations) and verify that dragged elements move in the expected direction in screen space.

3. Minimal bundle impact: Uses only Math.cos() and Math.sin() with existing projection node data - no new dependencies.

4. Consistent application: The transformation is applied to both drag offset (in onMove) and velocity (in stop), ensuring correct behavior during drag and momentum animations.

5. Follows code style: Uses interfaces, arrow functions, strict equality, and proper TypeScript typing per CLAUDE.md guidelines.

🔍 Potential Issues & Improvements

1. Missing rotateZ support (Medium priority)

The code only checks for latestValues.rotate but CSS also supports rotateZ for 2D rotation:

// Current (VisualElementDragControls.ts:356-358)
if (latestValues.rotate && typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}

Recommendation: Also check latestValues.rotateZ:

if (latestValues.rotate && typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}
if (latestValues.rotateZ && typeof latestValues.rotateZ === "number") {
    totalRotation += latestValues.rotateZ
}

2. 3D rotation edge case (Low priority)

The comment mentions "2D rotation" but doesn't handle rotateX or rotateY which create 3D perspective effects. When 3D rotations are present, the simple 2D transformation won't be mathematically correct.

While this is likely an acceptable limitation for v1, consider:

• Adding a comment explaining this limitation
• Detecting 3D rotations and logging a dev warning (non-prod builds only)
• Future enhancement: handle 3D rotations using full matrix transformations

3. Test coverage gaps (Low priority)

Consider adding tests for:

• Negative rotation angles (e.g., -90°, -180°)
• Rotation values > 360° or compound rotations
• rotateZ property (if you add support per Initial API (WIP) #1)
• Nested rotated parents (multiple ancestors with rotation)
• dragDirectionLock with rotated parents
• Drag constraints with rotated parents

4. Type safety consideration (Very low priority)

The latestValues object is not strongly typed in this context. While unlikely, if someone passes a non-numeric value to rotate, it would silently fail the type check. Consider adding runtime validation or using TypeScript type guards more explicitly.

5. Performance consideration (Very low priority)

The applyParentRotation() method iterates through the entire projection path on every drag move. For deeply nested elements, this could be optimized by:

• Caching the cumulative rotation and only recalculating when the projection path changes
• However, this is likely premature optimization - only consider if profiling shows issues

🧪 Testing Recommendations

Before merging, ensure:

1. Run the full test suite: yarn test and yarn test-playwright
2. Manually test the dev app: cd dev/react && yarn start, then navigate to ?test=drag-rotated-parent&rotate=180
3. Test with nested rotated parents (multiple ancestors with different rotation values)
4. Test with dragConstraints enabled to ensure constraints work correctly with rotated parents

📝 Minor Nits

1. Comment clarity (VisualElementDragControls.ts:355): Consider clarifying "2D rotation" since rotateZ is also 2D:

   // Accumulate z-axis rotation (rotate or rotateZ) - 2D plane rotation

2. Math explanation (VisualElementDragControls.ts:367-370): The rotation matrix comment is excellent but could mention that this is the standard 2D rotation matrix formula for reference.

🎯 Verdict

This is a solid fix that addresses the reported issue effectively. The main concern is the missing rotateZ support (item #1), which should be addressed before merging since rotateZ is valid CSS and users may expect it to work.

After addressing the rotateZ support, this PR is ready to merge. The 3D rotation limitation is acceptable for the initial fix and can be enhanced later if needed.

Recommendation: Request changes for rotateZ support, then approve.

[claude]

Pull Request Review: Allow drag within rotated parents

Summary

This PR fixes a bug where draggable elements inside rotated parent containers would move in the wrong direction. The fix applies an inverse rotation transformation to both drag offsets and velocity to correctly map screen-space coordinates to the element's local coordinate space.

Code Quality & Best Practices ✅

Strengths:

• Clean, well-documented implementation with clear comments explaining the mathematical transformation
• Minimal bundle impact - uses only Math.cos and Math.sin, no new dependencies
• Follows the repository's code style (interface usage, named exports, arrow functions)
• Good method naming (applyParentRotation) that clearly describes intent
• Comprehensive test coverage with both basic drag tests and constraint tests

Suggestions for Improvement:

1. Consider 3D rotations (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:356):
   The current implementation only handles 2D rotation (rotate property). Consider whether rotateZ should also be supported, as it's semantically equivalent to rotate in 2D contexts. Users might use rotateZ instead of rotate.

   // Current code only checks 'rotate'
   if (latestValues.rotate && typeof latestValues.rotate === "number") {
       totalRotation += latestValues.rotate
   }
   
   // Consider also checking rotateZ
   if (latestValues.rotateZ && typeof latestValues.rotateZ === "number") {
       totalRotation += latestValues.rotateZ
   }

2. Type safety for offset parameter (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:320):
   The offset parameter is optional (offset?: Point), but applyParentRotation expects a non-optional Point. While the early return in updateAxis prevents undefined from reaching applyParentRotation, adding explicit type safety would be clearer:

   private applyParentRotation(offset: Point | undefined): Point {
       if (!offset) return { x: 0, y: 0 }
       // ... rest of implementation
   }

3. Edge case: what about rotateX and rotateY? While 3D rotations (rotateX, rotateY) would require a full 3D transformation matrix to handle correctly, it might be worth documenting that only 2D rotation is supported. Add a comment in the method explaining the limitation:

   /**
    * Transform offset to account for cumulative parent rotation.
    * When a parent element is rotated, the drag offset (calculated in screen space)
    * needs to be transformed to the element's local coordinate space.
    * 
    * Note: Only handles 2D rotation (rotate/rotateZ). 3D rotations (rotateX/rotateY)
    * are not supported as they would require full 3D matrix transformations.
    */

Potential Bugs or Issues 🔍

1. Nested rotations accumulation (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:351-359): The implementation correctly accumulates rotations from all ancestors, but I'd recommend verifying this works correctly with deeply nested rotated containers. The test file only tests single-level rotation.

2. Transform origin impact: The current implementation assumes the rotation center doesn't affect the drag offset transformation. This is correct for the inverse rotation of the offset vector, but might be worth adding a test case with non-default transformOrigin values to verify behavior.

3. Direction lock interaction (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:203-212): The dragDirectionLock logic uses the untransformed offset to determine direction, which might cause issues with rotated parents. Consider whether getCurrentDirection should also receive the transformed offset when parent rotation exists.

Performance Considerations ⚡

1. Unnecessary computation check: The early return when totalRotation === 0 is good (line 362), but you could also add an early return if there's no projection system:

   if (!projection || !projection.path?.length) return offset

2. Repeated trigonometric calculations: The Math.cos and Math.sin are called on every drag move event. For static rotations, these could be cached. However, given that rotations can be animated, the current approach is correct. No optimization needed.

3. Path iteration: The loop through projection.path is efficient and shouldn't cause performance issues even with deeply nested hierarchies.

Security Concerns 🔒

No security concerns identified. The mathematical transformations are straightforward and don't involve user input processing that could lead to injection or overflow issues.

Test Coverage 🧪

Excellent test coverage! The PR includes:

• Basic drag tests with 0°, 90°, and 180° rotations
• Comprehensive constraint tests with all four directions
• Tests for rotated parents with constraints
• Both positive and negative constraint values

Suggestions to strengthen tests:

1. Add tests for:

   • Nested rotations (grandparent: 45°, parent: 45°, should cumulate to 90°)
   • Non-standard transform origins (e.g., transformOrigin: "top left")
   • Animated rotations (parent rotation changes during drag)
   • rotateZ property usage alongside rotate
   • Fractional rotation values (e.g., 33.5°)
   • Negative rotation values (e.g., -90°)

2. Test for edge case with direction lock:

   it("Direction lock works correctly with 90-degree rotated parent", () => {
     // With dragDirectionLock enabled, ensure direction detection works correctly
     // when parent is rotated
   })

3. Performance test: Consider adding a test that drags within a deeply nested structure (4-5 levels) to ensure no performance degradation.

Additional Observations

1. Documentation: Consider updating user-facing documentation to mention this fix, as it resolves a common pain point for users working with rotated layouts.

2. Changelog: Ensure this is highlighted in the changelog as a bug fix for issue [BUG] Draggable direction is incorrect when parent container is rotated #3320.

3. Bundle size: ✅ Confirmed minimal impact - only adds ~20 lines of code and no new dependencies.

Conclusion

This is a solid, well-implemented fix for a real user pain point. The mathematical approach is correct, the code is clean and efficient, and the test coverage is comprehensive. The suggestions above are minor improvements that would make the implementation even more robust, but the PR is ready to merge as-is.

Recommendation: ✅ Approve with minor suggestions for future enhancement

Great work addressing this issue!

[claude]

Pull Request Review: Allow drag within rotated parents

Overview

This PR fixes a bug where dragging elements with rotated parent containers causes inverted or incorrect drag directions. The solution applies coordinate space transformation to account for parent rotation.

Code Quality & Best Practices ✅

Strengths:

• Clean implementation: The applyParentRotation() method is well-encapsulated and focused on a single responsibility
• Good naming: Method and variable names clearly convey intent (applyParentRotation, totalRotation, transformedOffset)
• Excellent documentation: The math is clearly explained with comments showing the rotation matrix formula (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:367-370)
• Minimal bundle impact: Uses only native Math.cos/sin, no new dependencies
• Leverages existing infrastructure: Reuses projection node data (path, latestValues)
• Consistent with codebase style: Uses interfaces, named exports, arrow callbacks, strict equality

Potential Issues & Suggestions 🔍

1. Missing Support for rotateZ Transform

Location: packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:356

The code only checks for latestValues.rotate:

if (latestValues.rotate && typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}

Issue: CSS supports both rotate and rotateZ for 2D rotation. If users specify rotateZ, it won't be accounted for.

Suggestion:

// Accumulate 2D rotation (rotate or rotateZ)
const rotation = (latestValues.rotate ?? latestValues.rotateZ) as number | undefined
if (rotation && typeof rotation === "number") {
    totalRotation += rotation
}

2. Missing 3D Rotation Handling

Context: The current implementation only handles 2D rotation around the Z-axis.

Considerations:

• 3D rotations (rotateX, rotateY, perspective) require full 3D transformation matrices
• The 2D approach won't work correctly if parents have 3D rotations
• This is likely acceptable for an initial fix (3D is complex and less common)

Suggestion: Add a comment or warning in the code:

/**
 * Transform offset to account for cumulative parent rotation.
 * When a parent element is rotated, the drag offset (calculated in screen space)
 * needs to be transformed to the element's local coordinate space.
 * 
 * Note: Only handles 2D rotation (rotate/rotateZ). 3D rotations 
 * (rotateX, rotateY, perspective) are not supported.
 */

3. Performance Consideration - Path Iteration

Location: packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:352-359

The method iterates through all ancestors on every onMove and stop event (which can fire many times per second during dragging).

Current: O(n) where n = number of ancestors
Impact: Likely minimal for typical DOM depths (< 20 levels), but could be optimized

Potential Optimization: Cache the cumulative rotation and only recalculate when the projection path changes:

private cachedRotation: number | null = null
private lastProjectionPath: any[] | null = null

private applyParentRotation(offset: Point): Point {
    const { projection } = this.visualElement
    if (!projection) return offset

    // Check if we can use cached rotation
    if (this.lastProjectionPath === projection.path && this.cachedRotation !== null) {
        if (this.cachedRotation === 0) return offset
        // Use cached rotation...
    }

    // Calculate and cache...
    this.cachedRotation = totalRotation
    this.lastProjectionPath = projection.path
    // ...
}

Note: Only optimize if profiling shows this is a bottleneck. Current approach prioritizes correctness and simplicity.

Test Coverage ✅

Excellent test coverage with 10 comprehensive E2E tests covering:

• Basic drag with rotation (0°, 90°, 180°)
• Drag constraints with rotation (right, left, top, bottom)
• Combined constraints at different angles
• Animated rotating parent (continuous rotation)
• Constraint enforcement during animation

Strengths:

• Tests use actual user interactions (pointerdown, pointermove, pointerup)
• Visual verification via getBoundingClientRect()
• Tests cover both positive and negative cases
• Good test organization and naming

Potential Enhancement: Consider adding unit tests for the applyParentRotation() method itself:

describe("applyParentRotation", () => {
    it("returns original offset when no rotation", () => {
        // Test with totalRotation = 0
    })
    
    it("correctly transforms offset for 180 degree rotation", () => {
        // Expected: (100, 100) -> (-100, -100)
    })
    
    it("correctly transforms offset for 90 degree rotation", () => {
        // Expected: (100, 0) -> (0, -100)
    })
})

Performance Considerations ⚡

1. Minimal overhead: Only adds rotation calculation when projection exists
2. Early exit optimization: Returns early if totalRotation === 0 (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:362)
3. No new allocations: Returns new Point object, but this is unavoidable
4. Math operations: cos/sin are highly optimized native functions

Verdict: Performance impact should be negligible. The optimization is well-balanced.

Security Concerns 🔒

No security issues identified.

• No user input is directly used in calculations
• All rotation values come from controlled internal state (latestValues)
• No risk of XSS, injection, or other common vulnerabilities
• Math operations are safe (no division by zero, no overflow concerns with degrees → radians)

Additional Suggestions

1. Edge Case: Very Small Rotations

Consider adding a threshold to skip transformation for negligible rotations:

// If rotation is negligible (< 0.01°), skip transformation
if (Math.abs(totalRotation) < 0.01) return offset

2. Consider Transform Origin

The current implementation assumes rotation is around the center. If transformOrigin is customized, the rotation calculation might need adjustment. This is likely a separate concern, but worth noting.

3. TypeScript Strictness

The code assumes latestValues.rotate is a number when it exists. Consider adding explicit type checking:

if (typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}

(Note: This is already done in the code - good!)

Summary

Approval Recommendation: ✅ Approve with minor suggestions

This is a solid fix for a real user-facing bug. The implementation is:

• ✅ Mathematically correct
• ✅ Well-tested with comprehensive E2E coverage
• ✅ Minimal bundle impact
• ✅ Clean and maintainable code
• ✅ No security concerns
• ✅ Follows codebase conventions

Suggested improvements (non-blocking):

1. Add support for rotateZ property
2. Document 3D rotation limitations
3. Consider caching rotation calculation if profiling shows it's needed
4. Consider unit tests for the transformation logic itself

Great work on this fix! The comprehensive test coverage is particularly impressive.

[claude]

PR Review: Allow drag within rotated parents

Summary

This PR fixes a critical bug where draggable elements inside rotated parent containers would move in incorrect directions. The fix applies inverse rotation transformations to convert drag offsets from screen space to local coordinate space.

✅ Strengths

1. Excellent Test Coverage

• 366 lines of comprehensive Cypress E2E tests covering:
  • Basic rotation scenarios (0°, 90°, 180°)
  • Constraint behavior with rotated parents
  • Animated rotating parents
  • Multiple constraint combinations
• Well-documented test cases with clear comments explaining coordinate space transformations
• Tests validate both the bug fix AND that constraints still work correctly

2. Clean Implementation

• Minimal bundle impact: only uses Math.cos/sin, no new dependencies
• Self-contained applyParentRotation() method with clear documentation
• Properly handles the no-rotation case (early return)
• Applied to both offset (during drag) and velocity (on drag end)

3. Good Architecture

• Leverages existing projection system data (path, latestValues)
• Accumulates rotation through ancestor chain correctly
• Follows repository patterns and coding style

🔍 Issues & Concerns

Critical: Missing 3D Rotation Support

Severity: High - The implementation only handles 2D rotation (rotate/rotateZ) but ignores 3D rotations:

Lines 355-358:

// Accumulate 2D rotation (rotate or rotateZ)
if (latestValues.rotate && typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}

Problems:

1. rotateX and rotateY are completely ignored - These would cause incorrect drag behavior
2. 3D rotations require matrix transformations, not simple 2D rotation
3. No validation or warning when 3D rotations are present

Example failure case:

<motion.div style={{ rotateX: 45 }}>  {/* This rotation is ignored! */}
  <motion.div drag />
</motion.div>

Recommendation:

• Add explicit check for 3D rotation properties (rotateX, rotateY, rotateZ)
• Either:
  • Support full 3D transformations (complex, requires matrix math), OR
  • Log a development warning when unsupported 3D rotations are detected, OR
  • Document the limitation in code comments and potentially in docs

Minor: Code Quality Issues

1. Loop iteration style inconsistency (line 353):

for (let i = 0; i < path.length; i++) {
    const { latestValues } = path[i]

Repository prefers functional iteration. Consider:

path.forEach(({ latestValues }) => {
    // ...
})

2. Magic number (line 365):

const radians = (totalRotation * Math.PI) / 180

Consider extracting as a constant or utility function since it appears twice:

const DEG_TO_RAD = Math.PI / 180
const radians = totalRotation * DEG_TO_RAD

3. Type safety - The latestValues.rotate property should be explicitly typed as number | string | undefined to handle percentage/string values that motion supports.

Minor: Test Completeness

Missing test cases:

1. Nested rotations - Multiple rotated ancestors with different angles
2. Negative rotation angles - e.g., rotate: -45
3. Rotation > 360° - Does the math still work correctly?
4. Scale + rotation combinations - How do these interact?
5. rotateZ vs rotate - Are both handled identically? (Should be, but worth testing)

Documentation

Missing inline documentation for:

• The mathematical transformation formula (lines 367-370) - While the comments explain it's an inverse rotation, a brief note about the 2D rotation matrix would help
• Why we apply the transformation to both offset AND velocity
• Known limitations (3D rotation support)

🎯 Recommendations

Must Fix (Blocking):

1. Address 3D rotation handling - At minimum, add detection and warning/error for unsupported cases

Should Fix (Non-blocking):

2. Add test for nested rotations (multiple rotated ancestors)
3. Improve code documentation around the math and limitations
4. Consider edge cases: negative angles, angles > 360°

Nice to Have:

5. Extract degree-to-radian conversion to shared utility
6. Use functional iteration style for consistency
7. Add explicit type checking for latestValues.rotate

🔒 Security

No security concerns identified.

⚡ Performance

Excellent - minimal performance impact:

• Only calculates rotation when projection exists
• Early return for zero rotation
• Simple trigonometric operations (cos/sin)
• No heap allocations in hot path

📊 Overall Assessment

Score: 7.5/10

This is a well-crafted fix for an important bug with excellent test coverage. The implementation is clean and efficient. However, the lack of 3D rotation handling is a significant gap that could lead to subtle bugs and user confusion.

Once the 3D rotation concern is addressed (even just with a warning), this would be ready to merge.

---

Great work on the comprehensive tests and clean implementation! 🎉

[claude]

Pull Request Review

I've reviewed PR #3486 which fixes dragging behavior when the parent element is rotated. This is a well-implemented solution to issue #3320. Here's my detailed analysis:

✅ Strengths

1. Excellent Test Coverage

• Comprehensive E2E tests covering multiple rotation angles (0°, 90°, 180°)
• Tests for drag constraints in rotated coordinate spaces with detailed comments explaining the local vs screen coordinate transformation
• Tests for animated rotation scenarios (continuously rotating parent)
• The constraint tests particularly well-documented with clear explanations of why screen directions are inverted with 180° rotation

2. Clean Mathematical Implementation

• Correct 2D rotation matrix math for transforming screen space to local space
• Proper inverse rotation using cos(θ) * x + sin(θ) * y for x and -sin(θ) * x + cos(θ) * y for y
• Handles cumulative rotation from all ancestors in the projection path
• Minimal bundle impact using only Math.cos/sin

3. Performance Optimization

• Smart early return when totalRotation === 0 avoids unnecessary calculations
• Stores screen-space offset to avoid recalculating it each frame
• Only runs frame update loop during active dragging

4. Good Architecture

• Separates concerns: applyParentRotation() handles transformation, updateDragPosition() handles position updates
• Properly cleans up frame loop on drag end or cancel
• Leverages existing projection system data (path, latestValues)

🔍 Potential Issues & Suggestions

1. Frame Loop Memory Leak Risk ⚠️

Issue: In startFrameUpdate(), the recursive call pattern could potentially leak if the drag state gets out of sync:

private startFrameUpdate() {
    const updateFrame = () => {
        if (!this.isDragging) return  // Early return but frame is already scheduled
        this.updateDragPosition()
        frame.update(updateFrame)  // Schedules next frame
    }
    this.frameUpdateCallback = updateFrame
    frame.update(updateFrame)  // Initial schedule
}

Problem: If isDragging becomes false between when frame.update(updateFrame) is called and when updateFrame executes, the callback is already scheduled for next frame. While the early return prevents further scheduling, the pattern could be more robust.

Recommendation: Cancel the frame before checking, similar to how PanSession does it:

private startFrameUpdate() {
    const updateFrame = () => {
        if (!this.isDragging) {
            cancelFrame(this.frameUpdateCallback!)
            return
        }
        this.updateDragPosition()
        this.frameUpdateCallback = frame.update(updateFrame)
    }
    this.frameUpdateCallback = frame.update(updateFrame)
}

2. Missing rotateZ Support

Issue: The code only checks for latestValues.rotate:

if (latestValues.rotate && typeof latestValues.rotate === "number") {
    totalRotation += latestValues.rotate
}

Problem: 3D transforms also support rotateZ which should have the same effect as rotate for 2D dragging.

Recommendation: Also check for rotateZ:

const rotate = latestValues.rotate ?? latestValues.rotateZ
if (rotate && typeof rotate === "number") {
    totalRotation += rotate
}

3. No Support for rotateX/rotateY 📝

The current implementation only handles 2D rotation (rotate/rotateZ). If a parent has rotateX or rotateY, the drag offset will be incorrect.

Analysis: This is a limitation but reasonable for this PR since:

• 3D rotation transforms require full 3x3 or 4x4 matrix operations (much more complex)
• The issue [BUG] Draggable direction is incorrect when parent container is rotated #3320 specifically shows 2D rotation scenarios
• 3D rotations are less common for drag interactions

Recommendation: Add a comment documenting this limitation for future work, or consider logging a warning when 3D rotations are detected.

4. Test Tolerance Values Could Be Tighter

In constraint tests, there are ranges like:

expect(finalRect.left).to.be.greaterThan(initialLeft - 60)
expect(finalRect.left).to.be.lessThan(initialLeft - 40)

This ±10px tolerance around the expected 50px constraint seems loose. Consider if tighter bounds like ±5px would still pass reliably.

5. Edge Case: Zero-Size Parent

If a parent element has zero width/height, the transform origin might behave unexpectedly. Consider whether validation or safeguards are needed.

🎯 Code Style & Best Practices

✅ Follows Codebase Conventions

• Uses interface for type definitions ✓
• Named exports ✓
• Arrow callbacks ✓
• Proper JSDoc comments ✓
• Adheres to the project's test writing practices ✓

✅ Security

• No security concerns identified
• No user input directly used in calculations
• No dynamic code execution

📊 Performance Considerations

Positive

• Early returns avoid unnecessary work
• Minimal per-frame overhead when not dragging
• Reuses existing projection system data

Minor Concern

The frame loop runs on every frame during drag, even for static (non-animating) parents. This is necessary for correctness but does add overhead.

Potential Optimization: Consider only starting the frame loop if:

1. A parent is currently animating, OR
2. After the first move event, check if parent rotation changed since drag start

However, this optimization may add complexity that isn't worth it for the typical use case.

🧪 Testing Quality

Excellent test coverage with:

• Basic drag functionality tests
• Constraint tests at multiple angles
• Animated rotation tests
• Good use of waiting and timing in async tests

Suggestion: Consider adding:

• Test for nested rotated parents (e.g., 45° parent inside 45° grandparent)
• Test for rotateZ property (once supported in code)
• Test for performance (no janky frame drops during drag)

📝 Documentation

The code has good inline comments explaining the rotation math. Consider adding:

• A brief explanation in the PR description about when/why the frame loop is needed
• Documentation about the 2D-only limitation

Summary

This is a high-quality PR that solves a real user problem with a mathematically sound approach and excellent test coverage. The main concern is the potential frame loop cleanup edge case, which should be addressed before merging. The missing rotateZ support is a minor gap that should be fixed. The 3D rotation limitation is acceptable but should be documented.

Recommendation: Request changes for issues #1 and #2, then approve.

Suggested Changes

1. Improve frame loop cleanup pattern (high priority)
2. Add rotateZ support (medium priority)
3. Document 3D rotation limitation (low priority)
4. Consider tighter test tolerances (low priority)

Great work overall! 🎉

[claude]

Pull Request Review: Allow drag within rotated parents

Summary

This PR addresses issue #3320 by fixing drag behavior when a draggable element is inside a rotated parent container. The implementation transforms drag offsets from screen space to local coordinate space using inverse rotation matrices.

---

✅ Strengths

Implementation Quality

• Clean coordinate transformation: The inverse rotation math is correct and well-documented (lines 423-433 in VisualElementDragControls.ts)
• Delta-based approach: Smart evolution from cumulative offset to delta-based updates prevents drift during animated rotations
• Minimal bundle impact: Uses only Math.cos/sin, no external dependencies
• Leverages existing data: Efficiently reuses projection node paths and latestValues

Test Coverage

• Comprehensive E2E tests: 366 lines of Cypress tests covering static rotations (0°, 90°, 180°), animated rotations, and all constraint directions
• Well-documented: Excellent comments explaining coordinate space inversions (lines 86-93 in drag-rotated-parent.ts)
• Interactive dev test: Includes parameterized test page for manual verification

---

🔍 Code Quality Observations

Positive

1. Clear separation of concerns: applyDelta() handles position updates, applyParentRotation() handles transforms
2. Consistent with repo patterns: Uses eachAxis(), motion values, and existing constraint application
3. Proper initialization: Resets tracking state in onStart handler (lines 197-199)

Areas for Consideration

1. Unused Field (Minor)

The lastParentRotation field is initialized (line 82, 199) but never read. Suggestion: Remove if not needed, or add comment explaining its future use.

2. Method Visibility (Architectural)

updateAxis() method (lines 343-362) is now unused but remains private. Since the new applyDelta() method replaces it, consider removing it or documenting the relationship.

3. Edge Case: No Projection Node

getParentRotation() returns 0 if no projection node exists (line 396). This is safe but worth verifying against real scenarios.

---

🐛 Potential Issues

Critical: Transform Origin Assumption

The rotation transformation assumes transformOrigin is at center (which is set in the test at line 32). However, rotation with non-center origins requires translation compensation. If a parent has transformOrigin: "top left" or any non-center origin, the transformation will be incorrect.

Recommendation: Document this limitation if acceptable for v1, OR extend getParentRotation() to return both rotation and origin, then adjust the transform accordingly.

Minor: Nested Rotations with Different Origins

When multiple ancestors have different transform origins AND rotations, the cumulative transformation becomes more complex. The current implementation sums rotations (line 403) which works for same-origin ancestors but may produce inaccuracies with varied origins.

---

⚡ Performance Considerations

Positive

• Early return optimization for zero rotation (line 418)
• Reuses existing projection path traversal
• Math operations are fast (cos/sin called twice per drag move)

Concern: Per-Frame Rotation Calculation

getParentRotation() traverses the full projection path on every pointermove event. For deeply nested hierarchies (10+ ancestors), this could add measurable overhead.

Potential optimization: Cache parent rotation during drag session, invalidate only on layout changes.

---

🧪 Test Coverage

Excellent Coverage

• Static rotations at 0°, 90°, 180°
• All constraint directions (top, right, bottom, left)
• Animated continuous rotation (0→360°)
• Combination scenarios (animated rotation + constraints)

Suggested Additional Tests

1. Non-standard transform origins (top-left, bottom-right, etc.)
2. 3D rotations (rotateX, rotateY, rotateZ combinations)
3. Scale + rotation combinations
4. Deeply nested rotations (5+ levels)

---

🔒 Security

No security concerns identified. This is purely computational geometry.

---

📋 Recommendations

Must Address

1. ⚠️ Transform origin limitation: Document or fix the non-center origin case

Should Consider

2. Remove unused lastParentRotation field or document its purpose
3. Add test for non-center transform origins
4. Benchmark performance with deeply nested hierarchies

Nice to Have

5. Add JSDoc for applyDelta() method
6. Add tests for scale + rotation combinations
7. Consider caching parent rotation during drag session

---

Conclusion

This is a high-quality PR that solves a real user pain point with clean, mathematical correctness. The delta-based approach shows thoughtful iteration. The main concern is the transform origin assumption, which should be validated against real-world usage patterns.

Recommendation: Approve with minor revisions (address transform origin documentation/testing)

Great work on the comprehensive test suite and clear implementation! 🎉

[claude]

Pull Request Review: Allow drag within rotated parents

Overview

This PR addresses issue #3320, fixing drag behavior when a draggable element has a rotated parent. The implementation transforms drag offsets from screen space to local coordinate space and includes comprehensive E2E tests.

---

Strengths

1. Well-Architected Solution

• Clean separation of concerns with applyParentRotation(), getParentRotation(), and updatePositionFromScreenOffset()
• Minimal bundle impact using only Math.cos/Math.sin - no new dependencies
• Leverages existing projection system infrastructure

2. Excellent Test Coverage

• 366 lines of comprehensive Cypress E2E tests covering static rotations, all constraint directions, animated rotating parents, and edge cases
• Flexible test component with URL parameters for different scenarios

3. Frame Loop Implementation
The frame loop approach elegantly handles animated parent rotations by recalculating position each frame, ensuring the element stays under the cursor as the parent rotates.

---

Code Quality Observations

1. Performance Consideration: Frame Loop Overhead

The frame loop runs on every frame during drag, even when the parent isn't animating. Each frame calls getParentRotation() which iterates through the projection path, performs trig calculations even when rotation hasn't changed, and calls updateAxis() and render() unnecessarily.

Suggestion: Consider optimizing for the static case by caching the rotation value and only updating if changed, or detecting if parent has animated rotation before starting the frame loop.

2. Mathematical Correctness
The 2D rotation transformation (lines 426-446) is mathematically correct, properly transforming from screen space to local space using inverse rotation.

3. Velocity Transformation
Good catch applying the same rotation to velocity at line 296 - this ensures momentum/inertia animations work correctly.

---

Potential Issues

1. Edge Case: Transform Origin
The rotation transformation assumes rotation around (0,0). If a parent has a non-default transformOrigin, the math may not account for the offset.

2. 3D Rotations Not Handled
The current implementation only handles 2D rotation (z-axis/rotate property). It doesn't account for rotateX, rotateY, skew transforms, or combined transforms with scale.

Recommendation: Add a comment documenting this limitation.

3. Multiple Nested Rotations
Line 406-418 accumulates rotation from all parents additively. While this seems correct, verify this matches how CSS transforms actually compose in the DOM.

---

Test Coverage

Excellent coverage overall, but consider adding:

1. Negative rotation tests (-90, -180) and angles > 360
2. Near-zero movements (1-2px) to ensure no precision issues
3. Multiple nested rotations test scenario
4. Constraint edge cases with asymmetric bounds
5. Performance benchmark for frame loop

---

Security

No security issues identified. The code properly cleans up frame loops and uses safe mathematical operations.

---

Recommendations

Should Consider (High Priority):

1. Performance optimization for static parents (avoid unnecessary frame loop overhead)
2. Document limitations regarding 3D transforms
3. Add edge case tests (negative angles, nested rotations, asymmetric constraints)

Nice to Have:
4. Add performance benchmark
5. Consider warning users about unsupported transform types

---

Final Verdict

Approve with minor recommendations. This is a solid fix that correctly solves the reported issue, has excellent test coverage, follows the codebase patterns, and introduces minimal bundle size impact.

The frame loop performance concern is the main area for potential optimization, but not a blocker since it only affects active drag operations.

Great work on the comprehensive testing and clean implementation!

[mattgperry]

Closing as although it works well for static rotations I feel like a more comprehensive approach would take into rotate animations also and it's not quite there.