Fix drag element detaching from cursor during scroll #3449
Conversation
Code Review: Fix drag element detaching from cursor during scrollSummaryThis PR addresses issue #1691 where dragging elements would detach from the cursor when scrolling a container with Positive Aspects ✅
Issues & Concerns 🚨1. Memory Leak Risk - WeakMap Not Properly ClearedSeverity: Medium The Recommendation: cancel() {
this.isDragging = false
const { projection, animationState } = this.visualElement
if (projection) {
projection.isAnimationBlocked = false
}
this.panSession && this.panSession.end()
this.panSession = undefined
const { dragPropagation } = this.getProps()
if (!dragPropagation && this.openDragLock) {
this.openDragLock()
this.openDragLock = null
}
if (this.removeScrollListeners) {
this.removeScrollListeners()
this.removeScrollListeners = null
}
// Clear scroll tracking data
const ancestors = this.getScrollableAncestors()
for (const element of ancestors) {
this.initialAncestorScrolls.delete(element)
}
animationState && animationState.setActive("whileDrag", false)
}2. Logic Error in Scroll Adjustment CalculationSeverity: High Lines 618-623 contain redundant and potentially confusing logic: const newOrigin = this.originPoint[axis] - scrollDelta
if (newOrigin !== this.originPoint[axis]) {
const adjustment = this.originPoint[axis] - newOrigin
this.originPoint[axis] = newOrigin
motionValue.set(motionValue.get() + adjustment)
}The Recommendation: if (scrollDelta !== 0) {
this.originPoint[axis] -= scrollDelta
motionValue.set(motionValue.get() + scrollDelta)
}3. Inefficient Repeated DOM QueriesSeverity: Medium In Recommendation: // Add private field
private scrollableAncestors: Element[] = []
// In startScrollTracking()
private startScrollTracking(): void {
this.scrollableAncestors = this.getScrollableAncestors()
// Capture initial scroll positions
for (const element of this.scrollableAncestors) {
this.initialAncestorScrolls.set(element, {
x: element.scrollLeft,
y: element.scrollTop,
})
}
// Attach scroll listeners
const cleanupFns: VoidFunction[] = this.scrollableAncestors.map((element) =>
addDomEvent(element, "scroll", this.onScroll, { passive: true })
)
this.removeScrollListeners = () => cleanupFns.forEach((fn) => fn())
}
// In onScroll()
private onScroll = (): void => {
if (!this.isDragging) return
eachAxis((axis) => {
const motionValue = this.getAxisMotionValue(axis)
if (!motionValue) return
let scrollDelta = 0
for (const element of this.scrollableAncestors) {
// ...
}
// ...
})
// Update stored positions
for (const element of this.scrollableAncestors) {
this.initialAncestorScrolls.set(element, {
x: element.scrollLeft,
y: element.scrollTop,
})
}
this.visualElement.render()
}
// In cancel()
cancel() {
// ... existing code ...
if (this.removeScrollListeners) {
this.removeScrollListeners()
this.removeScrollListeners = null
}
// Clear cached data
for (const element of this.scrollableAncestors) {
this.initialAncestorScrolls.delete(element)
}
this.scrollableAncestors = []
// ... rest of cleanup ...
}4. Missing Edge Case: Scroll During Constraint ResolutionSeverity: Low If scrolling occurs between Current order (line 191): addValueToWillChange(this.visualElement, "transform")
this.startScrollTracking() // Before animationStateSuggested order: this.resolveConstraints() // Line 152
// ... existing origin recording code ...
this.startScrollTracking() // After constraints are resolved
addValueToWillChange(this.visualElement, "transform")5. Potential Race Condition with
|
Pull Request ReviewGreat work on fixing the scroll-while-drag issue! This is a well-structured solution that addresses a real UX problem. Here's my detailed review: 🎯 Code Quality: ExcellentStrengths:
Minor suggestion:
🐛 Potential Issues1. Scroll event race condition (Medium Priority) In // Calculate delta from initial positions
scrollDelta += current - initial[axis]
// ...
// Then update stored positions
this.initialAncestorScrolls.set(element, { x: element.scrollLeft, y: element.scrollTop })If multiple scroll events fire before a render completes, you might compound the adjustment. Consider:
2. Missing null check (Low Priority) Line 614-615: ⚡ Performance ConsiderationsGood:
Concern:
🔒 Security: No concerns
🧪 Test Coverage: ExcellentStrengths:
Minor suggestions:
📝 Code Style Adherence✅ Uses 🎨 Architecture & DesignExcellent design choices:
Question for consideration:
🔍 Specific Line-by-Line Notes
✨ SummaryThis is high-quality code that solves a legitimate user problem. The implementation is clean, well-tested, and follows best practices. My main concerns are:
These are minor issues that could be addressed in follow-up PRs. I'd recommend approving with minor suggestions. Suggested action items:
Great work overall! 🚀 |
Pull Request Review: Fix drag element detaching from cursor during scrollOverviewThis PR addresses issue #1691 where draggable elements detach from the cursor when scrolling occurs during an active drag operation. The solution tracks scroll positions of ancestor containers and adjusts the drag origin point to compensate. Code Quality & Best Practices ✅Strengths:
Minor Style Notes:
Potential Issues & Bugs 🔍1. Scroll Delta Accumulation Logic (Medium Priority)Location: The current implementation accumulates scroll deltas from ALL scrollable ancestors: let scrollDelta = 0
for (const target of this.scrollableAncestors) {
const initial = this.initialAncestorScrolls.get(target)
if (!initial) continue
const current = getScrollPosition(target)
scrollDelta += current[axis] - initial[axis]
}Issue: This assumes scroll deltas are independent, but nested scrollable containers create a hierarchical relationship. If both a parent and child container scroll, you might be double-counting the scroll offset. Recommendation: Consider testing with deeply nested scrollable containers to verify the delta calculation is correct. 2. Performance Consideration (Low Priority)Location: The
Recommendation: While 3. Edge Case: Dynamic DOM ChangesLocation: Scrollable ancestors are computed once when drag starts. If the DOM structure changes during drag (e.g., a parent becomes scrollable), the tracking won't update. Assessment: This is likely acceptable since DOM changes during drag are rare, but worth documenting. 4. Memory Leak Risk (Low)Location: The Assessment: The existing code already has this pattern with other properties, so this is consistent with current risk tolerance. Test Coverage 🧪Strengths:
Suggestions:
Security Concerns 🔒No security issues identified. The implementation:
Performance Considerations ⚡Current Implementation:
Recommendations:
Additional Recommendations 📝1. Consider layoutScroll IntegrationThe issue mentions 2. DocumentationConsider adding:
3. Browser CompatibilityThe implementation uses:
No compatibility issues expected. ✅ SummaryThis is a solid implementation that effectively solves the reported issue. The code is clean, well-tested, and follows repository conventions. The main areas for follow-up are:
Overall Assessment: Ready to merge with minor suggestions for follow-up improvements. Great work! 🎉 |
Pull Request ReviewI've reviewed PR #3449 that fixes drag element detaching from cursor during scroll. Overall, this is a well-implemented solution with comprehensive test coverage. Below are my findings: ✅ Strengths
|
Code ReviewThank you for addressing issue #1691! This PR implements scroll compensation during drag operations. I've reviewed the implementation and have the following feedback: Critical Issues1. Console Logs in Production Code
|
PR Review: Fix drag element detaching from cursor during scrollSummaryThis PR addresses issue #1691 by preventing drag elements from detaching from the cursor when scrolling a container with Code Quality & Best Practices ✅Strengths
Minor Suggestions
const cleanupFns = this.scrollableAncestors.map(target =>
addDomEvent(target, "scroll", this.onScroll, { passive: true })
)
this.removeScrollListeners = () => cleanupFns.forEach(fn => fn())Potential Bugs & Issues 🔍Critical IssuesNone identified. Observations
Performance Considerations ⚡Potential Optimizations
private onScroll = (): void => {
if (!this.isDragging || this.scheduledScrollUpdate) return
this.scheduledScrollUpdate = true
requestAnimationFrame(() => {
this.scheduledScrollUpdate = false
this.handleScrollUpdate()
})
}
Positive Performance Aspects
Security Concerns 🔒No security issues identified. The code:
Test Coverage 🧪Strengths
Suggestions for Additional Tests
describe("isScrollable", () => {
it("returns true for overflow:scroll with scrollable content", () => { ... })
it("returns false for overflow:hidden", () => { ... })
it("returns false for overflow:auto without scrollable content", () => { ... })
})
describe("getScrollPosition", () => {
it("returns window scroll position for window", () => { ... })
it("returns element scroll position for elements", () => { ... })
})
Specific Code Review
|
mattgperry
force-pushed
the
scroll-while-drag
branch
from
0642419 to
373a498
Compare
PR Review: Fix drag element detaching from cursor during scrollOverviewThis PR addresses issue #1691 where draggable elements would detach from the cursor when a scrollable ancestor container scrolls during an active drag operation. The solution tracks scroll positions of ancestors and compensates by adjusting the origin point. Code Quality & Best Practices ✅Positive aspects:
Minor style note:
Logic & Implementation 🔍The approach is sound:
Potential issue - Cumulative scroll delta calculation (lines 619-626): let scrollDelta = 0
for (const target of this.scrollableAncestors) {
const initial = this.initialAncestorScrolls.get(target)
if (!initial) continue
const current = getScrollPosition(target)
scrollDelta += current[axis] - initial[axis]
}This calculates the total scroll delta from all ancestors, but this could be incorrect if you have nested scrollable containers. When a child container scrolls, its scroll offset is already relative to its parent, so adding parent scroll deltas might double-count the movement. Example scenario: The element's viewport position changes by the sum, but the compensation might be over-applied. However, given that scroll positions are viewport-relative for scrollable elements, this might actually be correct. Would benefit from testing with deeply nested scrollable containers. Line 637-638: Position updates after adjustment: for (const target of this.scrollableAncestors) {
this.initialAncestorScrolls.set(target, getScrollPosition(target))
}✅ Good - this enables incremental tracking, preventing accumulation errors on subsequent scrolls. Performance Considerations ⚡Concerns:
Security Concerns 🔒No security issues identified:
Test Coverage 📋Excellent test coverage:
Missing test cases (nice-to-have):
Potential Bugs 🐛
Recommendations 📌Critical:
High Priority:
Nice to Have:
SummaryThis is a well-implemented fix that addresses a real usability issue. The code is clean, well-documented, and follows project conventions. Test coverage is comprehensive. The main consideration is verifying behavior with nested scrollable containers in real-world usage. Recommendation: ✅ Approve with minor testing suggestion Great work! The implementation is thoughtful and the test coverage is excellent. 🤖 Generated with Claude Code |
mattgperry
force-pushed
the
scroll-while-drag
branch
from
373a498 to
ae1956f
Compare
Code Review SummaryThis PR implements a fix for drag elements detaching from the cursor when scrolling during drag operations. Overall, the implementation is solid and well-tested, but there are a few considerations worth addressing. ✅ Strengths1. Clean Architecture
2. Excellent Test Coverage
3. Performance Considerations
|
mattgperry
force-pushed
the
scroll-while-drag
branch
2 times, most recently
from
fce5557 to
3b613ff
Compare
Code Review for PR #3449: Fix drag element detaching from cursor during scrollThanks for working on this fix! Overall, this is a solid implementation that addresses the scroll-while-drag issue effectively. Here's my detailed review: ✅ Strengths
🐛 Potential Issues & Suggestions1. Memory Leak Risk with Arrow Function (Medium Priority)Location: The this.removeScrollListener = () =>
window.removeEventListener("scroll", this.onScroll, { capture: true })The // Current (line 588-589)
window.addEventListener("scroll", this.onScroll, {
capture: true,
passive: true,
})
// Should be:
this.removeScrollListener = () =>
window.removeEventListener("scroll", this.onScroll, {
capture: true
// Note: passive doesn't need to be specified for removal, but capture must match
})Actually, looking at the code again, this is correct! The 2. Type Safety Improvement (Low Priority)Location: The type narrowing for const eventTarget = event.target
const target: Element | Window =
eventTarget === document || eventTarget === document.documentElement
? window
: (eventTarget as Element)This assumes const eventTarget = event.target
if (!eventTarget) return
const target: Element | Window =
eventTarget === document || eventTarget === document.documentElement
? window
: (eventTarget as Element)3. Edge Case: Nested Scrollable Containers (Low Priority)The current implementation tracks all ancestor scroll positions and handles multiple scroll containers correctly. However, when multiple ancestors scroll simultaneously (unlikely but possible), the compensation is applied incrementally. This is actually the correct behavior - nice work! 4. Performance: Unnecessary Render on Zero Delta (Low Priority)Location: The code correctly returns early if eachAxis((axis) => {
const delta = current[axis] - initial[axis]
if (delta === 0) return // ✅ Good early return
// ... update logic
})
// Update stored position for incremental delta calculation
this.initialAncestorScrolls.set(target, current)
this.visualElement.render() // ⚠️ Called even if no updates were madeConsider tracking whether any updates occurred: let hasUpdates = false
eachAxis((axis) => {
const delta = current[axis] - initial[axis]
if (delta === 0) return
hasUpdates = true
// ... rest of logic
})
this.initialAncestorScrolls.set(target, current)
if (hasUpdates) {
this.visualElement.render()
}5. Test Coverage Gap (Medium Priority)Location: The Cypress test acknowledges that it cannot reliably test the actual scroll-during-drag behavior: /**
* Note: Testing actual scroll-during-drag is difficult in Cypress as
* scrollTo doesn't reliably trigger scroll events during pointer capture.
* The scroll compensation feature has been manually verified to work.
*/While manual testing is mentioned, consider:
🔒 Security ConsiderationsNo security concerns identified. The implementation:
⚡ Performance ConsiderationsPositive:
Minor Optimization Opportunity:
📋 Code Style ComplianceThe code follows the repository's conventions per CLAUDE.md:
🧪 Test Coverage AssessmentManual Test Page (
E2E Tests:
📝 Recommendations
✨ SummaryThis is a well-thought-out solution to a tricky problem. The implementation is clean, efficient, and follows best practices. The main areas for improvement are:
The fix should work correctly for the reported issue (#1691). Great job! 🎉 Overall Assessment: ✅ Approved with minor suggestions |
Code ReviewI've reviewed PR #3449 which fixes drag elements detaching from the cursor during scroll. Overall, this is a well-implemented solution with good test coverage. Here's my detailed feedback: ✅ Strengths1. Clean Architecture
2. Efficient Event Handling
3. Proper Cleanup
4. Comprehensive Test Coverage
🔍 Potential Issues & Improvements1. Performance: Unnecessary Ancestor Tracking (Medium Priority)Location: The current implementation stores scroll positions for all ancestors, even non-scrollable ones: let current = element.parentElement
while (current) {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
current = current.parentElement
}Issue: For deeply nested DOM trees, this creates unnecessary Map entries and checks during scroll events. Suggestion: Only track scrollable ancestors: let current = element.parentElement
while (current) {
const isScrollable = current.scrollHeight > current.clientHeight ||
current.scrollWidth > current.clientWidth
if (isScrollable) {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
}
current = current.parentElement
}2. Edge Case: Scroll During Constraints Calculation (Low Priority)Location: If an element has Consideration: Should constraints be re-evaluated after scroll compensation? Or is the current behavior (element stays with cursor even if outside constraints) the desired UX? 3. Type Safety: Event Target Casting (Low Priority)Location: const eventTarget = event.target
const target: Element | Window =
eventTarget === document || eventTarget === document.documentElement
? window
: (eventTarget as Element)Issue: The cast Suggestion: Add a null check: if (\!eventTarget || \!(eventTarget instanceof Element || eventTarget === document || eventTarget === document.documentElement)) return4. Missing Edge Case Test (Low Priority)The test suite doesn't cover:
Suggestion: Add a test for nested scrollables: it("Handles nested scrollable containers", () => {
// Test with outer and inner scrollable divs
})5. Code Style: Arrow Function Consistency (Very Low Priority)Location:
🔒 Security Considerations✅ No security concerns identified:
🎯 Test Coverage AssessmentGood coverage overall, but could be enhanced: ✅ Covered:
📝 Documentation & Code Quality✅ Excellent:
Minor suggestion: Add a comment explaining why window is always tracked even if the document doesn't scroll. ✅ RecommendationAPPROVE with minor suggestions. This PR successfully fixes issue #1691 with a clean, well-tested implementation. The suggestions above are optimizations and edge cases that don't block merging. The core functionality is solid and the fix works as intended. Priority of follow-ups:
Great work on this fix! 🎉 |
mattgperry
force-pushed
the
scroll-while-drag
branch
from
3b613ff to
417dd5f
Compare
Pull Request ReviewOverviewThis PR addresses issue #1691 by fixing a bug where draggable elements would detach from the cursor when a scrollable ancestor container scrolls during an active drag operation. The solution tracks scroll positions of ancestor elements and compensates for scroll deltas by adjusting the origin point. ✅ Strengths
🔍 Issues & Concerns1. Critical: Memory Leak RiskSeverity: High The private onScroll = (event: Event): void => {This creates a new function instance for every instance of Recommendation: The current implementation is actually correct because you're storing the removal function. However, consider adding a safety check in private startScrollTracking(): void {
const element = this.visualElement.current
if (\!element) return
// Prevent adding duplicate listeners
if (this.removeScrollListener) {
return
}
// ... rest of implementation
}2. Potential Performance Issue: Traversing All AncestorsSeverity: Medium In let current = element.parentElement
while (current) {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
current = current.parentElement
}Issues:
Recommendation: Only track scrollable ancestors: let current = element.parentElement
while (current) {
const style = window.getComputedStyle(current)
const overflowX = style.overflowX
const overflowY = style.overflowY
if (overflowX === 'auto' || overflowX === 'scroll' ||
overflowY === 'auto' || overflowY === 'scroll') {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
}
current = current.parentElement
}3. Edge Case: Document/Window Scroll HandlingSeverity: Low In const initial = this.initialAncestorScrolls.get(target)
if (\!initial) returnThe comment mentions ignoring window/document scroll, but when Recommendation: Add explicit guard: const target = event.target as Element
if (\!target || target === document as any || target === window as any) return4. Test Coverage Gap: Horizontal ScrollSeverity: Low The Cypress tests only verify vertical scrolling ( Recommendation: Add a test case for horizontal scrolling to ensure both axes work correctly. 5. Test Coverage Gap: Multiple Scrollable AncestorsSeverity: Low The test case only has one scrollable ancestor. Real-world scenarios might have nested scrollable containers. Recommendation: Add a test with nested scrollable containers to verify cumulative scroll compensation works correctly. 6. Race Condition: Render TimingSeverity: Low In Recommendation: Consider debouncing or using private scheduleRender() {
if (this.renderScheduled) return
this.renderScheduled = true
requestAnimationFrame(() => {
this.visualElement.render()
this.renderScheduled = false
})
}🎯 Code Quality
🔒 SecurityNo security concerns identified. The code:
🧪 TestingStrengths:
Gaps:
📋 SummaryThis is a solid implementation that solves the reported issue effectively. The main concerns are:
The implementation demonstrates good understanding of the codebase and follows established patterns. With the suggested optimizations, this will be production-ready. Recommendation: ✅ Approve with minor changes 📝 Suggested Changes// In startScrollTracking()
private startScrollTracking(): void {
const element = this.visualElement.current
if (\!element || this.removeScrollListener) return
// Only track scrollable ancestors
let current = element.parentElement
while (current) {
const style = window.getComputedStyle(current)
const overflowX = style.overflowX
const overflowY = style.overflowY
if (overflowX === 'auto' || overflowX === 'scroll' ||
overflowY === 'auto' || overflowY === 'scroll') {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
}
current = current.parentElement
}
window.addEventListener('scroll', this.onScroll, {
capture: true,
passive: true,
})
this.removeScrollListener = () =>
window.removeEventListener('scroll', this.onScroll, { capture: true })
}Great work on this fix! 🚀 |
mattgperry
force-pushed
the
scroll-while-drag
branch
from
417dd5f to
d7a042d
Compare
PR Review: Fix drag element detaching from cursor during scrollOverall this is a well-implemented fix for issue #1691. The solution properly addresses the scroll compensation problem with a clean, efficient approach. Here's my detailed feedback: ✅ Strengths
🐛 Potential Issues & Suggestions1. Memory Leak Risk with Arrow Function (Medium Priority)Location: The window.addEventListener("scroll", this.onScroll, { capture: true, passive: true })
// ...
window.removeEventListener("scroll", this.onScroll, { capture: true })The Recommendation: Make the removal consistent: this.removeScrollListener = () =>
window.removeEventListener("scroll", this.onScroll, {
capture: true,
// passive doesn't need to be specified for removal, but capture must match
})Actually, looking at the spec, this should work fine as-is since 2. Potential Type Safety Issue (Low Priority)Location: const target = event.target as Element
// ...
const current = { x: target.scrollLeft, y: target.scrollTop }The const target = event.target
if (!(target instanceof Element)) return3. Edge Case: Nested Scrollable Elements (Medium Priority)Location: When multiple nested ancestors scroll simultaneously (or in rapid succession), the current implementation updates
The incremental updates should handle this correctly, but it might be worth adding a comment explaining this behavior or adding a test case for nested scrolling. Recommendation: Add a test case or comment explaining nested scroll behavior. 4. Performance: Unnecessary Render Calls (Low Priority)Location: The scroll handler always calls private onScroll = (event: Event): void => {
if (!this.isDragging) return
const target = event.target as Element
const initial = this.initialAncestorScrolls.get(target)
if (!initial) return
const current = { x: target.scrollLeft, y: target.scrollTop }
let hasUpdated = false
eachAxis((axis) => {
const delta = current[axis] - initial[axis]
if (delta === 0) return
const motionValue = this.getAxisMotionValue(axis)
if (!motionValue) return
this.originPoint[axis] += delta
motionValue.set(motionValue.get() + delta)
hasUpdated = true
})
this.initialAncestorScrolls.set(target, current)
if (hasUpdated) {
this.visualElement.render()
}
}This is a micro-optimization and may not be necessary, but it prevents unnecessary render cycles. 5. Test Coverage Gaps (Medium Priority)The current tests are good but could be expanded: Missing test scenarios:
Recommendation: Add at least one test for nested scrollable containers since that's the most likely real-world edge case. 6. Race Condition Potential (Low Priority)Location: this.startScrollTracking()
const { animationState } = this.visualElement
animationState && animationState.setActive("whileDrag", true)If a scroll event fires immediately after 🎯 Style & Convention Notes
🧪 Test QualityStrengths:
Suggestions:
🔒 Security ConsiderationsNo security concerns identified. The implementation:
⚡ Performance ConsiderationsGood:
Could be optimized (minor):
📝 SummaryThis is a solid implementation that solves the reported issue. The code is clean, well-documented, and properly handles cleanup. The main suggestions are:
Recommendation: ✅ Approve with minor suggestions The core logic is sound and the fix works as intended. The suggestions above are mostly minor improvements that could be addressed in follow-up PRs if needed. Great work on this fix! 🎉 |
mattgperry
force-pushed
the
scroll-while-drag
branch
from
d7a042d to
53e426f
Compare
Code Review - PR #3449: Fix drag element detaching from cursor during scrollSummaryThis PR fixes issue #1691 where draggable elements detach from the cursor when scrolling a container with ✅ Strengths1. Well-structured solution
2. Excellent test coverage
3. Good documentation
🔍 Code Quality Issues1. Memory leak potential (lines 567-589) The let current = element.parentElement
while (current) {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
current = current.parentElement
}Issues:
Recommendation: let current = element.parentElement
let depth = 0
const MAX_DEPTH = 50 // Prevent runaway loops
while (current && depth < MAX_DEPTH) {
this.initialAncestorScrolls.set(current, {
x: current.scrollLeft,
y: current.scrollTop,
})
current = current.parentElement
depth++
}2. Arrow function binding issue (line 595) The private onScroll = (event: Event): void => {Issues:
Status: This is acceptable given the need for proper 3. Race condition potential (lines 606-616) The scroll handler modifies eachAxis((axis) => {
const delta = current[axis] - initial[axis]
if (delta === 0) return
const motionValue = this.getAxisMotionValue(axis)
if (!motionValue) return
this.originPoint[axis] += delta
motionValue.set(motionValue.get() + delta)
})Issues:
Recommendation: 4. Potential null reference (line 610) const motionValue = this.getAxisMotionValue(axis)
if (!motionValue) returnWhile there's a null check, this silently fails if a motion value is missing. This might hide bugs where the drag system is in an inconsistent state. Recommendation:
|
mattgperry
force-pushed
the
scroll-while-drag
branch
from
53e426f to
6a95dae
Compare
When dragging an element inside a scrollable container (or the window), the element would detach from the cursor if the container scrolled during the drag operation. This fix: - Detects scrollable ancestors by walking the DOM tree (checking overflow and scroll dimensions) - Always includes window for document-level scrolling - Attaches scroll listeners when drag starts - Adjusts originPoint and motionValue when scroll events fire to keep the element under the cursor - Cleans up listeners when drag ends Includes e2e tests for both element scroll and window scroll scenarios. Fixes #1691 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
mattgperry
force-pushed
the
scroll-while-drag
branch
from
6a95dae to
5d53f13
Compare
Move scroll compensation logic from VisualElementDragControls into PanSession where it belongs. This ensures scroll events during drag flow through the normal onMove pipeline, so constraints, elastic boundaries, and callbacks all work correctly. - Add scrollableElement option to PanSession - Element scroll: adjust history[0] to simulate pointer movement - Window scroll: adjust lastMoveEventInfo.point for pageX/pageY change - Remove 100+ lines of scroll handling from VisualElementDragControls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Only track ancestors with scrollable overflow styles - DRY up scroll handlers with unified handleScroll method - Use single scrollPositions map for elements and window - Rename scrollableElement option to element 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Pull Request ReviewSummaryThis PR fixes issue #1691 where draggable elements detach from the cursor when scrolling occurs during an active drag. The solution tracks scroll positions of layoutScroll ancestors and compensates for scroll movements by adjusting the drag origin point. Code Quality and ImplementationStrengths
Potential Issues and ConcernsCRITICAL: Scroll Event Listener ScopeLocation: PanSession.ts:195-203 The scroll listeners are attached to window globally, which means they will fire for ALL scroll events in the document, not just the tracked ancestors. The onElementScroll handler will be called for every scroll event, even for unrelated elements. While handleScroll returns early if the element is not in scrollPositions, this is inefficient and could cause performance issues with many scroll events. Recommendation: Consider attaching listeners directly to each tracked scrollable element instead of using a global capture listener. MEDIUM: Potential Race Condition with frame.updateLocation: PanSession.ts:257 The scroll handler schedules updatePoint to run on the next frame, but there is no guarantee about the timing relative to the next pointermove event. Consider debouncing scroll compensation or ensuring idempotency. MEDIUM: Missing Null CheckLocation: PanSession.ts:250-253 Direct mutation of history array without checking if it is still valid. If the drag session ends between scroll compensation calls, this could mutate stale data. Recommend adding a safety check for this.startEvent !== null. MEDIUM: getComputedStyle PerformanceLocation: PanSession.ts:175 getComputedStyle forces a style recalculation and can be expensive. Called for every ancestor during drag start. Worth profiling for deeply nested DOM structures. MINOR: Type SafetyLocation: PanSession.ts:214, 234-237 The cast as Element assumes the scroll target is an Element, but it could be a Document or other EventTarget. Recommend adding a type guard. Testing ConcernsThe Cypress tests acknowledge coordinate quirks and use lenient tolerances. While pragmatic, this could mask real issues. Consider adding tests for:
Security and PerformanceSecurity: No concerns - passive event listeners used appropriately, no XSS vectors, proper cleanup prevents memory leaks. Performance: Main concern is the global scroll listener. Also consider profiling for drag with continuous scrolling, deeply nested DOM structures, and multiple simultaneous drag sessions. Final VerdictRecommendation: Approve with minor changes This is a well-implemented solution to a long-standing issue. The core algorithm is sound, tests are comprehensive, and the code is clean. The main concern is the global scroll listener pattern which could be optimized. Before Merging:
Great work on tackling this complex interaction issue! |
Summary
layoutScrollduring an active draglayoutScrollancestors when drag startsFixes #1691
Test plan
🤖 Generated with Claude Code