Give ui.browseableMessage the ability to show copy and close buttons after the message, and add buttons to some instances (2nd try) #5
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
…instances. * Report link destination, Speak selection command, & character formatting browseableMessages given close and copy buttons. * Added a new browsable message component failure messaging function. * Used a live region to alert users about the status of the copy operation. * Added some further error handling, as proposed by the AI. * Change onkeypress to onkeydown, in order to pick up modifier keys in message.html. * Used Ctrl+Shift+C for the copy button accelerator key, because Alt+C makes a Windows error ding. * Switch to innerText from innerHTML where possible. * Remove invalid language attribute on body element in message.html per review. * Solve for MSHTML bug in the use of aria-labelledby. * Close the MSHTML Window if args wasn't provided for some reason.
|
Clone of the PR nvaccess/nvda#17018 |
![[development] Korbit - AI Mentor](https://avatars.githubusercontent.com/in/334508?s=40&u=8e7ea115878aacba56242d7ac87807ea98eac709&v=4){kind=avatar}
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
1 similar comment
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
WalkthroughThe changes involve enhancements to the user interface across multiple files, focusing on improving the functionality of message dialogs. Key updates include the addition of "Close" and "Copy" buttons in various UI components and modifications to function signatures to accommodate new parameters. These changes aim to provide a more interactive and user-friendly experience when managing messages and interactions within the application. Changes
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 Documentation and Community
|
![development-korbit-ai-mentor[bot]](https://avatars.githubusercontent.com/in/334508?s=60&v=4){kind=small}
| windll.mshtml.ShowHTMLDialogEx( | ||
| gui.mainFrame.Handle, | ||
| moniker, | ||
| HTMLDLG_MODELESS, | ||
| byref(dialogArgsVar), | ||
| DIALOG_OPTIONS, | ||
| None, | ||
| ) |
There was a problem hiding this comment.
It's recommended to add error handling for the windll.mshtml.ShowHTMLDialogEx call. If this operation fails, it could lead to unexpected behavior or silent failures. By catching potential exceptions and providing user feedback, we can improve the reliability of the browseableMessage function. Consider wrapping the call in a try-except block:
try:
windll.mshtml.ShowHTMLDialogEx(
gui.mainFrame.Handle,
moniker,
HTMLDLG_MODELESS,
byref(dialogArgsVar),
DIALOG_OPTIONS,
None,
)
except Exception as e:
log.error(f"Failed to show HTML dialog: {e}")
_warnBrowsableMessageComponentFailure(title)
returnThis change will ensure that users are informed if the browseable message cannot be displayed due to this specific failure.
Chat with Korbit by mentioning @development-korbit-ai-mentor, and give a 👍 or 👎 to help Korbit improve your reviews.
|
Mapping from the Github issue to the LangSmith prompt that generated the issue: None: https://smith.langchain.com/o/c92ee716-8a66-4de0-a133-524e36845f3f/projects/p/bb4140d7-b5d4-4198-8f37-41c91ccecea9/r/c8d0501e-5c73-4d64-baf1-7dd82f1b2021?poll=true |
|
System configuration at time of review: |
|
@coderabbitai full review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
user_docs/en/changes.md (2)
27-27: Replace the placeholder with the correct issue or PR number.The entry uses
#placeholder, which should be replaced with the actual issue or PR number for accurate tracking.* The Report link destination, Character formatting information, and Speak selection dialogs, now include "Close" and "Copy" buttons for user convenience. (#14641, @XLTechie)
63-63: Replace the placeholder with the correct issue or PR number.The entry uses
#placeholder, which should be replaced with the actual issue or PR number for accurate tracking.* `ui.browseableMessage` may now be called with options to present a button for copying to clipboard, and/or a button for closing the window. (#14641, @XLTechie)
| function onCopyButtonPress() { | ||
| // Copy code came from http://www.codestore.net/store.nsf/unid/DOMM-4QHQE8/ | ||
| var rng = document.body.createTextRange(); | ||
| rng.moveToElementText(messageDiv); | ||
| rng.scrollIntoView(); | ||
| rng.select(); | ||
| var success = rng.execCommand("Copy"); | ||
| rng.collapse(false); | ||
| rng.select(); | ||
| copyStatusDiv.innerHTML = '<p id="copyStatusText" style="font-weight:bold; text-align:center;"></p>'; | ||
| if (success) { // Notify the user about the copy result | ||
| copyStatusText.innerText = args.item('copySuccessfulAlertText'); | ||
| } else { | ||
| copyStatusText.innerText = args.item('copyFailedAlertText'); | ||
| } | ||
| }; | ||
| // Time out the user alert message | ||
| setTimeout(function() { | ||
| copyStatusDiv.innerHTML = ''; | ||
| }, copyStatusTimeout); | ||
| } |
There was a problem hiding this comment.
Consider modernizing the copy functionality.
The current implementation uses createTextRange, which is specific to older versions of Internet Explorer. Consider using the Clipboard API for broader browser support.
| function onCopyButtonPress() { | |
| // Copy code came from http://www.codestore.net/store.nsf/unid/DOMM-4QHQE8/ | |
| var rng = document.body.createTextRange(); | |
| rng.moveToElementText(messageDiv); | |
| rng.scrollIntoView(); | |
| rng.select(); | |
| var success = rng.execCommand("Copy"); | |
| rng.collapse(false); | |
| rng.select(); | |
| copyStatusDiv.innerHTML = '<p id="copyStatusText" style="font-weight:bold; text-align:center;"></p>'; | |
| if (success) { // Notify the user about the copy result | |
| copyStatusText.innerText = args.item('copySuccessfulAlertText'); | |
| } else { | |
| copyStatusText.innerText = args.item('copyFailedAlertText'); | |
| } | |
| }; | |
| // Time out the user alert message | |
| setTimeout(function() { | |
| copyStatusDiv.innerHTML = ''; | |
| }, copyStatusTimeout); | |
| } | |
| function onCopyButtonPress() { | |
| try { | |
| navigator.clipboard.writeText(messageDiv.innerText).then(() => { | |
| copyStatusText.innerText = args.item('copySuccessfulAlertText'); | |
| }, () => { | |
| copyStatusText.innerText = args.item('copyFailedAlertText'); | |
| }); | |
| } catch (err) { | |
| copyStatusText.innerText = args.item('copyFailedAlertText'); | |
| } | |
| setTimeout(function() { | |
| copyStatusDiv.innerHTML = ''; | |
| }, copyStatusTimeout); | |
| } |
|
/review |
PR Reviewer Guide 🔍
|
|
/review |
Link to issue number:
Fixes #14641
Addresses #16995
Addresses #16996
Summary of the issue:
In #14641 @Qchristensen reported that some users are confused by browseableMessages, and their lack of definite closure mechanisms.
In the conversation, it was pointed out that in some cases, a user might also desire a copy button.
During work on the PR (originally #16369), it was requested that the copy button be given an accelerator key.
Description of user facing changes
Added copy and close buttons to some browseableMessages, and the capability to add them to others.
Description of development approach
Scripting.Dictionaryto carry arbitrary query-string-equivalent style parameters to themshtmlinstance behindbrowseableMessage. Thus, it is now possible to pass in values for two new buttons, and various translatable messages, without any contortions.message.html, and eventually settled on one which displayed the two buttons side-by-side, under a separator. If neither button's label is supplied, the div containing the HR and buttons remains hidden.Ctrl+Shift+C, and the "Close" button byEscape.Ctrl+Shift+Cis indicated to the user via an accessibility label.preelement, without causing extra blank lines to appear for each blank line.preis created, assigning to it by.innerText, results in the same.white-space: pre" (or any of its variants), and then assigning through.innerText.<pre>element, to the div's.innerHTMLmethod, as was originally done in #12005.ui.browseableMessage, every other line is empty nvaccess/nvda#16995 (comment), changing various things in the MSHTML virtual buffer code, but got nowhere. This needs further exploration. by someone with more familiarity with screen layout's interaction with NVDAHelper.Miscellaneous additional development items
browseableMessageis called on a secure screen.wx.browseableMessage, this will return immediately, instead of stranding the user in a blank window with no obvious close mechanism. This is not logged or reported to the user directly.Testing strategy:
.innerHTML.Known issues with pull request:
Ctrl+Shift+Cis not an ideal key to activate the copy button, and it can not be changed by translators.html.escape(). I don't believe security is degraded by this PR, but it is not improved, either.Code Review Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Description by Korbit AI
What change is being made?
Enhance
ui.browseableMessageto support optional "Copy" and "Close" buttons, and integrate these buttons into the Report link destination, Character formatting information, and Speak selection dialogs.Why are these changes being made?
These changes improve user convenience by allowing users to easily copy message content to the clipboard and close the message window directly from the dialog. This enhancement addresses user feedback requesting more interactive and accessible message dialogs.