nvda icon indicating copy to clipboard operation
nvda copied to clipboard

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

Open XLTechie opened this issue 2 years ago • 20 comments

Code complete, but docs pending.

Link to issue number:

Closes #14641

Summary of the issue:

@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.

Description of user facing changes

Added optional copy and close buttons to all browseableMessages. The "optional" part, means that they are optional to the caller of the browseableMessage, not optional to the user.

The text of each button can be supplied at call time, so may be translated.

Description of development approach

  • Thanks to @michaelDCurran's recent browseableMessage change--namely adding a Scripting.Dictionary to carry GET style parameters to the mshtml instance behind browseableMessage, it was possible to pass in values for two new buttons without any contortions.
  • Tried different methods in the JS and CSS of 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.
  • Fixed up some docstring parameter listings to match modern format.
  • Per the issue, modified the Report Link Destination message, to have both a copy button and a close button.
  • Per the issue, added a close button to the Report Character Information message.
  • In both cases, the close button is set to say "Press escape to close", even though the user can just press the button. I think this matches the Jaws experience, though not sure other SRs use these browseables the way we do.

Testing strategy:

  • Created a scratchpad script, to test browseableMessages using both buttons, and a copy button, and a close button separately.
  • After modifying the link destination and character info messages, tested those as well.

Known issues with pull request:

In addition to the others, it was requested that the OCR results window provide a close button. This is more difficult, and I'd rather leave it to a separate PR in case it is not desired after all.

Is it technically API breaking to add optional parameters to an existing function? I don't think so--we've done it before.

Code Review Checklist:

  • [ ] Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • [x] Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • [x] UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • [x] API is compatible with existing add-ons.
  • [ ] Security precautions taken.

Summary by CodeRabbit

  • New Features

    • Added "Copy" and customizable "Close" buttons to message dialogs for enhanced usability.
    • Users can now copy text directly from message dialogs.
  • Documentation

    • Updated user documentation to reflect new "Copy" and "Close" button features in message dialogs.

XLTechie avatar Apr 06 '24 10:04 XLTechie

  • 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

AppVeyorBot avatar Apr 06 '24 10:04 AppVeyorBot

@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.

XLTechie avatar Apr 07 '24 06:04 XLTechie

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

wmhn1872265132 avatar Apr 07 '24 23:04 wmhn1872265132

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.

CyrilleB79 avatar Apr 08 '24 12:04 CyrilleB79

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 https://github.com/nvaccess/nvda/issues/14641#issuecomment-1443490395

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.

XLTechie avatar Apr 08 '24 14:04 XLTechie

@CyrilleB79 did you notice my comment above? https://github.com/nvaccess/nvda/pull/16369#issuecomment-2042895733 I would be interested in your further comments. Note that I have placed the copy button in the character information window, and changed the button to just "Close" for that Window, while leaving it as "Press escape to close" in the link destination window, for the contrast. I'll settle on one based on @Qchristensen's preference before finalizing the PR.

XLTechie avatar Apr 11 '24 08:04 XLTechie

Re all the comments in https://github.com/nvaccess/nvda/pull/16369#issuecomment-2042895733:

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?

CyrilleB79 avatar Apr 15 '24 09:04 CyrilleB79

@CyrilleB79 I just want to say that:

Most dialogs in NVDA that are real dialogs (actually all, that I can think of), have some sort of active close feature, be it an OK button, a Close button, or such. So this is not so abnormal. The average user doesn't know that this is not a dialog. I think that may be where some of the confusion comes from.

XLTechie avatar Apr 15 '24 10:04 XLTechie

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.

Adriani90 avatar Apr 15 '24 10:04 Adriani90

@Adriani90 wrote:

In general this is not a dialog, and in fact it is even not visible on the screen as far as I know.

It is absolutely visible on the screen.

XLTechie avatar Apr 15 '24 10:04 XLTechie

Even the OCR window?Von meinem iPhone gesendetAm 15.04.2024 um 12:46 schrieb Luke Davis @.***>: @Adriani90 wrote:

In general this is not a dialog, and in fact it is even not visible on the screen as far as I know.

It is absolutely visible on the screen.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

Adriani90 avatar Apr 15 '24 10:04 Adriani90

@Adriani90 wrote:

Even the OCR window?

no, but that is a custom dialog, not an mshtml document. Not applicable to this conversation.

XLTechie avatar Apr 15 '24 10:04 XLTechie

Is this ready for review?

seanbudd avatar Jun 16 '24 23:06 seanbudd

@seanbudd No. Back in April, I asked for comments from @Qchristensen prior to code review, as I believe he requested the original feature, which went through a few evolution's in style requested before implementation.

It still awaits Quentin's comments on UX, as I will need to make changes to finalize it thereafter, and adjust documentation.

Especially if he prefers the "copy then close" alteration someone proposed above.

@Qchristensen my questions to you are in https://github.com/nvaccess/nvda/pull/16369#issuecomment-2041339800

XLTechie avatar Jun 19 '24 08:06 XLTechie

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

File Path Change Summary
source/globalCommands.py Updated multiple methods to utilize new parameters in ui.browseableMessage for enhanced user interaction.
source/message.html Introduced onCopyButtonPress() and onCloseButtonPress() functions to manage new button actions.
source/ui.py Modified browseableMessage to accept closeButton and copyButton parameters for improved functionality.
user_docs/en/changes.md Updated documentation to reflect the addition of Copy and Close buttons, outlining enhancements to ui.browseableMessage.

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

Objective Addressed Explanation
Add "press escape to close" to dialogs which may not be obvious to users (#14641)
Include options for a copy button and customizable close button name in message dialogs

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
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[bot] avatar Jun 19 '24 23:06 coderabbitai[bot]

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 - #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.

Qchristensen avatar Jun 21 '24 05:06 Qchristensen

  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/9yx9rs1x6gebx4iy/artifacts/output/
  • CI timing (mins): INIT 0.0, INSTALL_START 1.0, INSTALL_END 0.9, BUILD_START 0.0, FINISH_END 8.3

See test results for failed build of commit ff71fb144f

AppVeyorBot avatar Jun 22 '24 10:06 AppVeyorBot

  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/s1ji3tivw4y6h66s/artifacts/output/
  • CI timing (mins): INIT 0.0, INSTALL_START 1.2, INSTALL_END 1.0, BUILD_START 0.0, FINISH_END 9.3

See test results for failed build of commit 56c24563a5

AppVeyorBot avatar Jun 22 '24 11:06 AppVeyorBot

@Qchristensen, a question:

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.

This should be a normal browse mode select. If you do a select all in browse mode on a web page in FF or Edge, is the same problem extant, if that page has buttons?

It might be possible to detect a select all via javascript, and change the styling of the buttons to make them look selected. But given that this is browse mode, I'm not sure the browser instance even knows about the selection state of various elements.

XLTechie avatar Jun 26 '24 10:06 XLTechie

  • PASS: Translation comments check.
  • PASS: License check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results and lint artifacts for more information.
  • PASS: System tests (tags: installer NVDA).
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/8kjvk7sxok0in5hd/artifacts/output/nvda_snapshot_pr16369-32805,895b63fb.exe
  • CI timing (mins): INIT 0.0, INSTALL_START 1.2, INSTALL_END 1.0, BUILD_START 0.0, BUILD_END 13.0, TESTSETUP_START 0.0, TESTSETUP_END 0.4, TEST_START 0.0, TEST_END 18.4, FINISH_END 0.2

See test results for failed build of commit 895b63fbc0

AppVeyorBot avatar Jul 06 '24 10:07 AppVeyorBot

@Qchristensen, @seanbudd, The AI suggested that we add Close and Copy buttons for some additional uses of browseableMessage. I was aware of those, but had initially planned to do these in a separate PR, so we could get some feedback on whether they should have the buttons, and so rolling them back wouldn't un-do this one. If you would prefer, however, I can do them here.

Effected:
  • Apparently the Poedit AppMod uses one. (@LeonarddeR?)
  • Word and Excel use a few of them.
Detailed file listings:
  • source/appModules/poedit.py: line 30
  • source/NVDAObjects/window/winword.py: line 5
  • source/NVDAObjects/IAccessible/winword.py: line 20
  • source/NVDAObjects/UIA/wordDocument.py: line 5
  • source/NVDAObjects/UIA/excel.py: line 5
  • source/NVDAObjects/window/excel.py: line 10

XLTechie avatar Aug 03 '24 09:08 XLTechie

@Qchristensen, @CyrilleB79, others: from the PR comment:

The "Copy" button can be activated with Alt+C, and the "Close" button by Escape. However these keys are not indicated to the user in any way.

Any ideas what user experience might be desirable here? We could underline the "C" of "Copy", which I believe is the form used for other Alt plus keys, but even if I'm right about that, there's nothing I can do for Escape in that regard.

EDIT: No, we can't, at least not easily. Because the word "copy" is translatable, I can't underline the accelerator. The best I've been able to do, is add it in an accessibility label.

XLTechie avatar Aug 03 '24 10:08 XLTechie

Luke Davis wrote, in part: "Any ideas what user experience might be desirable here? We could underline the "C" of "Copy", which I believe is the form used for other Alt plus keys, but even if I'm right about that, there's nothing I can do for Escape in that regard."

The underlining of the C for Copy on a button is indeed an age-old Windows convention for button controls, and the "Alt +" part goes without saying for most experienced Windows users, particularly those who have a keyboard-centric focus.

Escape has been a "often, but not always" method to close almost any dialog/pop-up you can name, and I have never seen that "officially documented" anywhere. The key name itself explains what it is supposed to trigger, an escape, if such can be triggered by it. I've actually used Escape to close lots of things for decades even when working in "point and click" mode.

It is only reasonable to presume that most users will not be complete Windows neophytes and that certain conventions that have been in use for a very, very long time are already known and understood, including the shorthands to indicate "Alt +" codes for buttons via underlining the character to be struck. It should, of course, also result in an announcement from the screen reader, e.g., "Close button, Alt + C".

britechguy avatar Aug 03 '24 15:08 britechguy

It should, of course, also result in an announcement from the screen reader,

Which, in this case, it won't. Hence my question.

I think I may try putting an aria-label on it. Not sure if MSHTML respects that, but we shall see. Or not see, hopefully.

Also, I have just realized that once the word "Copy" gets translated, Alt+C is probably going to stop making sense. I have an idea on how to deal with that, but it's annoying.

XLTechie avatar Aug 04 '24 03:08 XLTechie

@XLTechie - I'm happy either way with adding more usages of the buttons. Ones that are likely controversial may be best kept to a subsequent PR.

seanbudd avatar Aug 05 '24 01:08 seanbudd

@Qchristensen Can you please take a look at https://github.com/nvaccess/nvda/pull/16369#issuecomment-2191355897, and also comment upon, using the latest build:

  • The alignment of the buttons.
  • The look of the message which appears when the copy button is pressed.

XLTechie avatar Aug 05 '24 07:08 XLTechie

@britechguy, @Qchristensen I can not find a convenient way to underline the "c" in "copy", unfortunately, since it's a translatable string without the ability to change the key when translated. I may work on this in the future, but javascript makes it non-trivial.

I do now have an accessibility label set up to announce the translated version of "Alt+C", along with the button, without actually displaying it. I'll see how well that works when the current build finishes, but it is likely as far as I can take this for now.

XLTechie avatar Aug 05 '24 07:08 XLTechie

I like this pr, but there is a lot of nasty stuff going on because we insist on using an MSHTML based control. I wonder, are you aware of the fact that wx python seems to support the Edge Webview2 control? May be we should consider supporting that, using the old urlmon based control as a fallback, eventually facing it out when dropping support for Windows 8.1?

LeonarddeR avatar Aug 05 '24 19:08 LeonarddeR

@LeonarddeR

I like this pr

Thanks. :)

but there is a lot of nasty stuff going on because we insist on supporting MSHTML.

Tell me about it! This was going to be a quick solve. About 80% of the time I spent on it, was researching workarounds for ridiculous MSHTML edge cases.

I wonder, are you aware of the fact that wx python seems to support the Edge Webview2 control? May be we should consider supporting that, using the old urlmon based control as a fallback, eventually facing it out when dropping support for Windows 8.1?

I thought it might. Investigating switching us over to that has been on my to-do list since I realized 24 hours ago that mshtml only partially supports aria-labelledby. That was the last straw.

But this was pretty much done, so I wanted to ship it before delving into a complete re-engineering. It already took way way longer than I expected because of my schedule, and all the IE compatibility crap.

XLTechie avatar Aug 05 '24 21:08 XLTechie

@LeonarddeR The other thing I would like to do long term, is move many of these use cases out of webviews entirely.

Most of the common ones (link destination viewer, character information, the Office usages) don't need HTML support.

They would be fine with modeless dialogs. But we don't have a standard in core for modeless dialogs, as we do with, for example, gui.message.messageBox.

There is an issue somewhere for that, but once it finally happens I would like to convert most of these to dialogs. We would still have to keep browseableMessage around, of course, for the few cases that don't fit, and add-ons, but most of the time I don't think we need to be using it.

XLTechie avatar Aug 05 '24 21:08 XLTechie