Give ui.browseableMessage the ability to show copy and close buttons after the message, and add buttons to some instances (2nd try) #17018
Conversation
…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.
XLTechie
changed the title
XLTechie
changed the title
WalkthroughThe recent changes enhance the user interface by adding interactive "Close" and "Copy" buttons to various dialog boxes, improving accessibility and user experience. Additionally, new key handling functionality allows users to easily close dialogs and copy text, while updated messaging functions provide clearer user notifications. These improvements aim to make the application more intuitive, particularly for users relying on assistive technologies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dialog
participant UI
User->>Dialog: Open Dialog
Dialog->>UI: Display message with Close and Copy buttons
User->>Dialog: Press Escape
Dialog->>UI: Close dialog
User->>Dialog: Click Copy button
Dialog->>UI: Copy content to clipboard
UI->>User: Show copy status message
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 Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…d, and it doesn't. This reverts commit f58e85f.
|
@CyrilleB79 Here is a new version of this PR, which should address most of your concerns. |
|
@seanbudd I've added a detailed answer to the description. |
|
All seems nice, either with or without screen layout. Only one remaining point: Also another point but it's not worth blocking the PR for this. The shortcut |
|
@CyrilleB79 Thank you for catching that oversight. An easy fix, which should be in the next build. |
|
@seanbudd Can you say why this is milestoned for 2025.1 instead of 2024.4? I don't care particularly, but I had intended to work on the NH3 integration after this was merged, and before 2025.1. |
See test results for failed build of commit 3dc5643057 |
seanbudd
added
the
conceptApproved
|
@XLTechie - we try not to introduce new/risky features in the weeks before the first beta, it was unlikely that this would be merged before the first beta |
|
@seanbudd Updated this to master. Note that the "Changes" section was missing from the changelog, and I have had to add it in this. |
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.
During 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
ui.browseableMessage--namely adding aScripting.Dictionaryto carry arbitrary query-string-equivalent style parameters to themshtmlinstance behindbrowseableMessage, 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.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.Testing strategy:
.innerHTML.ui.browseableMessage, every other line is empty #16995). This can apparently only be achieved, by passing the message to the div using.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.Ctrl+Shift+Cis reported, no matter the state of "Report object shortcut keys" in "Object Presentation" settings. This is, AFAIK, unavoidable.html.escape(). I don't believe security is degraded by this PR, but it is not improved, either.Code Review Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation