fix: improve notification close button visibility logic#1525
Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom Mar 24, 2026
Merged
fix: improve notification close button visibility logic#1525deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
1. Added `focusedByNavigation` property to GroupNotify, NormalNotify, and OverlapNotify components 2. Modified close button visibility condition to only show when focused via keyboard navigation, not programmatic focus 3. Updated NotifyView and NotifyViewDelegate to set `focusedByNavigation=true` when navigating between items 4. Reset `focusedByNavigation` to false when item loses active focus The issue was that close buttons were incorrectly showing when notifications received programmatic focus (e.g., during initialization or other non-navigation scenarios). This fix ensures close buttons only appear during actual keyboard navigation, improving the user experience by preventing visual clutter when not needed. Log: Fixed notification close button visibility during keyboard navigation Influence: 1. Test keyboard navigation between notification items using arrow keys 2. Verify close buttons appear only during keyboard navigation, not during initial load 3. Test mouse hover behavior - close buttons should appear on hover 4. Verify focus transitions between notifications work correctly 5. Test that programmatic focus changes don't trigger close button display 6. Check accessibility navigation with screen readers fix: 改进通知关闭按钮可见性逻辑 1. 在 GroupNotify、NormalNotify 和 OverlapNotify 组件中添加 `focusedByNavigation` 属性 2. 修改关闭按钮可见性条件，仅在通过键盘导航获得焦点时显示，而不是程序化 焦点 3. 更新 NotifyView 和 NotifyViewDelegate，在导航到项目时设置 `focusedByNavigation=true` 4. 当项目失去活动焦点时，将 `focusedByNavigation` 重置为 false 问题在于当通知获得程序化焦点时（例如初始化期间或其他非导航场景），关闭按 钮会错误地显示。此修复确保关闭按钮仅在真正的键盘导航期间出现，通过在不必 要时防止视觉混乱来改善用户体验。 Log: 修复了键盘导航期间通知关闭按钮的可见性问题 Influence: 1. 使用方向键测试通知项目之间的键盘导航 2. 验证关闭按钮仅在键盘导航期间出现，而不是在初始加载时 3. 测试鼠标悬停行为 - 悬停时应显示关闭按钮 4. 验证通知之间的焦点转换正常工作 5. 测试程序化焦点更改不会触发关闭按钮显示 6. 检查屏幕阅读器的辅助功能导航
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts notification components so the close button only appears when a notification gains focus via explicit keyboard navigation or hover, not when focus is set programmatically, by tracking a new focusedByNavigation flag on items and managing it in NotifyView and NotifyViewDelegate. Sequence diagram for notification focus navigation and close button visibilitysequenceDiagram
actor User
participant NotifyView
participant NotifyViewDelegate
participant NotifyItem as NormalNotify
participant CloseButton
User->>NotifyView: Press arrow key
NotifyView->>NotifyViewDelegate: handleNavigation(currentIndex)
NotifyViewDelegate->>NotifyView: view.itemAtIndex(nextIndex)
NotifyView-->>NotifyViewDelegate: nextItem
NotifyViewDelegate->>NotifyItem: focusedByNavigation = true
NotifyViewDelegate->>NotifyItem: resetFocus()
NotifyViewDelegate->>NotifyItem: forceActiveFocus()
NotifyItem->>NotifyItem: activeFocus becomes true
NotifyItem->>CloseButton: parentHovered = hovered OR (activeFocus AND focusedByNavigation)
CloseButton-->>User: Close button visible
User->>NotifyItem: Move focus away
NotifyItem->>NotifyItem: activeFocus becomes false
NotifyItem->>NotifyItem: focusedByNavigation = false
NotifyItem->>CloseButton: parentHovered recomputed
CloseButton-->>User: Close button hidden
Class diagram for updated notification components and focus handlingclassDiagram
class NotifyItem {
}
class NormalNotify {
bool focusedByNavigation
gotoNextItem()
gotoPrevItem()
}
class OverlapNotify {
bool focusedByNavigation
var removedCallback
notifyContent
expand()
gotoNextItem()
gotoPrevItem()
}
class GroupNotify {
bool focusedByNavigation
collapse()
gotoNextItem()
gotoPrevItem()
}
class NotifyView {
tryFocus(retries)
}
class NotifyViewDelegate {
handleNext(currentIndex)
handlePrev(currentIndex)
}
NotifyItem <|-- NormalNotify
NotifyItem <|-- OverlapNotify
NotifyItem <|-- GroupNotify
NotifyView "1" o-- "many" NotifyItem : contains
NotifyViewDelegate "1" o-- "1" NotifyView : controls_navigation
NotifyViewDelegate ..> NotifyItem : sets_focusedByNavigation
NotifyView ..> NotifyItem : sets_focusedByNavigation_and_focus
Flow diagram for close button visibility logicflowchart TD
A[Start] --> B{impl.hovered?}
B -->|Yes| C[Show close button]
B -->|No| D{root.activeFocus?}
D -->|No| E[Hide close button]
D -->|Yes| F{root.focusedByNavigation?}
F -->|Yes| C[Show close button]
F -->|No| E[Hide close button]
C --> G[End]
E --> G[End]
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 found 2 issues, and left some high level feedback:
- The
focusedByNavigationproperty andonActiveFocusChangedhandler are duplicated acrossNormalNotify,OverlapNotify, andGroupNotify; consider moving this state and reset logic intoNotifyItem(or a shared base) to avoid repetition and keep behavior consistent across all notify types. - Now that close-button visibility depends on
focusedByNavigation, make sure all keyboard navigation paths (not just next/prev andtryFocus, e.g. page up/down, home/end, list refocus, etc.) set this flag appropriately so the close button behavior remains consistent for all navigation affordances.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `focusedByNavigation` property and `onActiveFocusChanged` handler are duplicated across `NormalNotify`, `OverlapNotify`, and `GroupNotify`; consider moving this state and reset logic into `NotifyItem` (or a shared base) to avoid repetition and keep behavior consistent across all notify types.
- Now that close-button visibility depends on `focusedByNavigation`, make sure all keyboard navigation paths (not just next/prev and `tryFocus`, e.g. page up/down, home/end, list refocus, etc.) set this flag appropriately so the close button behavior remains consistent for all navigation affordances.
## Individual Comments
### Comment 1
<location path="panels/notification/center/NormalNotify.qml" line_range="17-18" />
<code_context>
id: root
implicitWidth: impl.implicitWidth
implicitHeight: impl.implicitHeight
+ property bool focusedByNavigation: false
+ onActiveFocusChanged: if (!activeFocus) focusedByNavigation = false
signal collapse()
</code_context>
<issue_to_address>
**suggestion:** Consider centralizing the focusedByNavigation behavior to avoid repetition across notify item types.
`focusedByNavigation` and the `onActiveFocusChanged` handler are now repeated in `NormalNotify`, `OverlapNotify`, and `GroupNotify`. Centralizing this (e.g., in `NotifyItem` or a shared mixin) would keep the behavior defined in one place and avoid future inconsistencies, especially when adding new delegate types.
</issue_to_address>
### Comment 2
<location path="panels/notification/center/NotifyViewDelegate.qml" line_range="27-31" />
<code_context>
Qt.callLater(function() {
let nextItem = view.itemAtIndex(currentIndex + 1)
if (nextItem && nextItem.enabled) {
+ nextItem.focusedByNavigation = true
nextItem.resetFocus()
nextItem.forceActiveFocus()
}
</code_context>
<issue_to_address>
**suggestion:** Navigation to next and previous items handle focus slightly differently; consider aligning the behavior.
For next items you call `resetFocus()` before `forceActiveFocus()`, but for previous items you only call `forceActiveFocus()`. If delegates manage internal focus targets, this asymmetry can cause different focus behavior when moving up vs down. Please either apply the same sequence in both directions or document why they must differ.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
18202781743
approved these changes
Mar 24, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, wjyrich 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 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: behind) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
focusedByNavigationproperty to GroupNotify, NormalNotify, and OverlapNotify componentsfocusedByNavigation=truewhen navigating between itemsfocusedByNavigationto false when item loses active focusThe issue was that close buttons were incorrectly showing when notifications received programmatic focus (e.g., during initialization or other non-navigation scenarios). This fix ensures close buttons only appear during actual keyboard navigation, improving the user experience by preventing visual clutter when not needed.
Log: Fixed notification close button visibility during keyboard navigation
Influence:
fix: 改进通知关闭按钮可见性逻辑
focusedByNavigation属性focusedByNavigation=truefocusedByNavigation重置为 false问题在于当通知获得程序化焦点时（例如初始化期间或其他非导航场景），关闭按
钮会错误地显示。此修复确保关闭按钮仅在真正的键盘导航期间出现，通过在不必
要时防止视觉混乱来改善用户体验。
Log: 修复了键盘导航期间通知关闭按钮的可见性问题
Influence:
Summary by Sourcery
Restrict notification close button visibility to focus gained via keyboard navigation rather than any programmatic focus, improving UX consistency across notification types.
Bug Fixes:
Enhancements: