-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Ensure drag always applies to keyboard-accessible elements
#3480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Code ReviewOverviewThis PR fixes a regression where draggable keyboard-accessible elements (like ✅ Strengths1. Correct Logic Implementation const isClickingKeyboardAccessibleChild =
target !== element &&
isElementKeyboardAccessible(target)This allows 2. Excellent Test Coverage
3. Clear Documentation 🔍 ObservationsNo Issues Found
📊 Test VerificationThe tests cover all critical scenarios:
Recommendation✅ APPROVED - This is a clean, well-tested fix that correctly addresses issue #3473. The implementation is minimal, the logic is sound, and the test coverage is comprehensive. Ready to merge. |
Previously, the drag gesture was blocked on any keyboard-accessible element (button, input, select, textarea, a) regardless of whether the element itself had the drag prop. This meant motion.button with drag prop couldn't be dragged. The fix only blocks drag when clicking on a keyboard-accessible *child* element of the draggable element. If the draggable element itself is keyboard-accessible, dragging should work when clicking directly on it. Fixes #3473
b67cd79 to
eb5a47c
Compare
Code ReviewSummaryThis PR fixes a regression where keyboard-accessible elements (button, input, select, textarea, a) with the ✅ Strengths1. Correct Logic Fix
2. Excellent Test Coverage
3. Clear Documentation 🔍 Code QualityVariable Naming: Performance: Extracting Readability: The boolean expression is clear and the negation pattern ( 💡 Minor Observations1. Test Pattern Consistency 2. Edge Case: Nested Keyboard-Accessible Elements <motion.button drag>
<span>
<a href="#">Link inside button</a>
</span>
</motion.button>Clicking the 3. Event Bubbling 🎯 Testing ValidationTo verify the fix works as intended, you could run: cd packages/framer-motion && yarn test-client --testNamePattern="keyboard accessible elements"✨ VerdictLGTM! 🚀 This is a well-implemented fix that:
No blocking issues found. The code is ready to merge. |
Previously, the drag gesture was blocked on any keyboard-accessible element (button, input, select, textarea, a) regardless of whether the element itself had the drag prop. This meant motion.button with drag prop couldn't be dragged.
The fix only blocks drag when clicking on a keyboard-accessible child element of the draggable element. If the draggable element itself is keyboard-accessible, dragging should work when clicking directly on it.
Fixes #3473