Re-order focus events for menuitemcheckbox and menuitemradio #16551
Conversation
…consider menuitemcheckbox and menuitemradio
…ecution if not applicable
WalkthroughThe recent changes focus on improving NVDA's handling of radio and checkbox menu items in web browsers. These updates in the Changes
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions PerformedReview triggered.
|
|
@SaschaCowley, I have reviewed the implementation of the The changes appear to be well-implemented and should address the issue of NVDA not correctly announcing checkbox or radio button menuitems when first entering submenus in Firefox or Chrome. If there are any specific aspects you would like me to focus on or if you have any questions, please let me know! Analysis chain@SaschaCowley, I will review the changes made in this pull request to ensure they align with the stated objectives and provide feedback accordingly. Let's start by examining the modifications in the Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for the implementation of the `FocusLossCancellableSpeechCommand.isMenuItemOfCurrentFocus` method to review the changes.
ast-grep --lang python --pattern $'class FocusLossCancellableSpeechCommand {
$$$
def isMenuItemOfCurrentFocus($_) {
$$$
}
$$$
}'
Length of output: 488 Script: #!/bin/bash
# Search for the definition of the `isMenuItemOfCurrentFocus` method in the `FocusLossCancellableSpeechCommand` class using ripgrep.
rg 'def isMenuItemOfCurrentFocus' -A 10
Length of output: 1007 Script: #!/bin/bash
# Extract the full implementation of the `isMenuItemOfCurrentFocus` method from the `source/eventHandler.py` file.
ast-grep --lang python --pattern $'def isMenuItemOfCurrentFocus($_) {
$$$
}' source/eventHandler.py
Length of output: 567 Script: #!/bin/bash
# Extract a larger portion of the `source/eventHandler.py` file around the `isMenuItemOfCurrentFocus` method to capture its full implementation.
rg 'def isMenuItemOfCurrentFocus' -A 50 source/eventHandler.py
Length of output: 2162 Actions PerformedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- source/eventHandler.py (4 hunks)
- user_docs/en/changes.md (1 hunks)
Additional Context Used
Ruff (1)
source/eventHandler.py (1)
278-278: f-string without any placeholders
Path-based Instructions (2)
source/eventHandler.py (1)
Pattern
**/*: focus on code smells, logic errors, edge cases, missing test cases and serious issues.
Avoid minor issues such as linting, formatting and style issues.
_ is defined for gettext globally, errors for this being undefined can be ignored.
This project uses tabs instead of spaces, do not suggest usage of spaces over tabs.user_docs/en/changes.md (1)
Pattern
**/*: focus on code smells, logic errors, edge cases, missing test cases and serious issues.
Avoid minor issues such as linting, formatting and style issues.
_ is defined for gettext globally, errors for this being undefined can be ignored.
This project uses tabs instead of spaces, do not suggest usage of spaces over tabs.
Additional comments not posted (5)
source/eventHandler.py (5)
253-267: The changes to includeIA2_ROLE_CHECK_MENU_ITEMandIA2_ROLE_RADIO_MENU_ITEMin theisMenuItemOfCurrentFocusmethod look good and align with the PR objectives.
321-323: The changes to allow focus redirection in theexecuteEventfunction look good and align with the PR objectives.
Line range hint
34-34: The removal of logic for redirecting focus events in thequeueEventfunction looks good and aligns with the PR objectives.
Line range hint
13-13: The addition of theIAccessible2Libimport looks good and aligns with the PR objectives.
Line range hint
242-285: The updates to theFocusLossCancellableSpeechCommandclass, including theisMenuItemOfCurrentFocusmethod, look good and align with the PR objectives.
See test results for failed build of commit c8c7874981 |
seanbudd
added
the
conceptApproved
# Conflicts: # user_docs/en/changes.md
source/eventHandler.py
Outdated
| return False | ||
|
|
||
| # Check that the old focus is a descendant of the new focus. | ||
| ancestor = self._obj.parent | ||
| while ancestor is not None: | ||
| if ancestor == lastFocus: | ||
| log.debugWarning( | ||
| log.debug( |
There was a problem hiding this comment.
Is there a reason why the log level changed here?
Debug warning makes sense to me in this case, as it is a warning message.
There was a problem hiding this comment.
| log.debug( | |
| log.debugWarning( |
Link to issue number:
Fixes #14550
Summary of the issue:
NVDA does not correctly announce checkbox or radio button menuitems when first entering submenues in Firefox or Chrome, as its announcement of the menuitem is disrupted by its announcement of the containing menu or grouping.
Description of user facing changes
Checkbox and radio button menuitems are correctly read in Chrome and Firefox.
Description of development approach
Updated
FocusLossCancellableSpeechCommand.isMenuItemOfCurrentFocusto:IA2_ROLE_CHECK_MENU_ITEMorIA2.IA2_ROLE_RADIO_MENU_ITEMas well asoleacc.ROLE_SYSTEM_MENUITEM; andTesting strategy:
Manual testing in Chrome and Firefox, using the APG editor menubar example.
Known issues with pull request:
None.
Code Review Checklist:
Summary by CodeRabbit