Skip to content

Commit 9fcbc6a

Browse files
icyleaficyleaf
committed
fix(ui): prevent background sync from stealing focus when modal overlays are active
- Unify modal overlay visibility tracking into hasActiveModal - Inhibit search field focus restoration during authStatusProc, vaultSyncProc, and vaultListProc callbacks while any modal is active - Guard global Shortcuts and imperative Keys.onPressed handlers with hasActiveModal to prevent background shortcut leakage during modal editing - Ensure restoreSearchFocus on modal close only executes when no other modal remains active - Add regression test in tests/tst_modal_focus.qml Fixes #212
1 parent 98ec76f commit 9fcbc6a

2 files changed

Lines changed: 153 additions & 34 deletions

File tree

‎OmarchyBitwarden.qml‎

Lines changed: 37 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,7 @@ Item {
303303
property var sshKeyModalItem: null
304304
property bool showPasswordHistoryModal: false
305305
property var activePasswordHistoryItem: null
306+
readonly property bool hasActiveModal: root.showActionPalette || root.showSshKeyModal || root.showPasswordHistoryModal
306307
property var activeAttachmentPreview: null
307308
property string loadingAttachmentId: ""
308309

@@ -349,7 +350,7 @@ Item {
349350

350351
onEffectiveViewChanged: {
351352
Qt.callLater(function() {
352-
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField) {
353+
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.hasActiveModal) {
353354
searchHeader.searchField.forceActiveFocus()
354355
} else if (root.opened && root.effectiveView !== "dependency_check" && authViewComponent && authViewComponent.unlockInput) {
355356
authViewComponent.unlockInput.forceActiveFocus()
@@ -969,19 +970,19 @@ Item {
969970
onShowActionPaletteChanged: {
970971
if (root.showActionPalette) {
971972
root.updateAvailableActions()
972-
} else {
973+
} else if (!root.hasActiveModal) {
973974
root.restoreSearchFocus()
974975
}
975976
}
976977

977978
onShowSshKeyModalChanged: {
978-
if (!root.showSshKeyModal) {
979+
if (!root.showSshKeyModal && !root.hasActiveModal) {
979980
root.restoreSearchFocus()
980981
}
981982
}
982983

983984
onShowPasswordHistoryModalChanged: {
984-
if (!root.showPasswordHistoryModal) {
985+
if (!root.showPasswordHistoryModal && !root.hasActiveModal) {
985986
root.restoreSearchFocus()
986987
}
987988
}
@@ -1883,7 +1884,7 @@ Item {
18831884

18841885
Shortcut {
18851886
sequence: "Ctrl+K"
1886-
enabled: root.opened && root.effectiveView === "search" && !root.showActionPalette && !root.showSshKeyModal
1887+
enabled: root.opened && root.effectiveView === "search" && !root.hasActiveModal
18871888
onActivated: {
18881889
root.actionPaletteIndex = 0
18891890
root.showActionPalette = true
@@ -1892,51 +1893,51 @@ Item {
18921893

18931894
Shortcut {
18941895
sequences: ["Ctrl+Return", "Ctrl+Enter"]
1895-
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal
1896+
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal
18961897
onActivated: root.copyItemTotp(root.selectedItem)
18971898
}
18981899

18991900
Shortcut {
19001901
sequence: "Ctrl+U"
1901-
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal
1902+
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal
19021903
onActivated: root.copyItemUsername(root.selectedItem)
19031904
}
19041905

19051906
Shortcut {
19061907
sequence: "Ctrl+T"
1907-
enabled: root.opened && (root.showPasswordHistoryModal || (root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal))
1908+
enabled: root.opened && (root.showPasswordHistoryModal || (root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal))
19081909
onActivated: root.toggleItemRevealed()
19091910
}
19101911

19111912
Shortcut {
19121913
sequence: "Ctrl+O"
1913-
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal
1914+
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal
19141915
onActivated: root.openFirstWebsite(root.selectedItem)
19151916
}
19161917

19171918
Shortcut {
19181919
sequence: "Ctrl+E"
1919-
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && root.selectedItem.type_name === "ssh_key" && !root.showActionPalette && !root.showSshKeyModal
1920+
enabled: root.opened && root.effectiveView === "search" && root.selectedItem !== null && root.selectedItem.type_name === "ssh_key" && !root.hasActiveModal
19201921
onActivated: {
19211922
root.openSshKeyModal("export", root.selectedItem)
19221923
}
19231924
}
19241925

19251926
Shortcut {
19261927
sequence: "Ctrl+L"
1927-
enabled: root.opened && root.authState.status === "unlocked" && !root.showSshKeyModal
1928+
enabled: root.opened && root.authState.status === "unlocked" && !root.hasActiveModal
19281929
onActivated: root.doLock()
19291930
}
19301931

19311932
Shortcut {
19321933
sequence: "Ctrl+R"
1933-
enabled: root.opened && (root.authState.status === "unlocked" || (root.authState.status === "locked" && root.authState.has_session)) && !root.isBusy && !root.showSshKeyModal
1934+
enabled: root.opened && (root.authState.status === "unlocked" || (root.authState.status === "locked" && root.authState.has_session)) && !root.isBusy && !root.hasActiveModal
19341935
onActivated: root.syncVault(false, true)
19351936
}
19361937

19371938
Shortcut {
19381939
sequence: "Ctrl+,"
1939-
enabled: root.opened && !root.showSshKeyModal
1940+
enabled: root.opened && !root.hasActiveModal
19401941
onActivated: {
19411942
if (settingsView.showReleaseNotes) {
19421943
settingsView.showReleaseNotes = false
@@ -1947,15 +1948,15 @@ Item {
19471948

19481949
Shortcut {
19491950
sequence: "Alt+V"
1950-
enabled: root.opened && root.effectiveView === "search" && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal
1951+
enabled: root.opened && root.effectiveView === "search" && !root.hasActiveModal
19511952
onActivated: {
19521953
if (searchHeader) searchHeader.openVaultScope()
19531954
}
19541955
}
19551956

19561957
Shortcut {
19571958
sequence: "Alt+F"
1958-
enabled: root.opened && root.effectiveView === "search" && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal
1959+
enabled: root.opened && root.effectiveView === "search" && !root.hasActiveModal
19591960
onActivated: {
19601961
if (searchHeader) searchHeader.openFolderScope()
19611962
}
@@ -1994,12 +1995,14 @@ Item {
19941995
Keys.priority: Keys.BeforeItem
19951996
Keys.onPressed: function(event) {
19961997
if (event.modifiers & Qt.ControlModifier && (event.key === Qt.Key_T || (event.text && event.text.toLowerCase() === "t"))) {
1997-
root.toggleItemRevealed()
1998-
event.accepted = true
1999-
return
1998+
if (root.showPasswordHistoryModal || (root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal)) {
1999+
root.toggleItemRevealed()
2000+
event.accepted = true
2001+
return
2002+
}
20002003
}
20012004

2002-
if (event.modifiers & Qt.AltModifier && root.effectiveView === "search") {
2005+
if (event.modifiers & Qt.AltModifier && root.effectiveView === "search" && !root.hasActiveModal) {
20032006
if (event.key === Qt.Key_V) {
20042007
if (searchHeader) searchHeader.openVaultScope()
20052008
event.accepted = true
@@ -2076,40 +2079,40 @@ Item {
20762079
}
20772080
event.accepted = true
20782081
} else if (event.modifiers & Qt.ControlModifier) {
2079-
if ((event.key === Qt.Key_Return || event.key === Qt.Key_Enter) && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2082+
if ((event.key === Qt.Key_Return || event.key === Qt.Key_Enter) && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal) {
20802083
root.copyItemTotp(root.selectedItem)
20812084
event.accepted = true
2082-
} else if (event.key === Qt.Key_U && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2085+
} else if (event.key === Qt.Key_U && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal) {
20832086
root.copyItemUsername(root.selectedItem)
20842087
event.accepted = true
2085-
} else if (event.key === Qt.Key_K && root.effectiveView === "search" && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2088+
} else if (event.key === Qt.Key_K && root.effectiveView === "search" && !root.hasActiveModal) {
20862089
root.actionPaletteIndex = 0
20872090
root.showActionPalette = true
20882091
event.accepted = true
2089-
} else if (event.key === Qt.Key_T && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2092+
} else if (event.key === Qt.Key_T && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal) {
20902093
root.toggleItemRevealed()
20912094
event.accepted = true
2092-
} else if (event.key === Qt.Key_O && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2095+
} else if (event.key === Qt.Key_O && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal) {
20932096
root.openFirstWebsite(root.selectedItem)
20942097
event.accepted = true
2095-
} else if (event.key === Qt.Key_E && root.effectiveView === "search" && root.selectedItem !== null && root.selectedItem.type_name === "ssh_key" && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2098+
} else if (event.key === Qt.Key_E && root.effectiveView === "search" && root.selectedItem !== null && root.selectedItem.type_name === "ssh_key" && !root.hasActiveModal) {
20962099
root.openSshKeyModal("export", root.selectedItem)
20972100
event.accepted = true
2098-
} else if (event.key === Qt.Key_H && root.effectiveView === "search" && root.selectedItem !== null && !root.showActionPalette && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2101+
} else if (event.key === Qt.Key_H && root.effectiveView === "search" && root.selectedItem !== null && !root.hasActiveModal) {
20992102
if (root.selectedItem.login && root.selectedItem.login.password_history && root.selectedItem.login.password_history.length > 0) {
21002103
root.activePasswordHistoryItem = root.selectedItem
21012104
root.showPasswordHistoryModal = true
21022105
} else {
21032106
root.statusMessage = "No password history recorded for this item."
21042107
}
21052108
event.accepted = true
2106-
} else if (event.key === Qt.Key_L && root.authState.status === "unlocked" && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2109+
} else if (event.key === Qt.Key_L && root.authState.status === "unlocked" && !root.hasActiveModal) {
21072110
root.doLock()
21082111
event.accepted = true
2109-
} else if (event.key === Qt.Key_R && (root.authState.status === "unlocked" || (root.authState.status === "locked" && root.authState.has_session)) && !root.isBusy && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2112+
} else if (event.key === Qt.Key_R && (root.authState.status === "unlocked" || (root.authState.status === "locked" && root.authState.has_session)) && !root.isBusy && !root.hasActiveModal) {
21102113
root.syncVault(false, true)
21112114
event.accepted = true
2112-
} else if (event.key === Qt.Key_Comma && !root.showSshKeyModal && !root.showPasswordHistoryModal) {
2115+
} else if (event.key === Qt.Key_Comma && !root.hasActiveModal) {
21132116
root.currentView = (root.effectiveView === "settings") ? "auto" : "settings"
21142117
event.accepted = true
21152118
}
@@ -2128,7 +2131,7 @@ Item {
21282131
SearchHeader {
21292132
id: searchHeader
21302133
visible: root.effectiveView === "search"
2131-
modalsActive: root.showActionPalette || root.showSshKeyModal || root.showPasswordHistoryModal
2134+
modalsActive: root.hasActiveModal
21322135
Layout.preferredHeight: visible ? implicitHeight : 0
21332136
searchQuery: root.searchQuery
21342137
categoryList: root.categoryList
@@ -2724,7 +2727,7 @@ Item {
27242727
root.clearSensitiveState()
27252728
}
27262729
Qt.callLater(function() {
2727-
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.showActionPalette) {
2730+
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.hasActiveModal) {
27282731
if (!searchHeader.searchField.activeFocus) {
27292732
var canRemember = Boolean(root.config && root.config.remember_last_search === true && root.authState && root.authState.status === "unlocked")
27302733
searchHeader.focusSearch(canRemember)
@@ -3100,7 +3103,7 @@ Item {
31003103
root.statusMessage = "Vault synchronized."
31013104
root.loadVaultItems()
31023105
Qt.callLater(function() {
3103-
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.showActionPalette) {
3106+
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.hasActiveModal) {
31043107
searchHeader.searchField.forceActiveFocus()
31053108
}
31063109
})
@@ -3134,7 +3137,7 @@ Item {
31343137
var cleanText = (text || "").trim()
31353138
if (cleanText === root.lastVaultItemsRawText && root.rawVaultItems && root.rawVaultItems.length > 0) {
31363139
Qt.callLater(function() {
3137-
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.showActionPalette) {
3140+
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.hasActiveModal) {
31383141
searchHeader.searchField.forceActiveFocus()
31393142
}
31403143
})
@@ -3161,7 +3164,7 @@ Item {
31613164
}
31623165
root.filterVaultItems(false)
31633166
Qt.callLater(function() {
3164-
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.showActionPalette) {
3167+
if (root.opened && root.effectiveView === "search" && searchHeader && searchHeader.searchField && !root.hasActiveModal) {
31653168
searchHeader.searchField.forceActiveFocus()
31663169
}
31673170
})

‎tests/tst_modal_focus.qml‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import QtQuick
2+
import QtQuick.Controls
3+
4+
Item {
5+
id: testRunner
6+
width: 400
7+
height: 300
8+
9+
property int failures: 0
10+
function check(cond, msg) {
11+
if (!cond) { failures++; console.error("FAIL: " + msg) }
12+
else console.log("PASS: " + msg)
13+
}
14+
15+
// Simulation harness mirroring OmarchyBitwarden properties
16+
property bool opened: true
17+
property string effectiveView: "search"
18+
property var authState: ({ status: "unlocked", has_session: true })
19+
property var config: ({ remember_last_search: false })
20+
property var selectedItem: ({ id: "item1", name: "Sample Item" })
21+
22+
property bool showActionPalette: false
23+
property bool showSshKeyModal: false
24+
property bool showPasswordHistoryModal: false
25+
26+
readonly property bool hasActiveModal: showActionPalette || showSshKeyModal || showPasswordHistoryModal
27+
28+
property int searchFocusRestoredCount: 0
29+
30+
function simulateFocusRestore() {
31+
if (opened && effectiveView === "search" && !hasActiveModal) {
32+
searchFocusRestoredCount++
33+
}
34+
}
35+
36+
// Helper simulating focusRoot.Keys.onPressed handler checks
37+
function canTriggerScopeShortcut() {
38+
return (effectiveView === "search" && !hasActiveModal)
39+
}
40+
41+
function canTriggerToggleRevealed() {
42+
return (showPasswordHistoryModal || (effectiveView === "search" && selectedItem !== null && !hasActiveModal))
43+
}
44+
45+
Component.onCompleted: {
46+
console.log("Running Modal Focus and Background Sync Guard Tests...")
47+
48+
// 1. Initial State: no modals active
49+
check(hasActiveModal === false, "Initial hasActiveModal is false")
50+
check(canTriggerScopeShortcut() === true, "Scope shortcuts (Alt+V / Alt+F) enabled when no modals active")
51+
check(canTriggerToggleRevealed() === true, "Toggle revealed (Ctrl+T) enabled when no modals active")
52+
53+
// 2. SSH Key modal active
54+
showSshKeyModal = true
55+
check(hasActiveModal === true, "hasActiveModal is true when showSshKeyModal is true")
56+
check(canTriggerScopeShortcut() === false, "Scope shortcuts inhibited during SSH Key modal")
57+
check(canTriggerToggleRevealed() === false, "Toggle revealed inhibited during SSH Key modal")
58+
59+
searchFocusRestoredCount = 0
60+
simulateFocusRestore()
61+
check(searchFocusRestoredCount === 0, "Background focus restore is inhibited when SSH Key modal is active")
62+
63+
showSshKeyModal = false
64+
check(hasActiveModal === false, "hasActiveModal is false after closing SSH Key modal")
65+
check(canTriggerScopeShortcut() === true, "Scope shortcuts re-enabled after closing SSH Key modal")
66+
67+
// 3. Password History modal active
68+
showPasswordHistoryModal = true
69+
check(hasActiveModal === true, "hasActiveModal is true when showPasswordHistoryModal is true")
70+
check(canTriggerScopeShortcut() === false, "Scope shortcuts inhibited during Password History modal")
71+
check(canTriggerToggleRevealed() === true, "Toggle revealed explicitly permitted during Password History modal")
72+
73+
searchFocusRestoredCount = 0
74+
simulateFocusRestore()
75+
check(searchFocusRestoredCount === 0, "Background focus restore is inhibited when Password History modal is active")
76+
77+
showPasswordHistoryModal = false
78+
check(hasActiveModal === false, "hasActiveModal is false after closing Password History modal")
79+
80+
// 4. Action Palette active
81+
showActionPalette = true
82+
check(hasActiveModal === true, "hasActiveModal is true when showActionPalette is true")
83+
check(canTriggerScopeShortcut() === false, "Scope shortcuts inhibited during Action Palette")
84+
check(canTriggerToggleRevealed() === false, "Toggle revealed inhibited during Action Palette")
85+
86+
searchFocusRestoredCount = 0
87+
simulateFocusRestore()
88+
check(searchFocusRestoredCount === 0, "Background focus restore is inhibited when Action Palette is active")
89+
90+
showActionPalette = false
91+
check(hasActiveModal === false, "hasActiveModal is false after closing Action Palette")
92+
93+
// 5. No modals: focus restore executes
94+
searchFocusRestoredCount = 0
95+
simulateFocusRestore()
96+
check(searchFocusRestoredCount === 1, "Background focus restore proceeds when no modals are active")
97+
98+
// 6. Test legacy unguarded behavior vs guarded behavior
99+
// The bug was: condition only checked !showActionPalette
100+
var legacyGuardSshModal = (!showActionPalette) // would be true even if showSshKeyModal is true!
101+
showSshKeyModal = true
102+
check(legacyGuardSshModal === true, "Legacy guard would erroneously pass during SSH Key modal")
103+
check(!hasActiveModal === false, "New unified guard correctly blocks focus steal during SSH Key modal")
104+
105+
showSshKeyModal = false
106+
showPasswordHistoryModal = true
107+
var legacyGuardHistoryModal = (!showActionPalette) // would be true even if showPasswordHistoryModal is true!
108+
check(legacyGuardHistoryModal === true, "Legacy guard would erroneously pass during Password History modal")
109+
check(!hasActiveModal === false, "New unified guard correctly blocks focus steal during Password History modal")
110+
111+
showPasswordHistoryModal = false
112+
113+
console.log("Modal Focus Tests Completed: " + (failures === 0 ? "ALL PASSED" : failures + " FAILURES"))
114+
Qt.exit(failures === 0 ? 0 : 1)
115+
}
116+
}

0 commit comments

Comments
 (0)