fix(DragDropSort): enable mobile drag-and-drop support#12418
fix(DragDropSort): enable mobile drag-and-drop support#12418logonoff wants to merge 1 commit intopatternfly:mainfrom
Conversation
WalkthroughRegisters PointerSensor with activationConstraint { distance: 8 }; adds restrictToWindowEdges to DndContext; applies ChangesDrag behavior and input handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 (1)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx (1)
106-112:⚠️ Potential issue | 🔴 CriticalRemove redundant TouchSensor or switch to MouseSensor + TouchSensor; PointerSensor + TouchSensor combination is unsupported and causes duplicate event handling.
The code registers both
PointerSensorandTouchSensor. According to dnd-kit documentation, this combination should not be used as both sensors overlap on touch input.The correct approach is one of the following:
Modern approach (recommended): Use
PointerSensoralone with activation constraints:const sensors = useSensors( useSensor(PointerSensor, { activationConstraint: { delay: 250, tolerance: 5 } }), useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates }) );Legacy approach: If
PointerSensordoes not work for your use case, replace it withMouseSensorand keepTouchSensorwith activation constraints:const sensors = useSensors( useSensor(MouseSensor), useSensor(TouchSensor, { activationConstraint: { delay: 250, tolerance: 5 } }), useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates }) );
PointerSensoris the unified replacement for Mouse and Touch APIs and handles all pointer input types.TouchSensoris legacy from dnd-kit v1 and is redundant whenPointerSensoris present.🤖 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 `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx` around lines 106 - 112, The sensors array currently registers both PointerSensor and TouchSensor which overlap on touch input; remove the TouchSensor and either (recommended) keep PointerSensor with an activationConstraint or (legacy) replace PointerSensor with MouseSensor and keep TouchSensor with activationConstraint; update the useSensors call (symbols: useSensors, useSensor, PointerSensor, TouchSensor, MouseSensor, KeyboardSensor, sortableKeyboardCoordinates) to use one of the two approaches and add activationConstraint (delay/tolerance) to the pointer/mouse or touch sensor as appropriate.
🤖 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 `@packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx`:
- Around line 106-112: The sensors array currently registers both PointerSensor
and TouchSensor which overlap on touch input; remove the TouchSensor and either
(recommended) keep PointerSensor with an activationConstraint or (legacy)
replace PointerSensor with MouseSensor and keep TouchSensor with
activationConstraint; update the useSensors call (symbols: useSensors,
useSensor, PointerSensor, TouchSensor, MouseSensor, KeyboardSensor,
sortableKeyboardCoordinates) to use one of the two approaches and add
activationConstraint (delay/tolerance) to the pointer/mouse or touch sensor as
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 835faa1f-7759-44dd-a3a2-10046563ea2f
📒 Files selected for processing (1)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsx
|
Preview: https://pf-react-pr-12418.surge.sh A11y report: https://pf-react-pr-12418-a11y.surge.sh |
logonoff
force-pushed
the
12415-touch-sensor
branch
from
8552fc4 to
fce3142
Compare
logonoff
changed the title
logonoff
force-pushed
the
12415-touch-sensor
branch
from
fce3142 to
bb0b653
Compare
Set touch-action: none on the drag activator element (DragButton when
useDragButton is true, wrapper div otherwise) to prevent the browser
from intercepting touch events for native scrolling. Without this,
mobile browsers fire pointercancel before the PointerSensor can
activate the drag.
Also adds activationConstraint: { distance: 8 } to PointerSensor so
the drag only activates after deliberate pointer movement, preventing
accidental drags on both desktop and mobile.
Adds restrictToWindowEdges modifier to DndContext so dragged items
cannot be moved outside the browser viewport. Consumers can override
this via the modifiers prop.
Fixes patternfly#12415
logonoff
force-pushed
the
12415-touch-sensor
branch
from
bb0b653 to
d6db4ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/react-drag-drop/src/components/DragDrop/Draggable.tsx`:
- Around line 29-33: The current style object lets caller-provided props.style
overwrite drag-critical CSS (transform, transition, touchAction) because props
is spread after computed style; fix by ensuring computed drag styles take
precedence: when building the style object (the local variable named style in
Draggable.tsx), spread props.style first and then set transform:
CSS.Transform.toString(transform), transition, and ...( !useDragButton && {
touchAction: 'none' }), so the computed values override any caller values; apply
the same change to both places where style is constructed so
transform/transition/touchAction cannot be overridden by props.style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 591c7d81-a876-42cf-ac64-b7c6d8266c38
📒 Files selected for processing (2)
packages/react-drag-drop/src/components/DragDrop/DragDropContainer.tsxpackages/react-drag-drop/src/components/DragDrop/Draggable.tsx
| const style = { | ||
| transform: CSS.Transform.toString(transform), | ||
| transition | ||
| transition, | ||
| ...(!useDragButton && { touchAction: 'none' as const }) | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Draggable is currently used with an explicit style prop in this repository.
# If matches exist, those call sites are directly exposed to this override bug.
rg -nP --type=tsx '<Draggable\b[^>]*\bstyle\s*='
rg -nP --type=tsx 'items\s*=\s*{[\s\S]*?\bprops\s*:\s*{[\s\S]*?\bstyle\s*:' -URepository: patternfly/patternfly-react
Length of output: 1970
🏁 Script executed:
# Locate the Draggable.tsx file
find . -name "Draggable.tsx" -type f 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 207
🏁 Script executed:
# Once located, read the full component to see lines 29-53 and verify the structure
# This will help verify the spreading order concern
cat -n packages/react-drag-drop/src/components/DragDrop/Draggable.tsx 2>/dev/null || echo "File not found at expected path"Repository: patternfly/patternfly-react
Length of output: 2155
🏁 Script executed:
# Search for DraggableProps definition
rg "interface DraggableProps|type DraggableProps" -A 10 2>/dev/null || echo "Definition not found with rg"Repository: patternfly/patternfly-react
Length of output: 2294
🏁 Script executed:
# Search for Draggable component usages with explicit style prop
rg '<Draggable\b[^>]*\bstyle\s*=' --type=tsx --type=tsRepository: patternfly/patternfly-react
Length of output: 99
🏁 Script executed:
# Also search in JSX/TSX for Draggable without explicit opening, to find all usages
rg 'Draggable' --type=tsx --type=ts -B 2 -A 5 | grep -A 7 '<Draggable'Repository: patternfly/patternfly-react
Length of output: 99
🏁 Script executed:
# Search for Draggable component usages with style prop using globbing
rg '<Draggable\b[^>]*\bstyle\s*=' -g '*.tsx' -g '*.ts'Repository: patternfly/patternfly-react
Length of output: 365
🏁 Script executed:
# Also search more broadly for any Draggable usage
rg '<Draggable' -g '*.tsx' -g '*.ts' -B 1 -A 3 | head -100Repository: patternfly/patternfly-react
Length of output: 10297
🏁 Script executed:
# Check where Draggable is exported/imported from
rg "from.*Draggable|import.*Draggable" -g '*.tsx' -g '*.ts' | head -30Repository: patternfly/patternfly-react
Length of output: 3263
🏁 Script executed:
# Check React.CSSProperties type usage in codebase for reference
rg "React\.CSSProperties" -g '*.tsx' -g '*.ts' | head -10Repository: patternfly/patternfly-react
Length of output: 1146
🏁 Script executed:
# Check how other components handle style prop merging in this repo
rg "propStyle|styleProps|\.style" -g '*.tsx' -g '*.ts' -B 2 -A 2 | grep -A 4 "propStyle\|const.*style.*=" | head -40Repository: patternfly/patternfly-react
Length of output: 3612
props.style can override drag-critical styles (including touchAction)
Because ...props is applied after style on Line 40 and Line 52, any caller-provided style replaces transform, transition, and touchAction. This breaks dragging and reintroduces the mobile failure this PR is fixing.
Suggested fix
export const Draggable: React.FunctionComponent<DraggableProps> = ({
children,
id,
className,
useDragButton = false,
+ style: propStyle,
...props
}: DraggableProps) => {
@@
- const style = {
- transform: CSS.Transform.toString(transform),
- transition,
- ...(!useDragButton && { touchAction: 'none' as const })
- };
+ const style = {
+ ...(propStyle as React.CSSProperties),
+ transform: CSS.Transform.toString(transform),
+ transition,
+ ...(!useDragButton ? { touchAction: 'none' as const } : {})
+ };Also applies to: 49-52
🤖 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 `@packages/react-drag-drop/src/components/DragDrop/Draggable.tsx` around lines
29 - 33, The current style object lets caller-provided props.style overwrite
drag-critical CSS (transform, transition, touchAction) because props is spread
after computed style; fix by ensuring computed drag styles take precedence: when
building the style object (the local variable named style in Draggable.tsx),
spread props.style first and then set transform:
CSS.Transform.toString(transform), transition, and ...( !useDragButton && {
touchAction: 'none' }), so the computed values override any caller values; apply
the same change to both places where style is constructed so
transform/transition/touchAction cannot be overridden by props.style.
2 participants
What: Closes #12415
Three changes to enable mobile drag-and-drop:
Draggable: Settouch-action: noneon the drag activator element — onDragButtonwhenuseDragButtonis true (so only the handle blocks native scrolling), on the wrapper div otherwise. Without this, mobile browsers firepointercancelbefore thePointerSensorcan activate.DragDropContainer: AddactivationConstraint: { distance: 8 }toPointerSensorso the drag only activates after 8px of deliberate pointer movement, preventing accidental drags on both desktop and mobile.DragDropContainer: AddrestrictToWindowEdgesmodifier toDndContextso dragged items cannot be moved outside the browser viewport. Placed before{...props}so consumers can override via themodifiersprop.Additional issues: N/A
Summary by CodeRabbit