fix: resolve Tab key focus navigation issues in notification center #1369
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes Tab key focus navigation issues in the notification center by implementing manual keyboard focus control. The changes replace the default QML focus behavior with custom Tab/Shift+Tab handling to prevent focus jumping and ensure proper cycling between header buttons and notification items.
Key changes include:
- Added focus cycling signals (
gotoNextItem,gotoPrevItem,gotoFirstNotify,gotoLastNotify) throughout the component hierarchy - Implemented helper functions (
focusFirstButton,focusLastButton,focusItemAtIndex) for programmatic focus management - Disabled
activeFocusOnTabon interactive elements and added customKeys.onTabPressedandKeys.onBacktabPressedhandlers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| NotifyItemContent.qml | Adds focus navigation signals, helper functions for focusing first/last buttons, custom Tab handlers for close button, and integrates action button navigation |
| NotifyAction.qml | Implements focus helpers and custom Tab/Shift+Tab handlers for action buttons with proper navigation between buttons and signals to parent |
| OverlapNotify.qml | Forwards focus navigation signals between NotifyItemContent and parent, exposes focusFirstButton helper |
| NotifyViewDelegate.qml | Implements custom Tab/Shift+Tab handlers for all three delegate types (group, normal, overlap) with navigation logic and focus state management |
| NotifyView.qml | Adds signal forwarding functions and focusItemAtIndex/focusLastItem helpers with retry logic for delegate creation |
| NotifyHeader.qml | Implements focus scope with Tab/Shift+Tab handlers for header buttons (collapse, more, clear all) and cycling to/from notify items |
| NotifyCenter.qml | Connects header and view focus navigation signals to enable cycling between components |
| NormalNotify.qml | Forwards focus navigation signals from NotifyItemContent to parent and exposes focusFirstButton helper |
| GroupNotify.qml | Adds focus navigation signals and Tab/Shift+Tab handlers for group buttons (fold, more, clear all) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| focus: true | ||
| anchors.right: parent.right | ||
| active: !(root.strongInteractive && root.actions.length > 0) && (root.closeVisible || closePlaceHolder.hovered || activeFocus) | ||
| // 在鼠标悬停或通知项获得焦点时显示 |
There was a problem hiding this comment.
Chinese comment should be replaced with English for consistency. The comment "在鼠标悬停或通知项获得焦点时显示" should be translated to English, such as "Show when mouse hovers or notification item has focus".
| // 在鼠标悬停或通知项获得焦点时显示 | |
| // Show when mouse hovers or notification item has focus |
| event.accepted = true | ||
| return | ||
| } | ||
| Qt.callLater(function() { |
There was a problem hiding this comment.
Inconsistent indentation: this line has extra leading spaces compared to the surrounding code. It should align with the if statement on line 213.
| Qt.callLater(function() { | |
| Qt.callLater(function() { |
| id: groupClearBtn | ||
| Layout.alignment: Qt.AlignRight | ||
| activeFocusOnTab: false | ||
| forcusBorderVisible: activeFocus |
There was a problem hiding this comment.
Typo in property name: "forcusBorderVisible" should be "focusBorderVisible" (missing 'o').
| Qt.callLater(function() { tryFocusClear(retries - 1) }) | ||
| } | ||
| } | ||
| Qt.callLater(function() { tryFocusClear(5) }) |
There was a problem hiding this comment.
The magic number 5 is used for retry attempts without explanation. Consider defining this as a named constant at the component or module level (e.g., readonly property int maxFocusRetries: 5) to improve code readability and maintainability.
| padding: 2 | ||
| forcusBorderVisible: visualFocus | ||
| activeFocusOnTab: false | ||
| forcusBorderVisible: activeFocus |
There was a problem hiding this comment.
Typo in property name: "forcusBorderVisible" should be "focusBorderVisible" (missing 'o').
| forcusBorderVisible: activeFocus | |
| focusBorderVisible: activeFocus |
| visible: !notifyModel.collapse | ||
| Layout.alignment: Qt.AlignRight | ||
| activeFocusOnTab: false | ||
| forcusBorderVisible: activeFocus |
There was a problem hiding this comment.
Typo in property name: "forcusBorderVisible" should be "focusBorderVisible" (missing 'o').
| forcusBorderVisible: activeFocus | |
| focusBorderVisible: activeFocus |
| id: clearAllBtn | ||
| objectName: "closeAllNotify" | ||
| activeFocusOnTab: false | ||
| forcusBorderVisible: activeFocus |
There was a problem hiding this comment.
Typo in property name: "forcusBorderVisible" should be "focusBorderVisible" (missing 'o').
| icon.name: "clean-alone" | ||
| text: qsTr("Clean All") | ||
| activeFocusOnTab: false | ||
| forcusBorderVisible: activeFocus |
There was a problem hiding this comment.
Typo in property name: "forcusBorderVisible" should be "focusBorderVisible" (missing 'o').
| forcusBorderVisible: activeFocus | |
| focusBorderVisible: activeFocus |
| Keys.onBacktabPressed: function(event) { | ||
| if (index > 0) { | ||
| root.view.currentIndex = index - 1 | ||
| root.view.positionViewAtIndex(index - 1, ListView.Contain) | ||
| Qt.callLater(function() { | ||
| let prevItem = root.view.itemAtIndex(index - 1) | ||
| if (prevItem) prevItem.forceActiveFocus() | ||
| }) | ||
| event.accepted = true | ||
| } else { | ||
| root.view.gotoHeaderLast() | ||
| event.accepted = true | ||
| } | ||
| } |
There was a problem hiding this comment.
This navigation logic is duplicated across groupNotify (lines 36-43), normalNotify (lines 121-127), and overlapNotify (lines 231-237). Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
| Qt.callLater(function() { tryFocus(retries - 1) }) | ||
| } | ||
| } | ||
| Qt.callLater(function() { tryFocus(5) }) |
There was a problem hiding this comment.
The magic number 5 is used for retry attempts without explanation. Consider defining this as a named constant at the component or module level (e.g., readonly property int maxFocusRetries: 5) to improve code readability and maintainability.
wyu71
force-pushed
the
master
branch
2 times, most recently
from
039ab54 to
31d98d4
Compare
deepin pr auto review我来对这个代码diff进行审查。这个改动主要涉及通知中心中焦点导航和Tab键循环的实现。让我从几个方面进行分析：
// 在NotifyView.qml中
readonly property int maxFocusRetries: 5 // 添加注释说明重试次数的考虑
// 改进focusItemAtIndex函数
function focusItemAtIndex(idx) {
if (idx < 0 || idx >= view.count) return false
view.currentIndex = idx
view.positionViewAtIndex(idx, ListView.Contain)
function tryFocus(retries) {
let item = view.itemAtIndex(idx)
if (item && item.enabled) { // 添加enabled检查
item.forceActiveFocus()
} else if (retries > 0) {
Qt.callLater(function() {
tryFocus(retries - 1)
}, 100) // 添加重试间隔
}
}
Qt.callLater(function() { tryFocus(root.maxFocusRetries) })
return true
}
// 在NotifyItemContent.qml中
function focusFirstButton() {
// 添加组件有效性检查
if (actionLoader.item && actionLoader.item.enabled) {
actionLoader.item.focusFirstButton()
return true
}
// ... 其余代码
}
// 添加公共焦点管理组件
component FocusManager : QtObject {
function navigateToNextItem(currentIndex, itemCount, view) {
if (currentIndex < itemCount - 1) {
view.currentIndex = currentIndex + 1
view.positionViewAtIndex(currentIndex + 1, ListView.Contain)
Qt.callLater(function() {
let nextItem = view.itemAtIndex(currentIndex + 1)
if (nextItem && nextItem.enabled) {
nextItem.forceActiveFocus()
}
})
return true
}
return false
}
}
总的来说，这个改动实现了良好的焦点导航功能，但在代码复用、错误处理和性能优化方面还有改进空间。建议进行上述改进以提高代码质量和可维护性。 |
- Implemented manual focus control for Tab/Shift+Tab between header and notify items - Added focus cycling signals and helper functions for proper keyboard navigation - Disabled default activeFocusOnTab to prevent focus jumping issues Log: resolve Tab key focus navigation issues in notification center pms: BUG-339893 pms: BUG-339891
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| onGotoNextItem: root.navigateToNextItem(index) | ||
| onGotoPrevItem: groupNotify.focusFirstButton() |
There was a problem hiding this comment.
This creates a focus loop. When Shift+Tab is pressed on the first button (foldBtn) in GroupNotify, it emits gotoPrevItem signal (line 56 in GroupNotify.qml). This handler then calls focusFirstButton which focuses foldBtn again, preventing navigation to the previous item. This should be consistent with NormalNotify and OverlapNotify which use forceActiveFocus on the item itself, allowing the delegate's Keys.onBacktabPressed handler to properly navigate to the previous item.
| Keys.onBacktabPressed: function(event) { | ||
| overlapNotify.gotoPrevItem() | ||
| event.accepted = true |
There was a problem hiding this comment.
The Shift+Tab handler for the overlap notification's "Clean All" button doesn't check for action buttons before calling gotoPrevItem. This is inconsistent with the standard close button behavior in NotifyItemContent.qml (lines 116-123) which checks if actionLoader.item exists and focuses the last action button before navigating to the previous item. This could cause the action buttons to be skipped during keyboard navigation.
| icon.name: "clean-group" | ||
| text: qsTr("Clear All") | ||
| Keys.onTabPressed: function(event) { | ||
| groupClearBtn.focus = false // Clear focus before signal to prevent focus state residue |
There was a problem hiding this comment.
Setting focus to false directly on a control before triggering navigation may not properly clear the focus state. Consider using Qt.callLater to ensure the focus state is cleared after the event is processed, similar to how other navigation code uses Qt.callLater for focus operations.
| // Focus first interactive button (action buttons first, then X button) | ||
| function focusFirstButton() { | ||
| if (actionLoader.item && actionLoader.item.enabled) { | ||
| actionLoader.item.focusFirstButton() | ||
| return true | ||
| } | ||
| if (clearLoader.item && clearLoader.item.enabled) { | ||
| clearLoader.item.forceActiveFocus() | ||
| return true | ||
| } | ||
| // Retry if clearLoader not yet created | ||
| function tryFocusClear(retries) { | ||
| if (clearLoader.item && clearLoader.item.enabled) { | ||
| clearLoader.item.forceActiveFocus() | ||
| } else if (retries > 0) { | ||
| Qt.callLater(function() { tryFocusClear(retries - 1) }) | ||
| } | ||
| } | ||
| Qt.callLater(function() { tryFocusClear(root.maxFocusRetries) }) |
There was a problem hiding this comment.
The focusFirstButton function always returns true even when no button can be focused. This could cause issues for the caller who relies on the return value to determine if focus was successfully set. Consider returning false when neither actionLoader nor clearLoader have items and the retry logic is triggered.
| // Focus notify item at specified index with retry logic for delegate creation | ||
| function focusItemAtIndex(idx) { | ||
| if (idx < 0 || idx >= view.count) return false | ||
| view.currentIndex = idx | ||
| view.positionViewAtIndex(idx, ListView.Contain) | ||
| function tryFocus(retries) { | ||
| let item = view.itemAtIndex(idx) | ||
| if (item && item.enabled) { | ||
| item.forceActiveFocus() | ||
| } else if (retries > 0) { | ||
| Qt.callLater(function() { tryFocus(retries - 1) }) | ||
| } | ||
| } | ||
| Qt.callLater(function() { tryFocus(root.maxFocusRetries) }) | ||
| return true |
There was a problem hiding this comment.
The focusItemAtIndex function returns true even when the item cannot be found and retry logic is triggered. This doesn't accurately reflect whether focus was successfully set. Consider tracking whether the retry succeeded and updating the return value accordingly, or document that the return value only indicates whether the attempt was initiated.
| view.positionViewAtIndex(idx, ListView.Contain) | ||
| function tryFocus(retries) { | ||
| let item = view.itemAtIndex(idx) | ||
| if (item && item.enabled) { | ||
| item.forceActiveFocus() | ||
| } else if (retries > 0) { | ||
| Qt.callLater(function() { tryFocus(retries - 1) }) | ||
| } |
There was a problem hiding this comment.
The nested function tryFocus is defined inside focusItemAtIndex but may cause confusion about scope. Consider extracting this as a separate function or using a more conventional pattern to make the retry logic clearer and more maintainable.
| onGotoFirstNotify: { | ||
| if (view.viewCount === 0 || !view.focusItemAtIndex(0)) header.focusFirstButton() |
There was a problem hiding this comment.
The condition relies on focusItemAtIndex returning false when it can't focus, but that function always returns true when the retry logic is initiated. This means if view.viewCount is greater than 0 but the item at index 0 doesn't exist yet (delegate not created), the header won't be focused even though the retry might fail. Consider checking view.viewCount first and only calling focusItemAtIndex if count is greater than 0, or fix focusItemAtIndex to return a meaningful value.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, wyu71 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
3 participants
Log: resolve Tab key focus navigation issues in notification center
pms: BUG-339893
pms: BUG-339891