-
Notifications
You must be signed in to change notification settings - Fork 55
fix: ensure focus before expand in OverlapNotify #1392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added forceActiveFocus() call before expand to fix Tab navigation starting from wrong position after mouse click expand. Log: ensure focus before expand in OverlapNotify pms: BUG-339891
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures the notification item gains active focus via the tap handler before triggering expand, so keyboard navigation (e.g., Tab after a mouse click) starts from the correct item. Sequence diagram for tap handling and focus before expand in OverlapNotifysequenceDiagram
actor User
participant TapHandler
participant OverlapNotify
participant FocusSystem
participant NotificationContent
User->>TapHandler: tap
TapHandler->>OverlapNotify: forceActiveFocus()
OverlapNotify->>FocusSystem: setActiveFocus(OverlapNotify)
FocusSystem-->>OverlapNotify: focusConfirmed
TapHandler->>OverlapNotify: expand()
OverlapNotify->>NotificationContent: showExpandedView()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- Consider passing an explicit focus reason to
forceActiveFocus()(e.g. mouse focus) if your Qt version supports it, so the focus change is correctly categorized for keyboard navigation and accessibility. - If there are other code paths that expand the same component via mouse/touch (beyond this
TapHandler), it may be worth centralizing theforceActiveFocus()+expand()sequence in a helper to avoid inconsistent behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider passing an explicit focus reason to `forceActiveFocus()` (e.g. mouse focus) if your Qt version supports it, so the focus change is correctly categorized for keyboard navigation and accessibility.
- If there are other code paths that expand the same component via mouse/touch (beyond this `TapHandler`), it may be worth centralizing the `forceActiveFocus()` + `expand()` sequence in a helper to avoid inconsistent behavior.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
deepin pr auto review我来对这段代码的变更进行审查:
建议:
Keys.onEnterPressed: {
root.forceActiveFocus()
root.expand()
}
Keys.onReturnPressed: {
root.forceActiveFocus()
root.expand()
}
function activateAndExpand() {
root.forceActiveFocus()
root.expand()
}然后在需要的地方调用这个函数。这样可以让代码更容易维护。 总的来说,这是一个好的修改,提升了用户体验和代码质量。 |
|
[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: blocked) |
Log: ensure focus before expand in OverlapNotify
pms: BUG-339891
Summary by Sourcery
Bug Fixes: