fix(ComboBox): fix highlight lingering and add keyboard navigation in popup#585
Open
electricface wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix(ComboBox): fix highlight lingering and add keyboard navigation in popup#585electricface wants to merge 1 commit intolinuxdeepin:masterfrom
electricface wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: electricface 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 GuideComboBox menu item highlighting is now gated on a new popup-level hover state so that the highlighted style clears when the mouse leaves the popup list area, implemented separately for Qt5 (MouseArea) and Qt6 (HoverHandler). Class diagram for updated ComboBox popup and delegate highlighting (Qt5 and Qt6)classDiagram
class ComboBox {
int currentIndex
int highlightedIndex
bool hoverEnabled
Popup popup
}
class Popup {
bool contentHovered
int implicitWidth
ArrowListView contentItem
}
class ArrowListView {
int maxVisibleItems
ListView view
}
class ListView {
int currentIndex
int highlightRangeMode
int highlightMoveDuration
}
class DelegateItem {
bool highlighted
bool hoverEnabled
bool autoExclusive
bool checked
+updateHighlighted(popupContentHovered, controlHighlightedIndex, index)
}
class MouseArea_Qt5 {
bool hoverEnabled
int acceptedButtons
+onContainsMouseChanged(containsMouse)
}
class HoverHandler_Qt6 {
+onHoveredChanged(hovered)
}
ComboBox --> Popup : owns
Popup --> ArrowListView : contentItem
ArrowListView --> ListView : view
ArrowListView --> MouseArea_Qt5 : Qt5 only
ArrowListView --> HoverHandler_Qt6 : Qt6 only
ListView --> DelegateItem : provides delegates
DelegateItem : highlighted = popup.contentHovered && control.highlightedIndex === index
MouseArea_Qt5 : onContainsMouseChanged sets popup.contentHovered
HoverHandler_Qt6 : onHoveredChanged sets popup.contentHovered
Flow diagram for ComboBox popup hover state driving delegate.highlightedflowchart TD
A[Mouse enters menu item area] --> B[Popup.contentHovered set to true]
B --> C[Control.highlightedIndex updated for hovered item]
C --> D[Delegate.highlighted evaluates popup.contentHovered && control.highlightedIndex === index]
D --> E[Matching delegate shows highlighted style]
F[Mouse leaves popup list area] --> G[Popup.contentHovered set to false]
G --> H[Delegate.highlighted re-evaluates to false for all items]
H --> I[All menu items clear highlighted style]
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 1 issue, and left some high level feedback:
- In both Qt5 and Qt6 versions, tying
delegate.highlightedtopopup.contentHovered && control.highlightedIndex === indexmeans keyboard-based navigation will no longer visually highlight items when the mouse is outside the popup; consider whethercontentHoveredshould be ignored (or handled differently) for focus/keyboard navigation to preserve accessibility and expected behavior. - For the Qt6
HoverHandler, instead of imperatively settingpopup.contentHoveredinonHoveredChanged, consider bindingpopup.contentHovereddirectly to the handler’shoveredproperty to avoid potential desynchronization if additional hover logic is added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both Qt5 and Qt6 versions, tying `delegate.highlighted` to `popup.contentHovered && control.highlightedIndex === index` means keyboard-based navigation will no longer visually highlight items when the mouse is outside the popup; consider whether `contentHovered` should be ignored (or handled differently) for focus/keyboard navigation to preserve accessibility and expected behavior.
- For the Qt6 `HoverHandler`, instead of imperatively setting `popup.contentHovered` in `onHoveredChanged`, consider binding `popup.contentHovered` directly to the handler’s `hovered` property to avoid potential desynchronization if additional hover logic is added later.
## Individual Comments
### Comment 1
<location path="qt6/src/qml/ComboBox.qml" line_range="39-42" />
<code_context>
text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData
icon.name: (control.iconNameRole && model[control.iconNameRole] !== undefined) ? model[control.iconNameRole] : null
- highlighted: control.highlightedIndex === index
+ highlighted: popup.contentHovered && control.highlightedIndex === index
hoverEnabled: control.hoverEnabled
autoExclusive: true
</code_context>
<issue_to_address>
**issue (bug_risk):** In Qt 6, `popup` is both missing an `id` and not visible from the delegate context.
`popup.contentHovered` will always fail here: the `Popup` has no `id`, so `popup` is undefined in this file, and even with `id: popup` the delegate can’t see it because the popup isn’t in the delegate’s context. Instead, expose the hover state via something the delegate can access (for example, a `control.popupContentHovered` property updated by the popup) and bind `highlighted` to that property.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
qt6/src/qml/ComboBox.qml
Outdated
Comment on lines
39
to
42
| highlighted: popup.contentHovered && control.highlightedIndex === index | ||
| hoverEnabled: control.hoverEnabled | ||
| autoExclusive: true | ||
| checked: control.currentIndex === index |
There was a problem hiding this comment.
issue (bug_risk): In Qt 6, popup is both missing an id and not visible from the delegate context.
popup.contentHovered will always fail here: the Popup has no id, so popup is undefined in this file, and even with id: popup the delegate can’t see it because the popup isn’t in the delegate’s context. Instead, expose the hover state via something the delegate can access (for example, a control.popupContentHovered property updated by the popup) and bind highlighted to that property.
electricface
force-pushed
the
swt/fix-bug306683
branch
from
e136cd3 to
42dda9f
Compare
18202781743
reviewed
Mar 10, 2026
qt6/src/qml/ComboBox.qml
Outdated
| text: control.textRole ? (Array.isArray(control.model) ? modelData[control.textRole] : (model[control.textRole] === undefined ? modelData[control.textRole] : model[control.textRole])) : modelData | ||
| icon.name: (control.iconNameRole && model[control.iconNameRole] !== undefined) ? model[control.iconNameRole] : null | ||
| highlighted: control.highlightedIndex === index | ||
| highlighted: popup.contentHovered && control.highlightedIndex === index |
Contributor
There was a problem hiding this comment.
popup是可以从外部赋值的，不应该在ComboBox里访问popup，可以参照 https://github.com/linuxdeepin/dtkdeclarative/pull/569/changes
electricface
force-pushed
the
swt/fix-bug306683
branch
2 times, most recently
from
957b205 to
0fcafee
Compare
Contributor
deepin pr auto review这段代码主要实现了 ComboBox 组件中键盘导航与鼠标悬停的交互逻辑，并添加了一些调试日志。以下是对这段代码的审查意见，分为语法逻辑、代码质量、代码性能和代码安全四个方面： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进建议总结
修改后的代码片段示例（仅展示关键部分修改）T.ComboBox {
id: control
// 删除了调试用的 Rectangle
property string iconNameRole
property string alertText
// ...
// 状态说明：
// keyboardNavIndex: 键盘导航时的当前索引，-1 表示未激活键盘导航
// contentHovered: 鼠标是否正在悬停控制高亮
// lastHoveredIndex: 上次鼠标悬停的索引，用于从键盘切回鼠标时的参考
property int keyboardNavIndex: -1
// ...
delegate: ItemDelegate {
// ...
highlighted: popup.contentHovered
? control.highlightedIndex === index
: control.keyboardNavIndex === index
onHighlightedChanged: {
if (highlighted && popup.contentHovered)
popup.lastHoveredIndex = index
}
// ...
}
popup: Popup {
id: popup
// ...
onOpened: {
// 初始化状态
control.keyboardNavIndex = -1
popup.lastHoveredIndex = -1
// 如果需要，可以在这里设置初始 keyboardNavIndex，例如基于 currentIndex
// control.keyboardNavIndex = control.currentIndex
}
contentItem: ArrowListView {
// ...
Keys.onPressed: (event) => {
// 移除了 console.log
switch (event.key) {
case Qt.Key_Down:
// 切换到键盘模式
popup.contentHovered = false
if (control.keyboardNavIndex === -1) {
// 从上次鼠标位置或当前选中位置开始
var startIdx = popup.lastHoveredIndex >= 0 ? popup.lastHoveredIndex : control.currentIndex
control.keyboardNavIndex = Math.min(startIdx + 1, control.count - 1)
} else {
control.keyboardNavIndex = Math.min(control.keyboardNavIndex + 1, control.count - 1)
}
event.accepted = true
break
case Qt.Key_Up:
popup.contentHovered = false
if (control.keyboardNavIndex === -1) {
var startIdx = popup.lastHoveredIndex >= 0 ? popup.lastHoveredIndex : control.currentIndex
control.keyboardNavIndex = Math.max(startIdx - 1, 0)
} else {
control.keyboardNavIndex = Math.max(control.keyboardNavIndex - 1, 0)
}
event.accepted = true
break
case Qt.Key_Return:
case Qt.Key_Enter:
if (control.keyboardNavIndex >= 0) {
control.currentIndex = control.keyboardNavIndex
control.activated(control.keyboardNavIndex)
popup.close()
} else {
// 如果没有键盘导航索引，可能是在鼠标模式下按回车，通常关闭弹窗即可
// 或者根据需求选择是否执行 currentIndex
popup.close()
}
event.accepted = true
break
}
}
HoverHandler {
id: hoverHandler
onHoveredChanged: {
if (hovered) {
popup.contentHovered = true
control.keyboardNavIndex = -1
} else {
// 注意：这里设置为 false 可能会导致鼠标移出列表一瞬间，
// 用户按键盘键时状态判断有微小延迟，但在当前逻辑下是必要的
popup.contentHovered = false
}
}
}
Connections {
target: control
function onHighlightedIndexChanged() {
// 只有当鼠标真正在操作（highlightedIndex 变化）且当前不是键盘模式时，才强制切回鼠标模式
// 这里的逻辑是为了防止键盘操作修改了 highlightedIndex 导致误判
if (control.highlightedIndex >= 0 && !popup.contentHovered) {
// 此时说明鼠标移动导致了 highlightedIndex 变化，或者外部逻辑改变了它
// 如果是键盘操作，通常会先设置 keyboardNavIndex，不会触发这里的逻辑（因为 keyboardNavIndex != -1 时逻辑分支不同）
// 但为了安全，确保状态一致性
popup.contentHovered = true
control.keyboardNavIndex = -1
}
}
}
}
// ...
}
} |
electricface
force-pushed
the
swt/fix-bug306683
branch
from
0fcafee to
5edc15e
Compare
… popup Fix highlight lingering after mouse leaves. Add keyboard navigation: - Up/Down: move from current hovered or last-hovered item - Home/End: jump to first/last item - Enter: confirm highlighted item in either mouse or keyboard mode Mouse and keyboard modes switch seamlessly in both directions. fix(ComboBox): 修复 ComboBox 的高亮残留问题，并为弹窗增加键盘导航功能 修复鼠标移出后高亮项仍然残留的问题 增加键盘导航支持： - 上/下键：在当前悬停或最后悬停的选项间移动 - Home/End 键：跳转至第一个或最后一个选项 - 回车键：确认选中当前高亮的选项（支持鼠标或键盘操作模式） 鼠标模式与键盘模式可双向无缝切换 Log: 修复ComboBox的菜单项鼠标离开后高亮残留问题 Influence: ComboBox菜单项 PMS: BUG-304991
electricface
force-pushed
the
swt/fix-bug306683
branch
from
5edc15e to
8398b4e
Compare
electricface
changed the title
18202781743
reviewed
Mar 11, 2026
| view.highlightRangeMode: ListView.ApplyRange | ||
| view.highlightMoveDuration: 0 | ||
| Keys.priority: Keys.BeforeItem | ||
| Keys.onPressed: (event) => { |
Contributor
There was a problem hiding this comment.
这是手搓了个ComboBox的按键处理呀，无法借助qt里面的处理么，
Contributor
|
TAG Bot New tag: 6.7.36 |
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.
Fix highlight lingering after mouse leaves.
Add keyboard navigation:
- Up/Down: move from current hovered or last-hovered item
- Home/End: jump to first/last item
- Enter: confirm highlighted item in either mouse or keyboard mode
Mouse and keyboard modes switch seamlessly in both directions.