Skip to content

Conversation

@wjyrich
Copy link
Contributor

@wjyrich wjyrich commented Sep 24, 2025

Changed the textVisible property condition to include both hovered and activeFocus states instead of just hovered. This ensures that the button text remains visible when the button has keyboard focus, improving accessibility for users navigating with keyboard or screen readers.

The previous implementation only showed text on hover, which made it difficult for keyboard-only users to see the button labels when navigating through focus.

fix: 改进 AnimationSettingButton 中的文本可见性逻辑

将 textVisible 属性的条件从仅检查悬停状态改为同时检查悬停和焦点状态。这
确保了按钮在获得键盘焦点时文本仍然可见,提高了使用键盘或屏幕阅读器用户的
可访问性。

之前的实现仅在悬停时显示文本,这使得仅使用键盘导航的用户在通过焦点浏览时
难以看到按钮标签。

Pms: BUG-284477

Summary by Sourcery

Bug Fixes:

  • Show button text when the button has active keyboard focus or hover instead of only on hover

@sourcery-ai
Copy link

sourcery-ai bot commented Sep 24, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

The PR enhances accessibility by revising the AnimationSettingButton’s text visibility logic to display button labels not only on hover but also when the button receives keyboard focus, ensuring screen-reader and keyboard-only users can see the text.

File-Level Changes

Change Details Files
Improve text visibility logic to include keyboard focus
  • Modify textVisible condition to use (hovered OR activeFocus)
  • Add activeFocus check alongside hovered
  • Keep existing non-empty text requirement
panels/notification/center/AnimationSettingButton.qml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Changed the textVisible property condition to include both hovered and
activeFocus states instead of just hovered. This ensures that the button
text remains visible when the button has keyboard focus, improving
accessibility for users navigating with keyboard or screen readers.

The previous implementation only showed text on hover, which made
it difficult for keyboard-only users to see the button labels when
navigating through focus.

fix: 改进 AnimationSettingButton 中的文本可见性逻辑

将 textVisible 属性的条件从仅检查悬停状态改为同时检查悬停和焦点状态。这
确保了按钮在获得键盘焦点时文本仍然可见,提高了使用键盘或屏幕阅读器用户的
可访问性。

之前的实现仅在悬停时显示文本,这使得仅使用键盘导航的用户在通过焦点浏览时
难以看到按钮标签。

Pms: BUG-284477
@deepin-ci-robot
Copy link

deepin pr auto review

我来审查这段代码的变更。这是一个QML文件,用于通知中心中的动画设置按钮。主要变更是在textVisible属性的判断条件中。

语法逻辑分析

变更前:

property bool textVisible: root.hovered && text !== ""

变更后:

property bool textVisible: (root.hovered || root.activeFocus) && text !== ""

这个变更在逻辑上是合理的,它扩展了文本可见的条件,不仅考虑了鼠标悬停状态(root.hovered),还增加了键盘焦点状态(root.activeFocus)。这样可以提高键盘导航的可访问性。

代码质量

  1. 可访问性改进:这个变更提高了组件的可访问性,使得用户可以通过键盘导航时也能看到文本提示,这是一个很好的改进。
  2. 代码清晰度:代码仍然保持简洁明了,条件判断的逻辑清晰。
  3. 括号使用:添加了括号使逻辑关系更加清晰,这是一个好的做法。

代码性能

这个变更对性能的影响可以忽略不计,因为:

  1. 只是增加了一个简单的布尔条件判断
  2. activeFocushovered都是QML内置属性,访问开销很小
  3. 整个表达式仍然非常简单,不会造成性能问题

代码安全

这个变更不会引入任何安全隐患,因为:

  1. 只是扩展了条件判断的范围,没有引入新的外部输入
  2. activeFocus是安全的内置属性,不会导致安全问题
  3. 没有改变原有的安全检查逻辑(如text !== ""

改进建议

  1. 考虑动画效果:既然这是一个动画设置按钮,可以考虑在文本显示/隐藏时添加平滑的过渡动画,提升用户体验。
  2. 添加文档注释:可以为textVisible属性添加注释,说明它的用途和触发条件,特别是新增的键盘焦点功能。
  3. 考虑边界情况:如果文本内容很长,可能需要考虑文本截断或换行的处理。

总结

这是一个很好的改进,提高了组件的可访问性,同时保持了代码的简洁性和性能。变更合理且安全,符合无障碍设计的原则。如果可能,可以考虑进一步添加动画效果和文档注释,使组件更加完善。

@wjyrich wjyrich merged commit c3bb9dd into linuxdeepin:master Sep 24, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants