fix: remove the delay in enabling of links after dragging#3697
fix: remove the delay in enabling of links after dragging#3697
Conversation
📝 WalkthroughWalkthroughThis PR fixes first-click consumption on Horizontal Scroller buttons by correcting event listener lifecycle management. The disabled-link click listeners are now removed at the end of drag interactions (mouseUpHandler) instead of during drag processing, preventing stale listeners from accumulating and blocking clicks. ChangesHorizontal Scroller Event Listener Cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/block/horizontal-scroller/frontend-horizontal-scroller.js (2)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
dragTimeoutis shared across all scroller elements
dragTimeoutis declared once ininit()scope and shared by every element'smouseDownHandler. On a page with multiple horizontal scrollers, scroller B'smousedownwillclearTimeout(dragTimeout)mid-flight for scroller A, leaving scroller A permanently stuck with thestk--snapping-deactivatedclass.🐛 Proposed fix — scope `dragTimeout` per element
els.forEach( el => { + let dragTimeout = null if ( el._StackableHasInitHorizontalScroller ) {- let dragTimeout = null let initialScrollLeft = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/block/horizontal-scroller/frontend-horizontal-scroller.js` at line 9, The shared dragTimeout declared in init() causes cross-element interference; make the timeout instance-scoped per scroller element by removing the single outer let dragTimeout and instead attach a per-element timeout (e.g., el._dragTimeout or a local let inside the per-element setup) and update all references in mouseDownHandler, mouseMoveHandler and mouseUpHandler to use that per-element property; ensure clearTimeout and setting to null happen on the same element (and on cleanup) so one scroller’s mousedown won’t cancel another’s snapping state (stk--snapping-deactivated).
53-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale click-blockers persist when the browser loses focus mid-drag (alt-tab / tab switch)
The fix correctly moves link re-enablement from inside the 500 ms timeout into
mouseUpHandler. However,mouseUpHandleronly runs whenmouseupfires ondocument.body. Changing desktops or tabs while the mouse is held down results in nomouseupevent ondocumentin either Firefox or Chrome on Windows. This means:
- User starts dragging →
mouseMoveHandlerattachesonClickHandlerto every child.- User alt-tabs while holding the button →
mouseupnever fires →mouseUpHandlernever runs → click listeners remain on children.- User returns to the tab and clicks a link → first click is consumed (re-creates the original bug).
This matches the issue's stated symptom: "The behavior recurs when a browser tab loses and regains focus."
The well-known mitigation is to treat
window.bluras a drag-end signal and run the same cleanup:🛡️ Proposed fix — add a `window` blur handler
const mouseUpHandler = function() { document.body.removeEventListener( 'mousemove', mouseMoveHandler ) document.body.removeEventListener( 'mouseup', mouseUpHandler ) + window.removeEventListener( 'blur', mouseUpHandler ) el.style.cursor = '' // ... rest unchanged } const mouseDownHandler = function( e ) { // ... document.body.addEventListener( 'mousemove', mouseMoveHandler ) document.body.addEventListener( 'mouseup', mouseUpHandler ) + window.addEventListener( 'blur', mouseUpHandler ) }One effective mitigation is to add an additional blur event handler on
window; the blur event fires when the window loses focus, and ending any drags in progress when this event fires produces a safe approximation of the true mouse state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/block/horizontal-scroller/frontend-horizontal-scroller.js` around lines 53 - 79, The drag cleanup in mouseUpHandler never runs if the tab/window loses focus mid-drag, leaving click-blockers attached; add a window 'blur' listener when starting a drag (where mouseMoveHandler is attached) that invokes the same cleanup as mouseUpHandler (or calls mouseUpHandler) so children have onClickHandler removed, dragTimeout cleared/handled, and class toggles restored; also ensure the blur listener is removed inside mouseUpHandler after performing the cleanup to avoid leaks.
🧹 Nitpick comments (1)
src/block/horizontal-scroller/frontend-horizontal-scroller.js (1)
44-50: 💤 Low valueLink-disabling listeners are attached on every
mousemoveevent
children.forEach(child => child.addEventListener('click', onClickHandler, true))runs once permousemove, i.e., once per pixel of movement. WhileaddEventListenerdeduplicates identical listeners (same reference + type + capture), the forEach loop still executes on every event. These listeners only need to be attached once per drag — move them tomouseDownHandler.♻️ Proposed refactor
const mouseMoveHandler = function( e ) { const dx = e.clientX - initialClientX el.scrollTo( { left: initialScrollLeft - dx } ) e.preventDefault() - children.forEach( child => { - child.addEventListener( 'click', onClickHandler, true ) - } ) } const mouseDownHandler = function( e ) { el.style.cursor = 'grabbing' clearTimeout( dragTimeout ) el.classList.add( 'stk--snapping-deactivated' ) initialScrollLeft = el.scrollLeft initialClientX = e.clientX document.body.addEventListener( 'mousemove', mouseMoveHandler ) document.body.addEventListener( 'mouseup', mouseUpHandler ) + window.addEventListener( 'blur', mouseUpHandler ) + children.forEach( child => { + child.addEventListener( 'click', onClickHandler, true ) + } ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/block/horizontal-scroller/frontend-horizontal-scroller.js` around lines 44 - 50, The click-disabling listeners are being added inside the mousemove flow causing redundant per-pixel iterations; move the child.addEventListener('click', onClickHandler, true) loop out of the mouseMoveHandler and into mouseDownHandler so listeners are attached once per drag start (use the same onClickHandler reference), and ensure you remove them in mouseUpHandler via child.removeEventListener('click', onClickHandler, true) to restore normal link behavior after the drag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/block/horizontal-scroller/frontend-horizontal-scroller.js`:
- Line 9: The shared dragTimeout declared in init() causes cross-element
interference; make the timeout instance-scoped per scroller element by removing
the single outer let dragTimeout and instead attach a per-element timeout (e.g.,
el._dragTimeout or a local let inside the per-element setup) and update all
references in mouseDownHandler, mouseMoveHandler and mouseUpHandler to use that
per-element property; ensure clearTimeout and setting to null happen on the same
element (and on cleanup) so one scroller’s mousedown won’t cancel another’s
snapping state (stk--snapping-deactivated).
- Around line 53-79: The drag cleanup in mouseUpHandler never runs if the
tab/window loses focus mid-drag, leaving click-blockers attached; add a window
'blur' listener when starting a drag (where mouseMoveHandler is attached) that
invokes the same cleanup as mouseUpHandler (or calls mouseUpHandler) so children
have onClickHandler removed, dragTimeout cleared/handled, and class toggles
restored; also ensure the blur listener is removed inside mouseUpHandler after
performing the cleanup to avoid leaks.
---
Nitpick comments:
In `@src/block/horizontal-scroller/frontend-horizontal-scroller.js`:
- Around line 44-50: The click-disabling listeners are being added inside the
mousemove flow causing redundant per-pixel iterations; move the
child.addEventListener('click', onClickHandler, true) loop out of the
mouseMoveHandler and into mouseDownHandler so listeners are attached once per
drag start (use the same onClickHandler reference), and ensure you remove them
in mouseUpHandler via child.removeEventListener('click', onClickHandler, true)
to restore normal link behavior after the drag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab963f70-c704-4338-aa6e-799bbade8cfe
📒 Files selected for processing (1)
src/block/horizontal-scroller/frontend-horizontal-scroller.js
1 participant
fixes #3694
Disabling of links when dragging and re-enabling them are introduced here: #2666. However, the re-enabling functionality is added to the snapping delay, making the links still unnecessarily disabled at that point.
Summary by CodeRabbit