Revert "Give ui.browseableMessage the ability to show copy and close buttons after the message, and add buttons to some instances"

[XLTechie]

Reverts PR

Reverts #16369

Issues fixed

Fixes #16996
Fixes #16995

Issues reopened

Reopens #14641

Reason for revert

Security tightening requested by @seanbudd, caused an inadvertent recurrence of #12004. (#16995)

In addition, a known (but I thought minor) quirk of the original PR, was that users pressing the Alt+C key combination to activate the copy button, would hear a ding. This is a "feature" of MSHTML, and not something for which I believe there is a solution. NVDA did speak a message indicating that text was successfully copied, but @CyrilleB79 suggested that was not enough to negate the ding. (#16996)

Can this PR be reimplemented? If so, what is required for the next attempt

For the blank lines issue, there are a couple possible paths forward:

1. After escaping the incoming message, replace \n with <br>, and feed it to .innerHTML instead of .innerText. This re-raises one of @seanbudd's original concerns about the use of .innerHTML, however, so may not be viable.
2. @michaelDCurran has outlined a potential path forward, in In ui.browseableMessage, every other line is empty #16995 (comment), but further investigation is required.

For the ding on Alt+C issue, there are two possibilities known to me:

1. Choose a key that doesn't ding. That will probably mean it can not use Alt, which makes it unusual for button activation. Perhaps @CyrilleB79 has ideas.
2. Do not use an accelerator for the copy button.

Summary by CodeRabbit

• New Features
  • Enhanced command line options for improved update control.
  • Introduced functionality for redirection of objects from on-screen coordinates.
  • Updated eSpeak NG to support new languages, including Faroese and Xextan.
• User Interface Improvements
  • Added "Close" and "Copy" buttons to various dialogs for better accessibility.
• Performance Enhancements
  • Improved SCons build system with --all-cores parameter for optimized performance.
• Documentation
  • Updated developer information with application architecture details.

[coderabbitai]

Walkthrough

The recent changes streamline the NVDA application by simplifying the user interface for displaying browsable messages. Key parameters related to button visibility have been removed, and JavaScript functionality has been reduced to solely handle closing messages with the Escape key. These modifications enhance usability while focusing on essential features, reflecting a shift towards a more minimalist design.

Changes

Files	Change Summary
source/globalCommands.py	Removed closeButton and copyButton parameters from multiple calls to ui.browseableMessage, simplifying the function's usage.
source/message.html	Renamed handleKeyPress to escapeKeyPress, eliminated onCopyButtonPress, and simplified message handling logic within windowOnLoad.
source/ui.py	Updated type hints in function signatures, removed _warnBrowsableMessageComponentFailure, and simplified the browseableMessage function logic.

Assessment against linked issues

Objective	Addressed	Explanation
Prevent "ding" sound on Alt+C when copying text (#[16996])	✅
Avoid empty lines in ui.browseableMessage (#[16995])	❌	The changes do not address the issue of empty lines.

---

Recent review details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c8a847b and 19b5cb4.

Files selected for processing (4)

• source/globalCommands.py (5 hunks)
• source/message.html (1 hunks)
• source/ui.py (3 hunks)
• user_docs/en/changes.md (2 hunks)

Additional context used

Path-based instructions (4)

source/message.html (1)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>

source/ui.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>

---

Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

source/globalCommands.py (2)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>

---

Pattern **/*.py: _, pgettext, ngettext, and ngettext are defined globally, errors for this being undefined can be ignored.

user_docs/en/changes.md (3)

Pattern **/*: Focus on code smells, logic errors, edge cases, missing test cases, security flaws and serious issues. Avoid commenting on minor issues such as linting, formatting and style issues. This project uses tabs instead of spaces, do not suggest usage of spaces over tabs. Are there any 'red flags' in this code that might warrant closer investigation from a security standpoint? Explain what makes them suspicious. When providing code suggestions, particularly when requested, ensure GitHub's suggestion format is used, i.e.: suggestion <code changes>

---

Pattern **/*.md: Focus on grammar, spelling, and punctuation. Also consider readability and clarity of contents. Ensure that changes follow the style of one sentence per line, suggest changes if this is not met.

---

Pattern user_docs/en/changes.md: Ensure each change log entry references an issue or pull request number. Change log entries can also include a reference to a GitHub author. Examples of valid change log entries: * Item with sub-items (#123, @username): * sub-item * bar (#342) * Item with. Multiple lines. (#143)

Additional comments not posted (8)

source/message.html (2)

7-12: LGTM! Simplified key press handling.

The renaming and simplification of the function to exclusively handle the Escape key aligns with the PR's objectives.

---

15-18: Security concern: Directly setting innerHTML.

Ensure that args.item('message') is properly sanitized to prevent potential XSS vulnerabilities when setting innerHTML.

source/ui.py (2)

49-49: LGTM! Improved type clarity.

The change to use Optional[str] enhances type clarity and is consistent with Python typing best practices.

---

91-96: LGTM! Simplified error handling and improved type clarity.

The changes align with the PR's objectives of streamlining error handling and improving type clarity.

Ensure that the removal of _warnBrowsableMessageComponentFailure has not affected other parts of the codebase.

Verification successful

---

No Impact from Removal of _warnBrowsableMessageComponentFailure

The function _warnBrowsableMessageComponentFailure is not used elsewhere in the codebase, confirming that its removal does not affect other parts of the project.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `_warnBrowsableMessageComponentFailure` is not used elsewhere in the codebase.

# Test: Search for the usage of `_warnBrowsableMessageComponentFailure`. Expect: No occurrences.
rg --type python '_warnBrowsableMessageComponentFailure'

Length of output: 87

---

Script:

#!/bin/bash
# Description: Verify that `_warnBrowsableMessageComponentFailure` is not used elsewhere in the codebase.

# Test: Search for the usage of `_warnBrowsableMessageComponentFailure`. Expect: No occurrences.
rg '_warnBrowsableMessageComponentFailure'

Length of output: 42

source/globalCommands.py (4)

4143-4143: Verify the impact of removing copyButton and closeButton.

Ensure that the removal of these parameters from ui.browseableMessage aligns with the intended user interface behavior and does not negatively impact accessibility.

---

2213-2213: Verify the impact of removing closeButton.

Ensure that the removal of the closeButton parameter from ui.browseableMessage aligns with the intended user interface behavior and does not negatively impact accessibility.

Verification successful

---

Removal of closeButton is consistent and does not impact UI behavior.

The closeButton parameter has been removed from all instances of ui.browseableMessage, aligning with the intended simplification and security improvements. There are no adverse impacts on the user interface observed.

• Instances of ui.browseableMessage across the codebase do not include closeButton.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing `closeButton` from `ui.browseableMessage` calls.

# Test: Search for `ui.browseableMessage` usage. Expect: Only occurrences without `closeButton`.
rg --type python -A 5 $'ui.browseableMessage'

Length of output: 75

---

Script:

#!/bin/bash
# Description: Verify the impact of removing `closeButton` from `ui.browseableMessage` calls.

# Corrected Test: Search for `ui.browseableMessage` usage in Python files. Expect: Only occurrences without `closeButton`.
rg --type py -A 5 $'ui.browseableMessage'

Length of output: 4559

---

4143-4143: Verify the impact of removing copyButton and closeButton.

Ensure that the removal of these parameters from ui.browseableMessage aligns with the intended user interface behavior and does not negatively impact accessibility.

Verification successful

---

Removal of copyButton and closeButton is consistent with current usage.

The ui.browseableMessage calls do not include copyButton and closeButton parameters across the codebase, confirming that their removal aligns with the intended usage and does not negatively impact accessibility.

• Occurrences found in source/globalCommands.py, source/appModules/poedit.py, and other modules confirm this consistency.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing `copyButton` and `closeButton` from `ui.browseableMessage` calls.

# Test: Search for `ui.browseableMessage` usage. Expect: Only occurrences without `copyButton` and `closeButton`.
rg --type python -A 5 $'ui.browseableMessage'

Length of output: 75

---

Script:

#!/bin/bash
# Description: Verify the impact of removing `copyButton` and `closeButton` from `ui.browseableMessage` calls.

# Test: Search for `ui.browseableMessage` usage. Expect: Only occurrences without `copyButton` and `closeButton`.
rg -A 5 $'ui.browseableMessage'

Length of output: 21250

---

343-345: Verify the impact of removing copyButton and closeButton.

The removal of these parameters changes the user interface behavior. Ensure that this does not negatively impact the user experience or introduce regressions related to issues #16996 and #16995.

Verification successful

---

Verification complete: No regressions found with ui.browseableMessage changes.

The removal of copyButton and closeButton parameters from ui.browseableMessage calls is consistent throughout the codebase. This change aligns with the security-focused objectives of the PR and does not introduce regressions related to issues #16996 and #16995.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing `copyButton` and `closeButton` from `ui.browseableMessage` calls.

# Test: Search for `ui.browseableMessage` usage. Expect: Only occurrences without `copyButton` and `closeButton`.
rg --type python -A 5 $'ui.browseableMessage'

Length of output: 75

---

Script:

#!/bin/bash
# Description: Verify the impact of removing `copyButton` and `closeButton` from `ui.browseableMessage` calls.

# Test: Search for `ui.browseableMessage` usage. Expect: Only occurrences without `copyButton` and `closeButton`.
rg -A 5 'ui.browseableMessage'

Length of output: 21250

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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.
  • @coderabbitai help me debug CodeRabbit configuration file.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

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

CodeRabbit Configuration 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.

[coderabbitai]

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (5)

user_docs/en/changes.md (5)

Line range hint 1-3:
Ensure consistent style with one sentence per line.

The entry about the -c/--config-path and --disable-addons options is correctly formatted. However, ensure that all entries follow the style of one sentence per line for consistency.

---

Line range hint 2-3:
Clarify language addition details.

The entry about eSpeak NG could be clearer by specifying that the new languages are added to the supported list.

* eSpeak NG has been updated to 1.52-dev commit `961454ff`. (#16775)
  * New languages, Faroese and Xextan, have been added to the supported list.

---

Line range hint 4-5:
Ensure clarity and adherence to style guidelines.

The entry about redirecting objects is clear and references an issue and author correctly. Ensure that all entries maintain clarity and adhere to the style of one sentence per line.

---

Line range hint 5-6:
Clarify the SCons parameter entry.

The entry about the SCons parameter could be clearer by specifying its effect on build performance.

* Running SCons with the parameter `--all-cores` will automatically select the maximum number of available CPU cores, optimizing build performance. (#16943, #16868, @LeonarddeR)

---

Line range hint 6-7:
Ensure clarity and consistency in developer info entry.

The entry about developer info is clear but ensure consistency in style by maintaining one sentence per line.

[CyrilleB79]

Re the ding issue, I had just opened an issue so that it is tracked.

I am not opposed to keep this known issue if it is documented in the known issues of the (possibly future) PR and if the consensus is that the drawback of the ding does not compensate the added value of a shortcut key.

[XLTechie]

@CyrilleB79 What do you think of switching to Ctrl+Alt+C? That does not ding.

[SaschaCowley]

I wouldn't go with control+alt+c as it may conflict with a desktop shortcut hotkey.

[XLTechie]

@SaschaCowley I thought of that. Pretty much anything could. Google Docs, for example, uses lots of Ctrl+Alt combinations in their keyboard interfaces. Not that I advocate emulating their style in anything, but my point is it's pretty common not to take possible desktop shortcuts into account. I'm personally fine with this button not having an accelerator at all, but a couple people seemed to expect one.

[SaschaCowley]

@XLTechie that's true, but I suspect a high number of NVDA users use desktop shortcut hotkeys, so I think control+alt+letter is probably best avoided. Just my thoughts though :)

[XLTechie]

All right, Ctrl+Shift+C it is.

[seanbudd]

CTRL + INSERT is a pretty common alternative to ctrl+c in windows, not sure if thats viable

[XLTechie]

@seanbudd Would NVDA users expect Insert (i.e. the NVDA key) to be used in this way? I didn't test that one, but shall do for completeness.

[CyrilleB79]

Ctrl+insert was common in DOS times, but is much less common, or at least much less advertised, since Windows 95, even if still supported. Moreover, it is clearly still less used by NVDA or Jaws users due to the Insert key being used as NVDA/Jaws key. So that's not a desirable choice at all.

The best solution would be to remove the ding if @SaschaCowley's suggestion works and keep alt+c.

If not possible, I am unsure if no shortcut at all is better or if something else less common (e.g. control+shift+c) would be suitable.

[CyrilleB79]

Oh sorry, just realized that Sasha's suggestion does not work.