• FAIL: Translation comments check. Translation comments missing or unexpectedly included. See build log for more information.
• PASS: Unit tests.
• PASS: Lint check.
• PASS: System tests (tags: installer NVDA).
• Build (for testing PR): https://ci.appveyor.com/api/buildjobs/6fors0f1o4d2sq3a/artifacts/output/nvda_snapshot_pr16369-31560,29b526c9.exe
• CI timing (mins):
  INIT 0.0,
  INSTALL_START 0.9,
  INSTALL_END 0.8,
  BUILD_START 0.0,
  BUILD_END 25.4,
  TESTSETUP_START 0.0,
  TESTSETUP_END 0.3,
  TEST_START 0.0,
  TEST_END 2.1,
  FINISH_END 0.2

See test results for failed build of commit 29b526c98d

@Qchristensen would you mind trying this build, and commenting on the UX?
(Edit: link updated with June 19 build)

Specifically, try NVDA+k NVDA+k, and NVDA+f NVDA+f, and observe the buttons which appear in each message.
Please consider them in the light of #14641.

I haven't written dev or user documentation yet, but I want to make sure this is going in a reasonable direction.

Also note, that the copy button is optional. Further, note the comment below by @wmhn1872265132, and consider whether that would be a useful alternative.

I prefer to change the behavior of copying to copy and exit, generally speaking once the information is copied the window loses its necessity to exist

I am not convinced of the usefulness of this PR. As an experimented user, the change in this PR is a degradation of my personal experience, due to the "select all" issue, i.e. when selecting all, you select both the content and the control buttons.

But I understand that this PR rather targets beginners. Though, it seems to me that there has not been so many reporting from beginners or teachers with the current browseable message. Escape or Alt+F4 are classical commands in Windows, so I guess many beginners learn it quite quickly in any case. Also aren't touch device users accustomated to go to previous object in fow to reach the close button?

In any case, I guess that teachers (@XLTechie, @britechguy, maybe @cary-rowen) are the best persons to indicate if this PR is useful or not; and I will not go against their choice.

I have various remarks though on this PR:

1. The button containing "Press escape to exit" is a strange UX. Either you put a button (or a link) with only "Exit" or just write "Press escape to exit" in static text, but not both. Did you ever see dialog boxes with a button labeled "Press escape to cancel"?
2. IMO, the user experience should be the same in all browseable messages, at least in NVDA core's ones. I do not know in which case the "Copy" and "Exit" button text should be configurable. Thus, the strings of the buttons should be defined in ui.browseableMessage (at least by default), not in the calling function. If we really want these buttons still to be configurable,maybe a simple True/False parameter would be enough?
3. When you press the "Copy" button, nothing is heard. If we keep this "Copy" button, a speech feedback is at least needed.
4. More specifically, I am not convinced of the "Copy" button. In NVDA 2024.1, we can easily copy the text with control+A and control+C. With this PR we add an additional footer with an "Exit" button which degrade the copy experience. Thus we need to add one more button which loads more this windows. I have the feeling that we have not found the good solution. Is there an alternative solution? E.g. a description of the document labelled "Press escape to exit".

Sorry Luke for this quite negative feedback, but I thought it was worth sharing.

I am not convinced of the usefulness of this PR. As an experimented user, the change in this PR is a degradation of my personal experience, due to the "select all" issue, i.e. when selecting all, you select both the content and the control buttons.

I believe that is why @nvdaes proposed the "copy" button in the original issue. The copy button specifically avoids copying the footer section.
For clarification: what is the purpose of selecting all and copying, if you can use the copy button provided? I think we could put a Alt+c keyboard shortcut on it if needed.

But I understand that this PR rather targets beginners. Though, it seems to me that there has not been so many reporting from beginners or teachers with the current browseable message.

@Qchristensen originally raised this. That was my only context; I am assuming he, as NV Access, had sufficient complaints to open an issue.

I think the debate is supposed to be done in the original issue. Certainly the context is there.

1. The button containing "Press escape to exit" is a strange UX. Either you put a button (or a link) with only "Exit" or just write "Press escape to exit" in static text, but not both. Did you ever see dialog boxes with a button labeled "Press escape to cancel"?

This is a rip-off of Jaws, and in part a response to the request in the original
issue. It would not bother me for this to be just "Close", leaving the escape
part out entirely. This was in part why I requested a pre-review from
@Qchristensen. I also do not care for this text.

The Jaws UI here, I believe, is just a text line saying "Press escape to close", that for some reason acts as a close button if you happen to interact with it. I could have done that here (with a clickable), but I thought people would find that as objectionable as you find this.
Jaws doesn't provide a copy button, and I often see their closing line show up in people's copied text from these Windows, which Jaws uses for more purposes I believe.

2. IMO, the user experience should be the same in all browseable messages, at least in NVDA core's ones.

I agree in general, and can easily do that. At this stage of the draft PR, I wished to demonstrate both possibilities.

However, add-ons may have other requirements, and I didn't want to insist that they have a copy button.

3. When you press the "Copy" button, nothing is heard. If we keep this "Copy" button, a speech feedback is at least needed.

Agreed. Done in a live region.

4. More specifically, I am not convinced of the "Copy" button. In NVDA 2024.1, we can easily copy the text with control+A and control+C. With this PR we add an additional footer with an "Exit" button which degrade the copy experience. Thus we need to add one more button which loads more this windows.

"Copy" exists to enable copying without inclusion of the footer. That may be what you're saying there, the translation is unclear of "which loads more this windows", although I think it means that it adds more content to the window. If that is what you meant, then the reason is to provide a copy function that doesn't capture the footer, which is what I got from #14641 (comment)

I have the feeling that we have not found the good solution. Is there an alternative solution? E.g. a description of the document labelled "Press escape to exit".

"Press escape to exit" could be added to the title bar of every message.

• I'm not sure how useful that is given that some title bars may already be quite long (if they contain the name of a link, for example), and visually I am not sure the title bar is even shown.
• That also still leaves us with the problem of touch users, which @josephsl raised originally.
• Lastly, @michaelDCurran preferred the footer close button in the original issue.

I am not sure there is a "good" solution to this.
Actually, there is: replace everything currently shown in a UI.browseableMessage, with a real dialog in an appropriate WX control. Then provide the close button only. That should solve the select all problem, if it's a read-only editable text field.
ui.browseableMessage is a convenient, but ultimately lazy, way to get a non-modal dialog with HTML side-benefits.

Re all the comments in #16369 (comment):

I have the feeling that we did not investigate enough #14641, i.e. why some people may have difficulty with the browseable message. Are there many such people? What is the profile of these people? Have these people switched recently from Jaws and do they expect the same from NVDA?
@Qchristensen, can you provide answers to these questions?

It seems to me that almost every blind windows user know that pressing Alt+F4 or Escape in any window or dialog allows to close it; a user that would not be aware of this is lacking important information and thus would need extra teaching.

My personal need for select all is only to copy the content. So I agree that the "Copy" button does the same thing.

Why I am a bit reluctant to the change in this PR is that it seems to me a UX degradation. The browseable message becomes heavier (in term of UX) with the Close button, and moreover adding the Close button causes a new issue, which is resolved by adding the Copy button, which makes this browseable message more heavy and complex. But people have not expressed difficulty in copying the content of current version of the browseable message.

Also, I understand why the buttons (presence or not and label) may need to be configurable. However, this configurability will make the experience of browseable message less unified, which is a UX degradation.

The wx dialog is not an option due to the isHtml option, except if you remove this possibility which is not used in NVDA's core in any case.

Bu if I am the only one feeling that it's a UX degradation, go ahead anyway. Especially if @Qchristensen confirms the "go" for this PR.

Thanks for having changed the label of the "Close" button. The speech feedback of the "Copy" button is still needed. And also, what is copied when isHtml is True?

In general this is not a dialog, and in fact it is even not visible on the screen as far as I know. NVDA reports "document" when opening them, and not "dialog" as it does when opening settings dialogs. I agree with @CyrilleB79's doubts on the usefullness of this PR.
I would rather vote to add some lines to the user guide explaining the difference between a dialog and a virtual browseable document.

@XLTechie wrote:

The average user doesn't know that this is not a dialog. I think that may be where some of the confusion comes from.

I think it would make more sense to tell users that there is a difference instead of changing the interface so the users don't notice a difference.

In NVDA terms virtual browseable documents are commonly used in many discussoins, so it is ok in my view not to treat them as if they were dialogs.

Is this ready for review?

Walkthrough

The recent changes enhance user interaction in message dialogs by introducing new parameters to the ui.browseableMessage function, adding "Copy" and "Close" buttons. This improvement aims to make dialogs more accessible and user-friendly, allowing users to easily copy text and close dialogs directly, enhancing overall usability.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant GlobalCommands
    participant UI
    participant MessageHtml
    
    User ->> GlobalCommands: Trigger Report Link Destination
    GlobalCommands ->> UI: Call browseableMessage with new parameters
    UI ->> MessageHtml: Render message with copy & close buttons
    User ->> MessageHtml: Click Copy Button
    MessageHtml ->> User: Handle copy action
    User ->> MessageHtml: Click Close Button
    MessageHtml ->> User: Handle close action

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?

Ok firstly @XLTechie, @Adriani90 & everyone, my apologies for the delay replying. I got behind on issues over the last few months, and I am working on catching up!

Looking at the try build, and having just caught up on all the comments. I like the NVDA+f functionality. Just to save anyone needing to go back and test the try build, the behaviour is:
NVDA+F twice: Dialog has text in a read-only edit, with 'copy' and 'close' buttons. If you press escape, the dialog closes, if you move to the close button and activate it, the dialog closes. There is also an 'X' close button up the top-right for touch users (they could also use the close button).

NVDA+k twice: dialog is essentially the same, but the close button reads "Press escape to close". I think just close is fine here.

Re the question of copying the information, as someone noted, if you press control+a, then control+c, you DO get the button text as well as the formatting (or link) information. Whether that will annoy more users who expect to be able to press control+a then control+c, vs those who instead tab to the copy button and use that is a good question it's probably hard to know without having people test it in the wild.

The original idea behind "Press escape to close" was that it was read out automatically, and from when the dialog did not have any buttons. From memory, I only had one user ask about this, but the confusion seemed like it could feasibly more widespread, so I created the issue - essentially when you press NVDA+f once NVDA reads the information. When you press it twice, NVDA reads it and you can now navigate it, but it may not be 100% clear that you are now in a dialogue. I can say since I created the issue, that I haven't had anyone else come to me confused. Having created the buttons may make it easier to add additional functionality down the track (I can't think of anything else which might be useful to add here). But whether the lack of additionally confused users pushes towards this not being such an important issue to solve at all?

Two additional mostly unrelated visual observations:

1. the dialog which is presented is about three times higher than needed. That is nothing to do with this PR, it's how the dialogs have previously been presented (I created an issue for that - Dialog created when NVDA+f or NVDA+k are pressed twice is much higher than needed #16727)

2. When you do select all, only the text is highlighted with NVDA's focus highlight. A user with enough sight to see that may not expect the button text to be copied as well.

@seanbudd I have updated this and marked it as ready again, pursuant to my comment #16369 (comment)
If you disagree about NH3 being a separate action/API breaking, then we can put it back to draft again and I'll work on that.

Also, I am interested in the directions from which you see an injection threat coming?
Given that any code capable of launching ui.browseableMessage, and populating it with HTML, is also capable of much more effectively accessing the entire user space via Python, I'm trying to figure out what exactly we are protecting against.

Presumably a user could click a link in HTML provided by an add-on, which would then load a secondary malicious page, or could even pull something in via auto-loaded external refs of other kinds. But once the initial HTML is loaded, Python is out of the picture, and obviously we can't sanitize that.
We can only clean the HTML before it gets to the browser, and given anything able to put it in the browser could also unlink the whole filesystem from within Python, or exfiltrate anything via requests, I'm unclear on what door we are closing.

Even if an add-on pushes in a third party link via a refresh directive, short of banning those entirely, we wouldn't be able to do anything about it. And maybe we should ban those entirely, I'm just trying to get a sense of the threat vector.

@XLTechie - I'll need to discuss internally on whether we want to perform the sanitization in this release.

The vector we're concerned about is NVDA translations, as these are fairly unregulated, translation strings are the only "code" included in NVDA without a direct review from NV Access or as a dependency. If NVDA translations can perform RCE, we have a problem.
Considering no NVDA source code uses isHTML currently, this isn't an active vector.
However, if we ever start using isHTML, it becomes an active vector, which is something we want to avoid and prevent from becoming a possibility.

I think we can tackle this in 2025.1 as the sanitization rules might break add-on functionality, and there's no active risk right now.

@XLTechie I'll write up an issue to do this in 2025.1. I think we could make the sanitization rules for this message as part of our public API so add-ons can adjust them as necessary

I've opened #16985

Re the "ding" issue, did you try e.stopPropagation or e.preventDefault in the key handler?