-
-
Notifications
You must be signed in to change notification settings - Fork 617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Give ui.browseableMessage the ability to show copy and/or close buttons after the message, and add buttons to two instances #16369
base: master
Are you sure you want to change the base?
Conversation
See test results for failed build of commit 29b526c98d |
@Qchristensen would you mind trying this build, and commenting on the UX? Specifically, try NVDA+k NVDA+k, and NVDA+f NVDA+f, and observe the buttons which appear in each message. 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:
Sorry Luke for this quite negative feedback, but I thought it was worth sharing. |
I believe that is why @nvdaes proposed the "copy" button in the original issue. The copy button specifically avoids copying the footer section.
@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.
This is a rip-off of Jaws, and in part a response to the request in the original 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.
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.
My original reason for that, along with hesitating to insist that add-ons adopt the core phrasing, was because I originally planned to implement this PR just to add the functionality to
Agreed.
"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)
"Press escape to exit" could be added to the title bar of every message.
I am not sure there is a "good" solution to this. |
@CyrilleB79 did you notice my comment above? #16369 (comment) I would be interested in your further comments. |
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? 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 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 |
@CyrilleB79 I just want to say that:
1. 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.
2. The close text does not have to be configurable. I did that for add-ons, but I don't need to keep it. I have already removed it for the Copy button.
3. I didn't yet implement the message when copy is performed, awaiting Quentin's pre-review before spending any more time on this.
|
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. @XLTechie wrote:
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 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.
|
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 wrote:
Even the OCR window?
no, but that is a custom dialog, not an mshtml document. Not applicable to this
conversation.
|
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
browseableMessage
change--namely adding aScripting.Dictionary
to carry GET style parameters to themshtml
instance behindbrowseableMessage
, it was possible to pass in values for two new buttons 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.Testing strategy:
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: