fix(notification): hide close button when mouse leaves center after e…#1427
fix(notification): hide close button when mouse leaves center after e…#1427re2zero wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: re2zero 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds hover tracking for the entire notification center and threads that state through the view and notification items so that the close button only remains visible when either hovered or focused while the mouse is still within the notification center area. Sequence diagram for mouse leave handling and close button visibilitysequenceDiagram
actor User
participant Mouse as MousePointer
participant NotifyCenter
participant HoverHandler as centerHoverHandler
participant NotifyView
participant NormalNotify
participant Impl as ImplComponent
User->>Mouse: Move inside notification center
Mouse->>HoverHandler: hover enter
HoverHandler-->>NotifyCenter: hovered = true
NotifyCenter->>NotifyView: centerHovered = mouseInCenter(true)
NotifyView->>NormalNotify: centerHovered = true
NormalNotify->>Impl: parentHovered = impl.hovered || (activeFocus && centerHovered)
Impl-->>User: Close button visible
User->>Mouse: Move outside notification center
Mouse->>HoverHandler: hover leave
HoverHandler-->>NotifyCenter: hovered = false
NotifyCenter->>NotifyView: centerHovered = mouseInCenter(false)
NotifyView->>NormalNotify: centerHovered = false
NormalNotify->>Impl: recompute parentHovered
Impl-->>User: Close button hidden when only focus is active
Class diagram for updated notification center hover trackingclassDiagram
class NotifyCenter {
int maxViewHeight
int stagingViewCount
int viewCount
bool mouseInCenter
gotoStagingLast()
gotoStagingFirst()
}
class HoverHandler_center {
bool hovered
}
class NotifyView {
alias notifyModel
alias viewPanelShown
real viewHeight
int viewCount
bool centerHovered
gotoHeaderFirst()
gotoHeaderLast()
}
class NotifyViewDelegate {
NotifyModel notifyModel
Item view
bool centerHovered
setting(var pos, var params)
}
class NormalNotify {
int implicitWidth
int implicitHeight
bool centerHovered
gotoNextItem()
gotoPrevItem()
}
class ImplComponent {
bool hovered
bool strongInteractive
string contentIcon
int contentRowCount
bool parentHovered
}
NotifyCenter --> HoverHandler_center : contains
NotifyCenter --> NotifyView : contains
NotifyView --> NotifyViewDelegate : uses
NotifyViewDelegate --> NormalNotify : selects_as_delegate
NotifyView --> NotifyModel : uses
NotifyView --> Item : view
NormalNotify --> ImplComponent : contains
NotifyCenter : mouseInCenter = centerHoverHandler.hovered
NotifyView : centerHovered from NotifyCenter.mouseInCenter
NotifyViewDelegate : centerHovered required
NormalNotify : centerHovered from NotifyView.centerHovered
ImplComponent : parentHovered = impl.hovered || (root.activeFocus && root.centerHovered)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The same concept is referred to as
mouseInCenterinNotifyCenter.qmlandcenterHoveredin other components; consider standardizing on a single property name to make the data flow easier to follow. - You now gate
parentHoveredonroot.activeFocus && root.centerHovered; if keyboard focus alone (with the mouse outside the center) should still visually indicate hover/close affordances for accessibility, you may want to revisit this condition or add a separate visual state for keyboard focus.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The same concept is referred to as `mouseInCenter` in `NotifyCenter.qml` and `centerHovered` in other components; consider standardizing on a single property name to make the data flow easier to follow.
- You now gate `parentHovered` on `root.activeFocus && root.centerHovered`; if keyboard focus alone (with the mouse outside the center) should still visually indicate hover/close affordances for accessibility, you may want to revisit this condition or add a separate visual state for keyboard focus.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Refactor Tab/Shift+Tab navigation logic to properly handle disabled buttons and unify focus management across notification items. 改进通知中心键盘导航逻辑，正确处理禁用按钮的焦点跳转， 统一通知项和按钮的焦点管理。 Log: 改进通知中心键盘导航 PMS: BUG-347241 Influence: 使用键盘 Tab/Shift+Tab 浏览通知时，现在会正确跳过禁用的按钮，焦点顺序更符合预期；关闭按钮仅在键盘导航或鼠标悬停时显示，界面更简洁。
deepin pr auto review这份代码修改主要是针对通知中心(Notification Center)中的键盘导航逻辑进行了重构，改进了Tab键和Shift+Tab键在通知项之间的切换行为，并优化了焦点管理和关闭按钮的显示逻辑。下面是对代码的详细审查和改进意见： 1. 语法逻辑审查优点：
潜在问题：
2. 代码质量审查优点：
改进建议：
3. 代码性能审查优点：
改进建议：
4. 代码安全审查优点：
改进建议：
5. 其他建议
总结这次修改整体上改进了通知中心的键盘导航体验，代码质量较高。主要需要关注的是：
建议在合并前进行全面的键盘导航测试，特别是针对各种边界情况和不同通知类型的组合场景。 |
There was a problem hiding this comment.
Pull request overview
This PR aims to fix a bug where the close button remains visible after expanding a notification when the mouse leaves the notification center area. The fix introduces mouse position tracking through a centerHovered property and keyboard navigation state tracking via shouldShowClose to control when the close button should be displayed.
Changes:
- Refactored Tab/Backtab navigation to use parent Control handlers instead of item-level handlers
- Added
centerHoveredandshouldShowCloseproperties to track mouse and keyboard focus states - Improved focus management functions to properly handle disabled buttons and return success/failure status
- Introduced
focusFirstActionOnly()to avoid circular focus loops in OverlapNotify
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| NotifyItemContent.qml | Refactored focus navigation logic with improved return values and added focusFirstActionOnly() method |
| NotifyAction.qml | Enhanced button focus functions to handle disabled actions and return boolean success indicators |
| OverlapNotify.qml | Added centerHovered/shouldShowClose properties and external clearButton reference for focus management |
| NotifyViewDelegate.qml | Simplified delegate by removing item-level Tab handlers and passing centerHovered to child components |
| NormalNotify.qml | Added centerHovered/shouldShowClose properties and moved Tab handlers to internal Control |
| GroupNotify.qml | Added focusLastButton() function and moved Tab handlers to internal Control |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to focus first action button | ||
| if (overlapNotify.focusFirstButton()) { |
There was a problem hiding this comment.
This creates a circular focus issue. When Tab is pressed on the clear button, it calls overlapNotify.focusFirstButton(), which will try to focus the clear button itself first (line 33-35 in OverlapNotify.qml), since clearButton is set to reference this same button (line 200). This means pressing Tab on the clear button will either focus itself again or do nothing. The Tab handler should instead call overlapNotify.focusFirstActionOnly() to skip the clear button and focus action buttons, or directly call gotoNextItem() if there are no action buttons.
| // Try to focus first action button | |
| if (overlapNotify.focusFirstButton()) { | |
| // Try to focus first action button (skip clear button itself) | |
| if (overlapNotify.focusFirstActionOnly && overlapNotify.focusFirstActionOnly()) { |
|
|
||
| Keys.onTabPressed: function(event) { | ||
| // Mark that this item got focus from Tab navigation | ||
| root.shouldShowClose = true |
There was a problem hiding this comment.
The shouldShowClose property is set to true here but is never reset to false anywhere in the codebase. This means once keyboard navigation is used (Tab/Shift+Tab), the close button will remain visible permanently even when the mouse leaves and the item loses focus. This contradicts the PR's stated goal of hiding the close button when the mouse leaves the notification center. Consider adding a mechanism to reset shouldShowClose to false when the item loses focus and the mouse is not hovering (e.g., using onActiveFocusChanged).
| Control { | ||
| id: impl | ||
| anchors.fill: parent | ||
| focus: true |
There was a problem hiding this comment.
The shouldShowClose property is set to true here but is never reset to false anywhere in the codebase. This means once keyboard navigation is used (Tab/Shift+Tab), the close button will remain visible permanently even when the mouse leaves and the item loses focus. This contradicts the PR's stated goal of hiding the close button when the mouse leaves the notification center. Consider adding a mechanism to reset shouldShowClose to false when the item loses focus and the mouse is not hovering (e.g., using onActiveFocusChanged).
| focus: true | |
| focus: true | |
| onActiveFocusChanged: { | |
| if (!activeFocus && !root.centerHovered) { | |
| root.shouldShowClose = false | |
| } | |
| } |
| // Try to focus first action button | ||
| if (overlapNotify.focusFirstButton()) { | ||
| event.accepted = true | ||
| return | ||
| } | ||
| // No enabled action buttons, go to next item |
There was a problem hiding this comment.
This comment is misleading. The code actually tries to focus all buttons in the notification (starting with the clear button itself), not just action buttons. The comment should be updated to reflect what actually happens, or the code should be changed to match the comment (e.g., by calling overlapNotify.notifyContent.focusFirstActionOnly() to skip the clear button).
| // Try to focus first action button | |
| if (overlapNotify.focusFirstButton()) { | |
| event.accepted = true | |
| return | |
| } | |
| // No enabled action buttons, go to next item | |
| // Try to focus the next enabled button in this notification | |
| if (overlapNotify.focusFirstButton()) { | |
| event.accepted = true | |
| return | |
| } | |
| // No enabled buttons to focus, go to next item |
| width: NotifyStyle.contentItem.width | ||
| activeFocusOnTab: false | ||
| z: index | ||
| centerHovered: root.centerHovered // Pass mouse hover state |
There was a problem hiding this comment.
The property 'centerHovered' is referenced here but is never defined in the DelegateChooser. This will cause a runtime error. The 'centerHovered' property needs to be added to NotifyViewDelegate.qml (the DelegateChooser root) and then passed down from wherever NotifyViewDelegate is instantiated (likely NotifyView.qml). According to the PR description, this property should track the mouse hover state of the notification center to fix the close button visibility issue.
| width: NotifyStyle.contentItem.width | ||
| activeFocusOnTab: false | ||
| z: index | ||
| centerHovered: root.centerHovered // Pass mouse hover state |
There was a problem hiding this comment.
The property 'centerHovered' is referenced here but is never defined in the DelegateChooser. This will cause a runtime error. The 'centerHovered' property needs to be added to NotifyViewDelegate.qml (the DelegateChooser root) and then passed down from wherever NotifyViewDelegate is instantiated (likely NotifyView.qml). According to the PR description, this property should track the mouse hover state of the notification center to fix the close button visibility issue.
|
TAG Bot New tag: 2.0.29 |
| property int count: 1 | ||
| readonly property int overlapItemRadius: 12 | ||
| property bool enableDismissed: true | ||
| property bool centerHovered: false // Mouse hover state from notification center |
There was a problem hiding this comment.
centerHovered 这个变量是不是没用到了，
3 participants
…xpand
Added mouse position tracking to notification center to prevent close button from staying visible after expand when mouse moves outside the center area.
在通知中心添加鼠标位置追踪，解决展开后鼠标移出中心时关闭按钮不隐藏的问题。
Log: 修复展开通知后鼠标移出中心关闭按钮不隐藏的问题
PMS: BUG-347241
Influence: 修复后，展开折叠通知并将鼠标移出通知中心区域时，展开的第一条信息右上角的关闭按钮会正确隐藏，提升用户体验。同时保留所有Tab键导航和键盘焦点功能。
Summary by Sourcery
Bug Fixes: