Confine the drag press-and-hold delay to touch input - #15186
Conversation
Build Artifacts
Smoke test screenshot |
akolson
added
TODO: needs review
🔵 Review postedLast updated: 2026-08-13 16:02 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15186 — the fix is correct and minimal. Traced SortableJS 1.15.7: _prepareDragStart gates the delay on options.delay && (!options.delayOnTouchOnly || touch), and during the delay window _delayedDragTouchMoveHandler cancels the drag once the pointer moves past touchStartThreshold (devicePixelRatio || 1, i.e. 1px) — so a mouse drag was cancelled instantly. delayOnTouchOnly: true resolves it for mouse while keeping the hold for touch.
CI passing. No UI QA run — the behaviour is a pointer-timing interaction that jsdom can't reproduce; live verification is worth doing before merge.
AC3 (other consumers) is met: new Sortable(...) appears once in the tree (useDraggableRegion.js:223) and spreads sortableDefaults, so LessonResourcesTable, QuestionsAccordion, SectionOrder, OrderInteraction and RearrangeChannelsPage all pick it up; none override delay.
One suggestion and one nitpick inline. Worth noting: touch in SortableJS excludes pointerType === 'pen', so a stylus drag now starts on first movement — arguably right (a pen is precise), but on Android a stylus also scrolls, so the scroll-vs-drag conflict returns for that input. SortableJS offers no pen-inclusive variant; flagging as information only.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
The 250ms press-and-hold delay exists so that a touch drag does not fight the page scroll, but it was applied to every input device. While a delayed drag is pending, SortableJS cancels it as soon as the pointer moves further than touchStartThreshold (1px). A mouse drag is performed by pressing and immediately moving, so that movement landed inside the 250ms window and the gesture was discarded before it began, silently. Rows only picked up when the user happened to hold still for a full quarter second first, which is why dragging appeared broken yet worked occasionally. delayOnTouchOnly confines the delay to touch input, where it is needed, and lets a mouse drag begin on the first movement. The option lives on the shared universe defaults, so every region gets it. Fixes learningequality#15175
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15186 — 2 of 2 prior findings resolved; no new findings.
Prior-finding status
RESOLVED — packages/kolibri-common/components/draggable/tests/DraggableRegion.spec.js:59 — drop the delay assertion pinning an unchanged default
RESOLVED — packages/kolibri-common/components/draggable/useDraggableUniverse.js:56 — consumer-facing docs still described a device-agnostic delay
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
habibayman
left a comment
habibayman
left a comment
There was a problem hiding this comment.
- Code changes are minimal and LGTM
- Ran it locally and it works as expected
Thank you Alan!
4 participants
Summary
This PR fixes drag-and-drop reordering of channels with a mouse.
The drag system configures
SortableJSwith a250mspress-and-hold delay to prevent touch dragging from interfering with page scrolling. That delay was also being applied to mouse input. Because a mouse drag normally involves pressing and immediately moving,SortableJSwould cancel the pending drag before it started, making reordering appear broken. The drag would only work if the user held the mouse still for the full250msbefore moving.Setting
delayOnTouchOnly: truelimits the delay to touch input, where it is needed, while allowing mouse drags to start immediately.Before:
Grabacion.de.pantalla.2026-08-10.a.la.s.10.50.37.a.m.mov
After:
Screen.Recording.2026-08-13.at.17.26.33.mov
References
Fixes #15175
Reviewer guidance
AI usage
Used Claude Code to investigate the bug, identify the root cause and propose the fix. It verified the change against a running instance and added a regression test. I reviewed the diagnosis, the fix and the test before opening this PR.