Skip to content
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

Windows 11 emoji panel/navigation menu item: handle UIA elemenet selected event by ignoring it or manipulating NVDA's focus object #16555

Merged

Conversation

josephsl
Copy link
Collaborator

@josephsl josephsl commented May 16, 2024

Link to issue number:

Closes #16346
Closes #16532
Closes #16533
Follow-up to #16347

Summary of the issue:

NVDA cuts off speech, repeats items, and does not report system focus upon closing emji panel when handling element selected event in Windows 11 emoji panel's navigation menu items.

Description of user facing changes

NVDA will:

  • No longer cut off speech when reviewing kaomojis and symbols in fullscreen
  • Nl longer announce "clipboard history" twice when reviewing amoji panel categories
  • No longer get stuck in emoji panel items when the panel closes

Description of development approach

A dedicated overlay class was added for Windows 11 emoji panel navigation menu items along with UIA element selected event for them that does the following:

  • Ignores the event when the panel closes, system focus moves to menu items, and while kaomojis and symbols are shown on screen
  • Sets NVDA focus to system focus when emoji panel closes and NVDA finds itself stuck in emoji panel main window.

Testing strategy:

Manual testing: see #16346, #16532, and #16533. Specifically, test to make sure that NVDA is no longer announcing "clipboard history" twice while navigating parts of the emoji panel and speech is no longer cut off while reviewing kaomojis and symbols.

Known issues with pull request:

This PR may not cover all the cases of element selected event mishap, but it covers most noticeable cases.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

Summary by CodeRabbit

  • New Features

    • Introduced NavigationMenuItem for better navigation in the Windows 11 emoji panel.
  • Bug Fixes

    • Enhanced NVDA's behavior with the Windows 11 emoji panel and clipboard history.
    • Improved text readout reliability in terminal applications and Excel cell editing announcements.

josephsl added 10 commits May 15, 2024 09:22
…avigation menu item.

Define a dedicated overlay class for Windows 11 emoji panel navigation menu items (deriving from UIA list item class) to handle elemenet selected event and others if needed. This keeps the element selectd event handler defined in the app module simple.
…ion Id string staring with 'navigation-menu-item' as navigation menu item class.

Rework list item recognition: if encountering a list item, differentiate between IME candidate items and Windows 11 emoji panel navigation menu items.
…ass handle the event.

Allow Windows 11 emoji panel navigation menu item overlay class to handle UIA element selected evnt in situations such as element selected event being fired when emoji panel closes in Windows 11.
…event if the event fires when the emoji panel is closed. Re nvaccess#16346.

Define UIA element selected event for navigation menu item overlay class, designed to ignore the event when conditions arise. The first condition is event being fired when closing the emoji panel - check the app modules and ignore the event if the focused app module is not the emoji panel app module, utilizing Python 3.8's assignment expression (:=) syntax (focus object is neeeded in other conditions).
… item when apropriate. Re nvaccess#16346.

Report the selected menu item when ignore conditions are dealt with.
…oji panel is closed and emoji search field is offscreen. Re nvaccess#16346.

Unlike the event ignore condition, NVDA's focus object stays on emoji edit field when the panel is closed, leaving it offscren. In this case, NVDA does not know where to move the 'focus object' to. Therefore, tell NVDA to queue gain focus event on the system focus control returned by obj.objectWithFocus() method.
…event if menu items are gaining focus. Re nvaccess#16532.

Gain focus event is fired by navigation menu itmes if the panel is open to clipboard history, resulting in repetitions (both gain focus and element selected events are fired for clipboard history item). Ignore the element selected event so gain focus event can announce the selected menu item.
…viewing kaomojis and symbols in fullscreen. Re nvaccess#16533.

Gain focus event is fired when moving through kaomoji and symbol entries. When Tab is pressed, both gain focus and elemenet selected events are fired. Therefore, trust the gain focus event.
…m focus when emoji panel main window unexpected gains focus accompanied by elemenet selected event. Re nvaccess#16347.

A variation of issue 16347: sometimes when closing emoji panel/clipboard history, input experience main window grabs NVDA's focus accompanied with UIA element selected event. The gain focus event handler defined at the app module level only manipulates focus if no events are pending, but inthis case, events were pending. Therefore, catch this and manipulate NVDA focus if NVDA finds itself stuck in emoji panel main window element while handling element selected event.
@josephsl josephsl requested a review from a team as a code owner May 16, 2024 00:39
@josephsl josephsl requested a review from michaelDCurran May 16, 2024 00:39
@josephsl
Copy link
Collaborator Author

Note: changelog wil be written after giving CodeRabbitAI bot a chance to check this PR.

@seanbudd
Copy link
Member

Hi @josephsl - I've turned off automatic CodeRabbit reviews for now until we are happy with the type of feedback being given. They can still be manually triggered with

@coderabbitai review

Copy link
Contributor

coderabbitai bot commented May 16, 2024

@seanbudd, I will review the changes now.

Actions Performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

coderabbitai bot commented May 16, 2024

Walkthrough

The recent updates enhance NVDA's interaction with the Windows 11 emoji panel and clipboard history by introducing the NavigationMenuItem class and refining the event_UIA_elementSelected logic. These changes address specific issues related to UIA element selection events, improving the accuracy and reliability of NVDA's behavior in handling emoji panel navigation and clipboard history.

Changes

Files/Groups Change Summary
source/appModules/windowsinternal_composableshell_experiences_textinput_inputapp.py Added ListItem import, introduced NavigationMenuItem class, and updated event_UIA_elementSelected logic.
user_docs/en/changes.md Documented improvements in NVDA's behavior with Windows 11 emoji panel and clipboard history.

Assessment against linked issues

Objective Addressed Explanation
Handle UIA element selected event after emoji panel closes (#16346)
Prevent duplicate announcements in clipboard history navigation menu (#16532)
Ensure uninterrupted speech when reviewing kaomojis/symbols (#16533)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Out of diff range and nitpick comments (2)
source/appModules/windowsinternal_composableshell_experiences_textinput_inputapp.py (2)

Line range hint 84-84: Undefined name _.

The _ function is used for translations but is not defined or imported in this file. Ensure it is imported or defined.

+ from gettext import gettext as _

Line range hint 229-229: Undefined name _.

The _ function is used for translations but is not defined or imported in this file. Ensure it is imported or defined.

+ from gettext import gettext as _

Comment on lines 27 to 30
* In emoji panel, NVDA will no longer appear to get stuck when closing clipboard history and emoji panel. (#16346, #16347, @josephsl)
* NVDA will once again announce visible candidates when opening Windows 11 IME interface. (#14023, @josephsl)
* NVDA will no longer announce "clipboard history" twice when reviewing emoji panel navigation menu items. (#16532, @josephsl)
* NVDA will no longer cut off speech and braille while reviewing kaomojis and symbols in emoji panel. (#16533, @josephsl)
Copy link
Contributor

@coderabbitai coderabbitai bot May 16, 2024

Choose a reason for hiding this comment

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

Ensure clarity and consistency in documentation.

The documentation changes accurately reflect the improvements and fixes mentioned in the PR summary. However, consider rephrasing for better clarity and consistency:

-  * In emoji panel, NVDA will no longer appear to get stuck when closing clipboard history and emoji panel. (#16346, #16347, @josephsl)
+  * NVDA will no longer get stuck when closing the clipboard history and emoji panel in Windows 11. (#16346, #16347, @josephsl)
-  * NVDA will once again announce visible candidates when opening Windows 11 IME interface. (#14023, @josephsl)
+  * NVDA will announce visible candidates again when opening the Windows 11 IME interface. (#14023, @josephsl)
-  * NVDA will no longer announce "clipboard history" twice when reviewing emoji panel navigation menu items. (#16532, @josephsl)
+  * NVDA will no longer announce "clipboard history" twice when navigating through the emoji panel menu items. (#16532, @josephsl)
-  * NVDA will no longer cut off speech and braille while reviewing kaomojis and symbols in emoji panel. (#16533, @josephsl)
+  * NVDA will no longer cut off speech and braille when reviewing kaomojis and symbols in the emoji panel. (#16533, @josephsl)

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
* In emoji panel, NVDA will no longer appear to get stuck when closing clipboard history and emoji panel. (#16346, #16347, @josephsl)
* NVDA will once again announce visible candidates when opening Windows 11 IME interface. (#14023, @josephsl)
* NVDA will no longer announce "clipboard history" twice when reviewing emoji panel navigation menu items. (#16532, @josephsl)
* NVDA will no longer cut off speech and braille while reviewing kaomojis and symbols in emoji panel. (#16533, @josephsl)
* NVDA will no longer get stuck when closing the clipboard history and emoji panel in Windows 11. (#16346, #16347, @josephsl)
* NVDA will announce visible candidates again when opening the Windows 11 IME interface. (#14023, @josephsl)
* NVDA will no longer announce "clipboard history" twice when navigating through the emoji panel menu items. (#16532, @josephsl)
* NVDA will no longer cut off speech and braille when reviewing kaomojis and symbols in the emoji panel. (#16533, @josephsl)

Copy link
Member

Choose a reason for hiding this comment

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

these suggestions look good to me, except perhaps the first one? The first one appears to be changing meaning

Copy link
Contributor

Choose a reason for hiding this comment

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

@seanbudd, thank you for your feedback. Upon reviewing the first suggestion, I agree that it might change the meaning slightly. The original text emphasizes that NVDA appears to get stuck, while the suggested change implies that it definitively gets stuck. To maintain the original intent, we can rephrase it for clarity without altering the meaning:

-  * In emoji panel, NVDA will no longer appear to get stuck when closing clipboard history and emoji panel. (#16346, #16347, @josephsl)
+  * NVDA will no longer appear to get stuck when closing the clipboard history and emoji panel in Windows 11. (#16346, #16347, @josephsl)

Would you like me to proceed with this revision?


New Learnings Added
User: seanbudd
PR: nvaccess/nvda#16555
File: user_docs/en/changes.md:27-30
Timestamp: 2024-05-16T01:53:50.659Z
Learning: When suggesting documentation changes, ensure that the rephrased text does not alter the original meaning, especially when the original text includes qualifiers like "appear to."

Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to NV Access and the community: the conversations I'm having with Code Rabbit is not meant for human consumption but feel free to look at it (it also shows how I'm approaching the bot as a software engineer and a grad student/researcher interested in communication and tech):

I see. Two things:

  1. Not all members of the project are trained in technical communication (I am schooled in it), and for many, English is not our first language (me included). So issues on clarity is acceptable (we have a staff member at NV Access who can go over grammar and other issues later).
  2. Look at the unindented line which says "Windows 11 fixes", so there is no need to repeat this in indented lines.

I know you are using sophisticated networks to analyze suggestions based on whatever software development data (philosophy, experiences, documentation, and what not). Do remember his though: whatever was set by the organization (including its discourse and mindset) can and do override commonly held ideas represented by the data you are using. At least, for the first try, you got it right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@josephsl
Copy link
Collaborator Author

josephsl commented May 16, 2024 via email

@josephsl
Copy link
Collaborator Author

Hi all,

I'll incorporate both human and machine generated suggestions for the changelog. A few thoughts:

  • At least I know that the bot incorporates supervised learning (contextual based on code location) and human input (it'll be interesting to observe M2M (machine-to-machine or model-to-model) or D2D (data-to-data) conversations).
  • The bot does a good job summarizing the overall purpose of the pull request. I wish it could incorporate Git commit logs as it can offer a much better "data" and "context" about what it is examining (privacy implications should also be discussed).
  • From communication studies perspective, the bot may have an idea that organizational and project discourse and learning matter but still insist on things like adding docstrings when in fact they are present. From this, it could be inferred that the bot offers specific style of programming (of course based on data and what the model has parsed/networked/learned so far). The interaction I have seen between Sean (and I) and the bot is a good example of CASA/computers as social actors paradigm (a machine looks and acts like a human and is sociable, according to human users who mindlessly interact with machines).

Two implications:

  • For software developers and community contributors: be mindful that suggestions from this bot (and friends) come from data. In other words, do not mindlessly think suggestions from the "bot collaborator" is 100% right and applicable (context matters).
  • For communication studies researchers and technical communication students: think about "digitized ideal communicator" idea - how data creates an idealized communicator who can offer suggestions. In other words, do not simply treat machines as just a collection of data - think of consequences of treating it as a communicator, with the data being the "lifeblood" and channel by which both itself and someone/somebody/something else (designers, organizations, societies) communicate.

Layman summary: the bot is useful, but have a critical mindset when interacting with it.

Thanks.

Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

Thanks @josephsl

@seanbudd seanbudd merged commit ee34621 into nvaccess:master May 17, 2024
1 check passed
@josephsl josephsl deleted the w11EmojiPanelNavigationMenuElementSelected branch May 17, 2024 06:59
@coderabbitai coderabbitai bot mentioned this pull request Dec 9, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants