Skip to content

Conversation

@simonw
Copy link
Owner

@simonw simonw commented Dec 1, 2025

  • Add touch event handlers (touchstart, touchmove, touchend) for dragging
    the crop box and resizing via corner handles
  • Add touch-action: none CSS to prevent unwanted scrolling while dragging
  • Enlarge resize handles on touch devices (pointer: coarse) for easier tapping

ffmpeg-crop make the cropping work on touch screen mobile devices

- Add touch event handlers (touchstart, touchmove, touchend) for dragging
  the crop box and resizing via corner handles
- Add touch-action: none CSS to prevent unwanted scrolling while dragging
- Enlarge resize handles on touch devices (pointer: coarse) for easier tapping
@claude
Copy link
Contributor

claude bot commented Dec 1, 2025

Claude finished @simonw's task —— View job


PR Review: Touch Screen Support for ffmpeg-crop Tool

Review Checklist

  • Read and analyze the modified ffmpeg-crop.html file
  • Review touch event implementation for correctness
  • Check for potential bugs and edge cases
  • Evaluate security considerations
  • Assess performance implications
  • Review code quality and best practices
  • Evaluate test coverage needs
  • Provide comprehensive feedback and recommendations

Code Review Summary

Overall, this is a solid implementation of touch support for the ffmpeg-crop tool. The touch events are properly implemented with good UX considerations.

Strengths

Touch Implementation Quality

  • Proper event handling: Touch events (touchstart, touchmove, touchend) correctly mirror mouse events
  • Good UX design: Larger touch targets (24px vs 12px handles) on coarse pointer devices via @media (pointer: coarse)
  • Scroll prevention: touch-action: none CSS and { passive: false } + preventDefault() properly prevent unwanted scrolling

Code Structure

  • DRY principle: Touch and mouse handlers share the same logic for dragging and resizing
  • Proper event management: Consistent use of preventDefault() and stopPropagation()
  • Responsive design: Media query approach for touch-specific styling is clean

Boundary Constraints

  • Proper bounds checking prevents crop box from going outside video area
  • Minimum size constraints (50px) prevent unusably small crop areas
  • Handles coordinate transformation between display and video dimensions correctly

🔍 Areas for Improvement

Edge Cases & Robustness

  1. Multi-touch handling: Code assumes single touch (e.touches[0]) but doesn't prevent multi-touch gestures
// Consider adding at start of touch handlers:
if (e.touches.length !== 1) return;
  1. Touch event cleanup: Touch events don't check if touch ID matches the original touch
// Could improve by storing touch identifier and verifying:
let activeTouchId = null;
// In touchstart: activeTouchId = e.touches[0].identifier;
// In touchmove: verify touch.identifier === activeTouchId

Performance Considerations

  1. Event throttling: Continuous updateCommand() calls during drag/resize could be throttled
  2. Memory cleanup: Video URL created with URL.createObjectURL() should be revoked when done

Accessibility

  1. Keyboard support missing: No keyboard navigation for crop box positioning
  2. Screen reader support: Missing ARIA labels for interactive elements
  3. Focus indicators: Touch handles lack visible focus states for keyboard users

🔒 Security Assessment

  • No security concerns identified
  • File handling is safe (only reads video metadata, doesn't execute)
  • No XSS vectors in filename handling (uses textContent, not innerHTML)
  • No network requests or external dependencies

🧪 Testing Considerations

This implementation would benefit from:

  • Touch device testing: Real device testing on phones/tablets
  • Cross-browser testing: Safari iOS, Chrome Android, etc.
  • Edge case testing: Very small videos, extreme aspect ratios
  • Performance testing: Large video files

📝 Minor Code Quality Suggestions

  1. Magic numbers: Consider constants for minimum sizes and handle dimensions:
const MIN_CROP_SIZE = 50;
const TOUCH_HANDLE_SIZE = 24;
  1. Error handling: Add error handling for video loading failures

  2. Debouncing: Consider debouncing the updateCommand() function during rapid movements

🎯 Recommendation

Approve with suggestions. This is a well-implemented feature that significantly improves mobile usability. The touch implementation follows best practices and properly handles the core use cases. The suggested improvements are enhancements rather than blockers.

The code is ready for merge, with the understanding that the suggested improvements could be addressed in future iterations if needed.


@cloudflare-workers-and-pages
Copy link

Deploying tools with  Cloudflare Pages  Cloudflare Pages

Latest commit: e369b12
Status: ✅  Deploy successful!
Preview URL: https://9fd26740.tools-b1q.pages.dev
Branch Preview URL: https://claude-fix-ffmpeg-crop-touch.tools-b1q.pages.dev

View logs

@simonw simonw merged commit c86f222 into main Dec 1, 2025
3 of 4 checks passed
@simonw simonw deleted the claude/fix-ffmpeg-crop-touch-01KkDsaXxe2XgHso1U7aaS2s branch December 1, 2025 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants