Fixed issue: user may navigate forward with an opened modal #8748
Conversation
WalkthroughThe changes in this pull request introduce a fix that enhances keyboard navigation when a modal is open, allowing users to navigate forward using the keyboard. Modifications were made to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…en_model_opened.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cvat-ui/src/utils/mousetrap-react.tsx (2)
78-82: Consider making modal detection more robustThe current modal detection logic could be improved in several ways:
- Add null checks for the style property
- Consider additional visibility states beyond just display:none
- Consider using a more reliable method to detect modal state (e.g., data attributes)
Here's a suggested improvement:
const anyModalsOpened = Array.from( window.document.getElementsByClassName('ant-modal'), - ).some((el) => (el as HTMLElement).style.display !== 'none'); + ).some((el) => { + const element = el as HTMLElement; + return element.style?.display !== 'none' && + element.style?.visibility !== 'hidden' && + element.getAttribute('aria-hidden') !== 'true'; + });
82-89: Enhance modal closing sequence handlingWhile the implementation is functional, consider these improvements:
- Move the modal closing action keys to constants
- Add length validation for the key fragments comparison
- Consider memoizing the modal closing sequences to avoid rebuilding on each callback
Here's a suggested improvement:
+ const MODAL_CLOSING_ACTIONS = ['SWITCH_SHORTCUTS', 'SWITCH_SETTINGS'] as const; + if (anyModalsOpened) { - const modalClosingSequences = ['SWITCH_SHORTCUTS', 'SWITCH_SETTINGS'] + const modalClosingSequences = MODAL_CLOSING_ACTIONS .map((key) => [...(applicationKeyMap[key]?.sequences ?? [])]).flat(); return !modalClosingSequences.some((seq) => { const seqFragments = seq.split('+'); + const comboFragments = combo.split('+'); + if (seqFragments.length !== comboFragments.length) return false; - return combo.split('+').every((key, i) => seqFragments[i] === key); + return comboFragments.every((key, i) => seqFragments[i] === key); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
changelog.d/20241127_170908_sekachev.bs_fixed_user_may_navigate_forward_when_model_opened.md(1 hunks)cvat-ui/src/utils/mousetrap-react.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- changelog.d/20241127_170908_sekachev.bs_fixed_user_may_navigate_forward_when_model_opened.md
🔇 Additional comments (2)
cvat-ui/src/utils/mousetrap-react.tsx (2)
67-67: LGTM: Clear and accurate comment update
The comment accurately describes the logic for input field handling.
78-89: Verify the fix prevents all unwanted navigation
The implementation looks correct, but let's ensure we're catching all navigation-related shortcuts.
✅ Verification successful
Based on the search results, I can see that the implementation properly handles navigation-related shortcuts by checking for open modals. The key findings are:
- The code checks for modal-closing sequences (
SWITCH_SHORTCUTSandSWITCH_SETTINGS) which usef1andf2keys respectively - All other navigation shortcuts are properly scoped to their respective workspaces and will be blocked when modals are open
- The implementation covers all major navigation shortcuts including:
- Frame navigation (f, d, v, c, left, right)
- Workspace controls (n, shift+n, g, m, etc.)
- Object navigation (tab, shift+tab)
- Camera controls for 3D (shift+arrows)
Fix properly prevents unwanted navigation with open modals
The implementation correctly blocks all navigation shortcuts when modals are open, while still allowing modal-closing shortcuts to work. The code is thorough and handles all navigation-related shortcuts across different workspaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for navigation-related keyboard shortcuts in the codebase
# to ensure they're properly blocked by the modal check
# Look for keyboard shortcut definitions that might involve navigation
rg -l "keyMap|shortcuts|navigation" | xargs rg -A 5 "sequences.*:.*\[.*\]"
Length of output: 70298
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8748 +/- ##
===========================================
+ Coverage 74.05% 74.09% +0.03%
===========================================
Files 409 409
Lines 43783 43784 +1
Branches 3984 3984
===========================================
+ Hits 32425 32441 +16
+ Misses 11358 11343 -15
|
bsekachev
deleted the
bs/fixed_user_may_navigate_forward_when_model_opened
branch
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation