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
Use ui.message more, capitalize some messages and docstrings #5557
Conversation
…em. Also add missing translator comments and do some tiny script doc and capitalization fixes. Question: should the messages in the Firefox, Thunderbird and Outlook app modules also be brailled? re nvaccess#5547
…ode name in braille instead of only speaking it. Fixes nvaccess#5239.
With regard to the only instance of speech.speakMessage in core.py @derekriemer wrote:
This is correct, braille is initialized a few lines down. I assume we probably want to preserve this order. |
I assume, then, that the majority of our messages start with a capital letter? (I suspect this is true also, but I'm not certain and you've been looking at this more recently than I.)
I believe the reason we don't do this is precisely because of other languages. My understanding is that in some languages, you can't just go "input help %s" and expect it to be correct for both on and off. @josephsl might be able to comment more on this.
It might be worth splitting that out into a separate pull request after this one is merged so it's easier to take these separately if we wish. My feeling is that if we're going to have to do it at some point, it doesn't matter when; now or later is the same. :)
This sounds like a horrible hack to me. If we're worried about braille, we should also consider Speech Viewer, for example. Also, there may be cases where we explicitly don't want the first letter capitalised. I think if we're going to do this, we should just do it properly. :)
Yes, we do. We hope that message doesn't show up too often these days :), but the whole point is that it needs to be as early as possible to be of any use and braille can take longer to initialise than speech (e.g. Bluetooth displays). |
The idea behind that code (which doesn't work in newer versions of Firefox AFAIK) is to speak a message when a document is being loaded so the user knows what's going on. My concern is that it could get rather annoying if that hangs around on the display even though the document has finished loading. In contrast, the message happens quite quickly for speech. I think it could be useful, but it would require additional code to dismiss any display message when the document finishes loading, which requires further thought; I don't quite know where we'd put this. |
Hi, |
|
|
Thinking about the point @josephsl made re messages for object navigation. At first glance the easiest way to suppress certain braille messages depending on what braille is tethered to is to give ui.message two optional parameters
Or some variation of that. Of course you can also select either ui.message or speech.speakMessage in each script, but that adds quite a few lines of code all solving the same problem. |
Hmm. That depends whether you consider this to be too ugly:
I guess that is pretty ugly. It's kinda cool, though. :) It just seems weird to me that ui.message, which is meant for output agnostic messages, should take arguments indicating what output methods should be used. It's like "okay, let's introduce some abstraction for messages. Hmm, now let's allow the caller to disable the abstraction." |
|
Hi, I would vote for a separate call to braille.handler.message for these messages to show that ui.message will not solve all problems and to contextualize some messages. Cheers, Joseph From: Davy Kager [mailto:notifications@github.com] Hmm. That depends whether you consider this to be too ugly: — |
Another thought:
Would this be too ugly too? I don't want to make people reading this code go 'Wut?' but I also don't like repeating the same if-statement over and over. The idea is that the rule for brailling these messages depends on the focus mode and applies to both review and object navigation, though I'm not sure if brailling "top" and "bottom" all the time when tethered to review would get excessive. |
Some commands, such as script_review_top and script_review_bottom speak a message when the action is performed. This is not currently brailled. But should it even be spoken? I'm led to believe that speech users like brevity. If you use one of these scripts and hear speech you know that the command worked. Do you first need to hear "top" or "bottom"? In braille this wouldn't be efficient because you'd have to wait a bit before seeing the actual top line in review mode. Is this standard behavior for all (virtual) navigation commands? |
Hmm, they're both pretty horrible. :)
True enough. I'm not against having a reviewMessage utility function somewhere. If it's only ever going to be used by globalCommands, it should go in that module, but if we think it might get used elsewhere, it could go in the review module.
I think that would get rather annoying, since it's not as "quick" in braille as it is in speech.
I agree it's superfluous, though interestingly, no one has ever complained. I believe this was modelled on other screen readers (ASAP, Speakup) that use this set of numpad review commands. At this point, I think I'd prefer to leave speech alone, but I certainly agree brailling this would be irritating. |
Are you planning to do the reviewMessage change as suggested in my last comment? Also, this now has conflicts. :( I'll try to keep on top of this one so it doesn't constantly bitrot. |
Apart from the conflict and the question around the review message stuff, this all looks good to me. |
Yes, I was planning to add the reviewMessage function, probably in the review module because globalCommands currently only has commands and no auxiliary functions. Should have time this week. From: James Teh [mailto:notifications@github.com] Apart from the conflict and the question around the review message stuff, this all looks good to me. — |
…s a message if braille is tethered to review. Use this new function in globalCommands.
I ended up putting reviewMessage in the ui module, because it avoids some extra imports in other modules and because it goes well with browseableMessage. |
Hi, I think there are messages that should be brailled regardless of tether value, including “no focus” and so on (unless others object to this). One suggestion: if you don’t mind doing this, can you update the copyright header at the top of the ui module? If not, let us know if I or someone should do it. From: Davy Kager [mailto:notifications@github.com] I ended up putting reviewMessage in the ui module, because it avoids some extra imports in other modules and because it goes well with browseableMessage. — |
So you'd say braille movement messages such as "Top" only if tethered to review, and all others regardless of tether mode? What about the activate message (e.g. NVDA+enter)? I changed this to use the new ui.reviewMessage but am having second thoughts. Probably a sign that I should quit and go celebrate Christmas. :) |
Hi, I think NvDA+ENTER message could be brailled (if needed). As for review top and bottom, I’d say if only tethered to review. As for having second thoughts, I think sometimes, a good rest helps, especially when you are writing a major feature change. I do agree that, because of hlidays, it’s better to rest. Merry Christmas. From: Davy Kager [mailto:notifications@github.com] So you'd say braille movement messages such as "Top" only if tethered to review, and all others regardless of tether mode? What about the activate message (e.g. NVDA+enter)? I changed this to use the new ui.reviewMessage but am having second thoughts. Probably a sign that I should quit and go celebrate Christmas. :) — |
Haha, fair enough. I'll definitely look at the complete diff again after the holidays. |
One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message? |
Hi, I think there’s no need for this, as braille output already provides all the necessary information. From: Davy Kager [mailto:notifications@github.com] One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message? — |
Not all, though of course I can’t think of a scenario where it doesn’t right now. But there have been cases where speech appeared to provide more info with this command than was visible in braille. I’ll revisit this if/when I have something more substantial. From: josephsl [mailto:notifications@github.com] Hi, I think there’s no need for this, as braille output already provides all the necessary information. From: Davy Kager [mailto:notifications@github.com] One more thing I thought of: should NVDA+tab also show the output in braille? The information it provides can be very useful, but maybe it's too verbose for a quick message? — — |
Ideally, it should. It actually does report more than normal focus reporting (in either braille or speech). For example, it always reports the selected/not selected state if the item is selectable, even if it wouldn't normally be included in a focus report. It also reports roles (e.g. list item, table cell, menu item, unknown) and states (e.g. focused, invisible) which would otherwise be silenced. Seeing the full role and state names (as spoken) could also be useful for braille users who aren't yet familiar with the abbreviations. Ideally, we'd do this for the navigatorObject_current script as well. Unfortunately, this "ideal" thing isn't so simple to do because speech.speakObject is used to do this and that speaks the message itself rather than returning a string. speech.speakObject does some stuff for editable text that can't be returned in a simple string, but that stuff isn't needed for braille. speech.speakObjectProperties/speech.getSpeechTextForProperties could be refactored a bit so you could get a string, but having to call those twice isn't terrific, although caching mitigates that a bit. I guess we could call it the second time only if braille is enabled.
I don't think so. If you scroll braille, the message timer gets reset, which means if the message is long, users should have time to read it. |
Fixed another merge conflict. |
|
… only spoken are now brailled as well. Also, fix inconsistent capitalisation and add translator comments for some messages. Fixes #5557.
Incubated in 55a554d. |
I don't think so. If you scroll braille, the message timer gets reset,
Websites: email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu |
I think there isn't a good heuristic for this. Some text is easier to read than others, and sometimes you are only interested in the first few words of a long line. Maybe it depends more on the category of text, i.e. 'No next heading' is expected on a webpage and doesn't need to stay 4 seconds whereas a status bar probably needs closer inspection. But there are no good rules to follow here. The solution seems to be to set a long timeout and dismiss messages manually, which is easy enough. |
Speaking of 'no next heading', those messages still aren't capitalized. Did we decide to exclude/postpone that or did I miss something? |
I didn't exclude any of the patch, so I guess it was missed or somehow
got excluded during a conflict resolution a while ago...?
|
Either that, or I skipped it because there are many ‘no next’ and ‘no previous’ and I wasn’t sure if adding a capital would cause all of them to be marked as needing new translations. From: James Teh [mailto:notifications@github.com] I didn't exclude any of the patch, so I guess it was missed or somehow — |
Yes, changing any of those would result in the message needing to be
re-translated.
|
The commit above contains fixes for all the capitalization and punctuation inconsistencies I could easily find. It is quite a diff, which is the reason for not including it in the original PR. Most of it is very repetitive: 'no next', 'no previous' or on/off toggles. |
Related issue: #5547
Some notes: