fix: shortcuts#13003
Conversation
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
|
/backport to stable5.9 |
|
Did you |
|
@coderabitAi review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refines keyboard shortcut handling across the Mail app's mailbox components. In Mailbox.vue, envelope navigation, deletion, and seen-toggle shortcuts gain safety guards to prevent invalid transitions. MailboxThread.vue adds a global Cmd+F shortcut handler to focus the search input, while SearchMessages.vue exposes a programmatic focus method. These changes address macOS shortcut reliability. ChangesKeyboard Shortcut Handling
Sequence DiagramsequenceDiagram
participant User
participant Document
participant MailboxThread
participant SearchMessages
participant SearchInput
User->>Document: Press Cmd+F
Document->>MailboxThread: keydown (capture phase)
MailboxThread->>MailboxThread: handleSearchShortcut() checks isFromFormElement
alt Not from form element
MailboxThread->>MailboxThread: preventDefault()
MailboxThread->>SearchMessages: focusInput() via ref
SearchMessages->>SearchInput: focus() via $refs.searchInput
SearchInput->>User: Input ready for search
else From form element
MailboxThread->>Document: Pass through (no intercept)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Mailbox.vue (1)
409-415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackspace deletion still misses the next-message navigation path.
This newly restored path still calls
onDelete(env.databaseId), butonDeletelater compares that numeric id withthis.$route.params.threadIdusing strict inequality. In this same component the route param is parsed as a string first (Line 367), so deleting the currently open message viabackspace/deltakes the “other message open” branch and never advances to the adjacent message.Suggested fix
- if (id !== this.$route.params.threadId) { + if (id !== parseInt(this.$route.params.threadId, 10)) { logger.debug('other message open, not jumping to the next/previous message') return }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9cc384e-b4a0-4eb8-bb65-71caf2999a94
📒 Files selected for processing (3)
src/components/Mailbox.vuesrc/components/MailboxThread.vuesrc/components/SearchMessages.vue
| handleSearchShortcut(event) { | ||
| if (event.target.matches('input, textarea, [contenteditable]')) { | ||
| return | ||
| } | ||
| if (event.key === 'f' && event.metaKey) { | ||
| event.preventDefault() | ||
| this.$refs.searchMessages?.focusInput() | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On Windows and Linux browsers, is Ctrl+F the keyboard shortcut for the find dialog while Cmd+F is used on macOS?
💡 Result:
Yes, that is correct. Across web browsers on Windows and Linux, Ctrl+F is the standard keyboard shortcut to open the find-in-page dialog [1][2]. On macOS, the standard shortcut for this function is Cmd+F [3][2][4]. This difference aligns with the standard modifier key conventions for each operating system: Windows and Linux applications typically use the Control (Ctrl) key for most keyboard shortcuts, while macOS applications typically use the Command (Cmd/⌘) key [3][1]. Most major web browsers—including Chrome, Firefox, Edge, and Safari—adhere to these platform-specific conventions to provide a consistent user experience [1][2][5].
Citations:
- 1: https://www.lenovo.com/us/en/glossary/ctrl-f/
- 2: https://www.leraweb.org/search-using-browser
- 3: https://gist.github.com/sojeri/9898254abef2d184c659
- 4: https://support.apple.com/en-ca/guide/safari/cpsh003/mac
- 5: https://me.pcmag.com/en/browsers/15948/chrome-edge-firefox-opera-or-safari-which-browser-is-best-for-2023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/components/MailboxThread.vue"
echo "== Context around handleSearchShortcut =="
rg -n "handleSearchShortcut" "$file" || true
nl -ba "$file" | sed -n '480,560p'
echo
echo "== Search for focusInput usage in this component =="
rg -n "\\$refs\\.searchMessages\\?\\.focusInput\\(|focusInput\\(" "$file" || true
echo
echo "== Search repo for other keyboard shortcut handlers =="
rg -n "handleSearchShortcut|searchShortcut|key === 'f'|metaKey.*f|ctrlKey.*f" src/components src || trueRepository: nextcloud/mail
Length of output: 352
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/components/MailboxThread.vue"
echo "== Context around handleSearchShortcut =="
rg -n "handleSearchShortcut" "$file" || true
echo
echo "== Lines 500-540 (for the exact key handler body) =="
cat -n "$file" | sed -n '500,540p'
echo
echo "== Search for focusInput usage in this component =="
rg -n "\\$refs\\.searchMessages\\?\\.focusInput\\(|focusInput\\(" "$file" || true
echo
echo "== Search repo for other keyboard shortcut handlers (metaKey/ctrlKey + f/search) =="
rg -n "handleSearchShortcut|searchShortcut|key === 'f'|metaKey.*f|ctrlKey.*f" src || trueRepository: nextcloud/mail
Length of output: 1660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/components/MailboxThread.vue"
echo "== focusInput usage in MailboxThread.vue (no $refs expansion) =="
rg -n 'focusInput\(' "$file" || true
echo
echo "== Any other keydown/shortcut handling in MailboxThread.vue around this feature =="
rg -n "keydown|key ===|metaKey|ctrlKey|handleSearchShortcut|searchMessages" "$file" || true
echo
echo "== Repo-wide: any other Ctrl/Cmd+F shortcut handlers that focus search =="
rg -n "metaKey|ctrlKey|key === 'f'|key === \"f\"|handleSearchShortcut|focusInput\(" src/components src || trueRepository: nextcloud/mail
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set +u # avoid failures from any accidental $var expansions in patterns
file="src/components/MailboxThread.vue"
echo "== focusInput usage in MailboxThread.vue =="
rg -n 'focusInput\(' "$file" || true
echo
echo "== Any keydown/shortcut handling in MailboxThread.vue around this feature =="
rg -n 'keydown|handleSearchShortcut|searchMessages|metaKey|ctrlKey|key\s*===\s*[\'"]f[\'"]' "$file" || true
echo
echo "== Repo-wide: other Ctrl/Cmd+F handlers that focus searchMessages/focusInput =="
rg -n 'focusInput\(|handleSearchShortcut|searchMessages|metaKey|ctrlKey|key\s*===\s*[\'"]f[\'"]' src || trueRepository: nextcloud/mail
Length of output: 316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set +u
file="src/components/MailboxThread.vue"
echo "== focusInput usage in MailboxThread.vue =="
rg -n "focusInput\(" "$file" || true
echo
echo "== searchMessages ref usage in MailboxThread.vue =="
rg -n "searchMessages" "$file" || true
echo
echo "== metaKey usage in MailboxThread.vue =="
rg -n "metaKey" "$file" || true
echo
echo "== ctrlKey usage in MailboxThread.vue =="
rg -n "ctrlKey" "$file" || true
echo
echo "== key check for f in MailboxThread.vue =="
rg -n "key\s*===\s*['\"]f['\"]" "$file" || true
echo
echo "== Repo-wide: any other metaKey/ctrlKey + key f shortcut handlers =="
rg -n "metaKey|ctrlKey" src || trueRepository: nextcloud/mail
Length of output: 967
Handle Ctrl+F as well as Cmd+F so message search focus works on Windows/Linux
src/components/MailboxThread.vue focuses this.$refs.searchMessages?.focusInput() only when event.key === 'f' && event.metaKey, so Ctrl+F on Windows/Linux will fall back to the browser’s native find dialog instead of focusing the mail search.
🔧 Handle both modifier keys
handleSearchShortcut(event) {
if (event.target.matches('input, textarea, [contenteditable]')) {
return
}
- if (event.key === 'f' && event.metaKey) {
+ if (event.key === 'f' && (event.metaKey || event.ctrlKey)) {
event.preventDefault()
this.$refs.searchMessages?.focusInput()
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handleSearchShortcut(event) { | |
| if (event.target.matches('input, textarea, [contenteditable]')) { | |
| return | |
| } | |
| if (event.key === 'f' && event.metaKey) { | |
| event.preventDefault() | |
| this.$refs.searchMessages?.focusInput() | |
| } | |
| }, | |
| handleSearchShortcut(event) { | |
| if (event.target.matches('input, textarea, [contenteditable]')) { | |
| return | |
| } | |
| if (event.key === 'f' && (event.metaKey || event.ctrlKey)) { | |
| event.preventDefault() | |
| this.$refs.searchMessages?.focusInput() | |
| } | |
| }, |
There was a problem hiding this comment.
Can someone using linux or windows tell me if this is needed
There was a problem hiding this comment.
@hamza221, from a purely technical standpoint, the behavior on Windows/Linux is predictable without needing a test:
event.metaKeyis the Command (⌘) key on macOS only. On Windows/Linux it is alwaysfalsewhen Ctrl is pressed.- So as the code stands today, pressing Ctrl+F on Windows/Linux will skip the
ifblock entirely and the browser's native find-in-page dialog will open instead of focusing the mail search input.
Whether that is a problem depends on the intended UX:
- If the goal is for Ctrl+F / Cmd+F to focus the mail's own search bar (consistent across all platforms), then adding
|| event.ctrlKeyis needed. - If Ctrl+F on Windows/Linux is intentionally left to the browser (and only macOS gets the override), then the current code is fine as-is.
Since the PR already mentions that Cmd+F was broken on macOS and the intent seems to be restoring search-focus parity, it's likely || event.ctrlKey would be needed for Windows/Linux too — but that's a product decision you or the maintainers can confirm.
|
I tested it on linux and these 2 dont work, backspace doesnt delete the msg, and the cmd f doesnt focus on search field.
|
3 participants
fix #12976
Needs testing on linux and/or windows to avoid regressions
Summary by CodeRabbit
New Features
Improvements