Skip to content
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

displaying font information in a virtual message will be helpful. #4908

Closed
nvaccessAuto opened this issue Feb 12, 2015 · 36 comments
Closed

displaying font information in a virtual message will be helpful. #4908

nvaccessAuto opened this issue Feb 12, 2015 · 36 comments
Assignees

Comments

@nvaccessAuto
Copy link

Reported by sumandogra on 2015-02-12 07:16
NVDA needs to have A virtual message window displaying all the font information.

With NVDA+F, NVDA gives information about font but there is no way one can read through the font information. This information is helpful for those who have a lot of information related to font important for them during proofreading and formatting a document.
Suggestion: this can be introduced in the form of a dialog that can be closed by just ESC.

@nvaccessAuto
Copy link
Author

Comment 2 by briang1 on 2015-02-13 08:54
Would this not be best done as an add on, at least in the first instance and only put into the core program if it is deemed of use by enough users.
Do people really need all of this in all editor programs, presumably even email

@nvaccessAuto
Copy link
Author

Comment 3 by dineshkaushal on 2015-02-20 10:46
First implementation is ready for trying.

In the current implementation, there is a problem that i am unable to fix. Escape does not work unless you move away from the dialog and come back with alt plus tab.

Checkout branch in_t4908
https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

@nvaccessAuto
Copy link
Author

Comment 4 by nvdakor (in reply to comment 3) on 2015-02-20 11:10
Replying to dineshkaushal:

First implementation is ready for trying.

In the current implementation, there is a problem that i am unable to fix. Escape does not work unless you move away from the dialog and come back with alt plus tab.

Checkout branch in_t4908

https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

Hi,
Wouldn't it be possible to use a read-only text box (perhaps available as part of WXPython) instead of generating an HTML page on the fly? That way it can be closed just by pressing the ESC key.
Thanks.

@nvaccessAuto
Copy link
Author

Comment 5 by leonarddr on 2015-02-20 14:48
It would probably be good to take note of how the virtual revision add-on creates its window. I agree with comment:2 though.

@nvaccessAuto
Copy link
Author

Comment 6 by dineshkaushal on 2015-04-09 13:54
Thanks Mick,
Your solution is working.

I have updated the fix.

Checkout branch in_t4908
​https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

@nvaccessAuto
Copy link
Author

Comment 7 by dineshkaushal on 2015-08-27 13:16
HTML dialog is now working fine. Please check branch in_t4908.

now using os.path.realpath to find the full path of the htmlMessage.html file which is a template.

@nvaccessAuto
Copy link
Author

Comment 8 by dineshkaushal on 2015-09-30 13:07
Implemented all the suggestions provided by Jamie on the email.

==== General ====
Please rename HTMLMessage to htmlMessage wherever it occurs (lower case html at the start). Capitalisation throughout NVDA is somewhat inconsistent, but we generally try to do camel case beginning with lower case for new stuff. The only exception is type/class names which start with an upper case letter.

I have renamed the file to message.html

There is commented out code/junk in several parts of this work, as well as some unnecessary blank lines at the start/end of files. This should be removed. Please make sure you're reviewing your own diffs before submission; this would have been caught in your own review.

Removed

The documentation needs to be updated in both the docstring for the script and the User Guide to specify that pressing report formatting twice displays the information in browse mode.

Updated the docstring. Once it is accepted, will update the user guide as well.

==== HTMlMessage.html ====
The title tag should probably empty. I realise you replace this later, but right now, it's misleading because it makes it seem as if this is an untranslatable message.

Removed

The focus_away function just contains commented out code and should be removed.

Removed

Please use camel case for the function names; e.g. escapeKeyPress.

Corrected

Indentation of the js code is inconsistent and/or non-existent in various places.

Fixed

To be consistent with the rest of the NVDA code, braces should be on the same line as the statement the block is associated with.

Done

In window_onload, you split the args at semi-colons. What if the message contains a semi-colon? The split should only be done for the first semi- colon.

Fixed by adding second arguement as 2 to ensure semicolon is only used once.

message_id should probably be a div (block) instead of a span (inline).

Changed

==== globalCommands.py ====
You've added a UTF-8 BOM at the start of globalCommands.py. This will break the build. Please remove it.

Fixed

I don't think it's useful to display "No formatting information" in a dialog. I realise you're trying to be consistent here--pressing twice should always pop up a dialog--but I think this is going to be more annoying than helpful.

Now pressing nvda + f twice is showing message only when format information is available.

For HTML, I wonder if we should display each bit of the formatting info on a separate line to make it easier to consume; i.e. join with line feed instead of space for HTML. Thoughts?

I tried, but it is not working as it seems that there is only 1 element for format information. I added

etc during join operation.

==== ui.py ====
The general convention in Python is to put standard library imports first, followed by third party package imports, followed by imports for your own project. Again, this is a rule some of our existing code unfortunately doesn't follow.

Fixed

The docstring for HTMLMessage isn't useful to developers wanting to know what the function actually does. It should explain that it displays the message in an HTML document so that the user can read it with browse mode.

Corrected

The function is called HTMLMessage, but it takes a text string. This is incongruous. Should we perhaps rename this to "browseableMessage"?

Changed

There should perhaps be the option of providing HTML input; e.g. isHtml=False keyword argument.

Changed innerText to innerHTML, now html text should work

The title "NVDA Message" needs to be made translatable. Also, although this should be the default, I think it would be good if this could be customised; e.g. title=_("NVDA Message") keyword argument.

Added additional parameter to provide dialog title. I am using font as title for nvda + f, but may be it can be "format".

Please check in_t4908_fresh on https://manish_agrawal@bitbucket.org/manish_agrawal/nvda.git

I have created a new branch from master as earlier branch is 7 month old.

@nvaccessAuto
Copy link
Author

Comment 9 by dineshkaushal on 2015-10-07 10:08
Updated userGuide.t2t for report formatting command

@nvaccessAuto
Copy link
Author

Comment 10 by jteh on 2015-10-26 05:08
Thanks. Review:

GlobalCommands.script_reportFormatting

  • To get each bit of formatting info on a separate line, I suggest adding a separator keyword argument to speech.getFormatFieldSpeech which defaults to CHUNK_SEPARATOR; i.e. separator=CHUNK_SEPARATOR. This way, you can pass separator="\n" to do new lines. If you use the pre tag as I suggest below, new lines should work nicely.

message.html

  • nit: There's still a "saved from url" comment which should be removed.

ui.browseableMessage

  • I think it's important to have a way to explicitly specify plain text or HTML; e.g. isHtml=False argument. The way it is currently implemented, it's possible for an author to inject unwanted HTML by manipulating, for example, the style name. The alternative is to have all callers escape the text in all cases, but i think this is not ideal, especially since plain text will be the most common use case. Perhaps isHtml=False would use a pre tag and the innerText property, whereas isHtml=True would do what you do now.
  • nit: Get rid of the logging calls please.

User Guide

  • A user may not know what "HTML document" means, so just mention it gets displayed in browse mode.

@nvaccessAuto
Copy link
Author

Comment 11 by jteh on 2015-10-26 11:14
One more nit: I think the title for the reportFormatting dialog should be "Formatting", rather than "Font".

@jcsteh jcsteh removed the Untriaged label Nov 13, 2015
jcsteh pushed a commit that referenced this issue Dec 22, 2015
…ation in browse mode so it can be reviewed.

You can now present a text or HTML message to the user in browse mode using ui.browseableMessage.

Fixes #4908.
@nvaccessAuto
Copy link
Author

Incubated in cd5ba70.

@jcsteh
Copy link
Contributor

jcsteh commented Dec 22, 2015

Pull request: #5593.

jcsteh added a commit that referenced this issue Dec 22, 2015
Also, anything presented to the user should be unicode anyway, so change the docstring to require this.
Re #4908. Fixes #5628.
@nvaccessAuto
Copy link
Author

Incubated in 92a1bcd.

@LeonarddeR
Copy link
Collaborator

It slightly puzzles me why this functionality is implemented this way with a HTMl template. Honestly, I prefer an implementation as proposed by Joseph in comment 4

@LeonarddeR
Copy link
Collaborator

Also found a little bug in here:

STR:

  1. Press nvda+f twice to get the formatting info dialog
  2. Press tab
  3. Press alt twice to open and close the menu bar
    Result: An error is generated
    ERROR - unhandled exception (21:15:44): Traceback (most recent call last): File "wx\_misc.pyc", line 1367, in Notify File "wx\_core.pyc", line 16869, in Notify File "core.pyc", line 474, in _callLaterExec File "IAccessibleHandler.pyc", line 820, in _fakeFocus File "NVDAObjects\__init__.pyc", line 259, in objectWithFocus File "NVDAObjects\__init__.pyc", line 189, in findBestAPIClass File "NVDAObjects\__init__.pyc", line 189, in findBestAPIClass File "NVDAObjects\__init__.pyc", line 188, in findBestAPIClass File "NVDAObjects\IAccessible\MSHTML.pyc", line 444, in kwargsFromSuper AttributeError: 'NoneType' object has no attribute 'nodeName'

@jcsteh
Copy link
Contributor

jcsteh commented Jan 3, 2016

It slightly puzzles me why this functionality is implemented this way with a HTMl template. Honestly, I prefer an implementation as proposed by Joseph in comment 4

Using HTML allows for content other than just text; e.g. links, headings, etc. I don't see a disadvantage with this approach using plain text; you can still just press escape to close it.

@dineshkaushal
Copy link
Contributor

I found the cause of the problem. I had set tabindex=-1 for body in message.html, so the focus was not getting there. The fix is to set tabindex=0.

I tried to checkout branch t4908, but could not find it. Making changes in in_t4908 is difficult as you made some changes after taking code from there.

Please suggest what should be done?

@jcsteh
Copy link
Contributor

jcsteh commented Feb 4, 2016 via email

@sumandogra
Copy link

I tested with the test build given with the fix. the issue is with tab and alt key is not present now and is fixed.

jcsteh pushed a commit that referenced this issue Feb 16, 2016
@nvaccessAuto
Copy link
Author

Incubated in 0feb4fe.

@jcsteh jcsteh closed this as completed in 7f87073 Mar 8, 2016
@derekriemer
Copy link
Collaborator

Just a thought. Should we hash and store the hash of the message.html file in the python code? This way it is resistant to tampering. Otherwise, could it be somehow packaged somewhere where someone couldn't mess with the HTML document as easily? My concern (not that big of a deal really) is that someone could replace the message.html file on the fly in someones NVDA install, and then they could cause the message to present interesting things. For a proof of concept, go to NVDA's installation directory, (bad if it is on an external usb drive because this isn't admin privlages required), and add this code.

<script> function windowOnLoad(e){document.body.innerHTML = "proof of concept self distructing evil file.";}
function escapeKeyPress(e){document.location = "http://nvaccess.org";}
</script>

It is not a huge vonerability though.

@jcsteh
Copy link
Contributor

jcsteh commented Mar 9, 2016 via email

@derekriemer
Copy link
Collaborator

Yeah. That's a good point.

@derekriemer
Copy link
Collaborator

Also, the virtual message only uses internet explorer when a link is pressed. Is this known behavior?

@LeonarddeR
Copy link
Collaborator

Found another little bug. When rpessing nvda+f more than twice, multiple formatting windows are opened. Hold instert and press f four times, for example.

@derekriemer
Copy link
Collaborator

In that case, it turns out that NVDA is opening a font window to display
font information (which just happens to be available) for the font
information. This works out because this is a mshtml control (I believe)
and NVDA supports retrieval of font information in those.

On 3/14/2016 9:30 AM, Leonard de Ruijter wrote:

Found another little bug. When rpessing nvda+f more than twice,
multiple formatting windows are opened. Hold instert and press f four
times, for example.


Reply to this email directly or view it on GitHub
#4908 (comment).


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@jcsteh
Copy link
Contributor

jcsteh commented Mar 15, 2016

Also, the virtual message only uses internet explorer when a link is pressed. Is this known behavior?

Ug. That's kind of annoying. I suppose it makes sense; this function actually uses IE behind the scenes to render the HTML. The question is whether there's any way we can force it to use the shell to open external URLs. @dineshkaushal, can you look into this?

Found another little bug. When rpessing nvda+f more than twice, multiple formatting windows are opened. Hold instert and press f four times, for example.

The code displays the window if there was more than 1 press. We should instead explicitly make this 2 presses.

@derekriemer
Copy link
Collaborator

@jcsteh
I believe this also has to do with the fact that font info can appear
for font-info window. To reproduce:

  1. Press nvda+f twice somewhere.
  2. In the window, press NVDA+f twice. It pops up font info for the
    font-info page.
  3. Pressing NVDA+f twice again produces more font-info for the font-info
    window that displays font info for the window that you originally
    requested font-info for.
    (that was a complex sentence, please excuse my non-brevity).

On 3/15/2016 5:00 PM, James Teh wrote:

Also, the virtual message only uses internet explorer when a link
is pressed. Is this known behavior?

Ug. That's kind of annoying. I suppose it makes sense; this function
actually uses IE behind the scenes to render the HTML. The question is
whether there's any way we can force it to use the shell to open
external URLs. @dineshkaushal https://github.com/Dineshkaushal, can
you look into this?

Found another little bug. When rpessing nvda+f more than twice,
multiple formatting windows are opened. Hold instert and press f
four times, for example.

The code displays the window if there was more than 1 press. We should
instead explicitly make this 2 presses.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#4908 (comment)


Derek Riemer
  • Department of computer science, third year undergraduate student.
  • Proud user of the NVDA screen reader.
  • Open source enthusiast.
  • Member of Bridge Cu
  • Avid skiier.

Websites:
Honors portfolio http://derekriemer.com
Awesome little hand built weather app!
http://django.derekriemer.com/weather/

email me at derek.riemer@colorado.edu mailto:derek.riemer@colorado.edu
Phone: (303) 906-2194

@jcsteh
Copy link
Contributor

jcsteh commented Mar 16, 2016 via email

@derekriemer
Copy link
Collaborator

I was just mentioning the fact that it exists because I'm pretty sure that's what you were referencing when you were quoting it. It had looked like you had misquoted it but maybe I misunderstood what you wrote. Sorry
I would also argue that it is not a book as well.

Sent from my heavily encrypted iPhone.
Please disregard errors as this is a smaller device.

On Mar 15, 2016, at 22:12, James Teh notifications@github.com wrote:

Sure, but I don't consider this to be a bug. If you want formatting info
for your formatting info, that's your business. It's a valid (if stupid)
use case. Unless you can give me some reason it isn't valid...

On the other hand, opening another dialog when you press it three times
is not valid, since it's only documented to do it when you press it
twice. It's also more likely to be accidentally triggered due to a
finger slip or the like.

You are receiving this because you commented.
Reply to this email directly or view it on GitHub

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Mar 16, 2016 via email

@jcsteh
Copy link
Contributor

jcsteh commented Mar 16, 2016 via email

@LeonarddeR
Copy link
Collaborator

LeonarddeR commented Mar 16, 2016 via email

@jcsteh
Copy link
Contributor

jcsteh commented Mar 18, 2016 via email

jcsteh added a commit that referenced this issue Mar 18, 2016
… presses; i.e. don't allow the third press, etc. to open another dialog.

Re #4908.
@tspivey
Copy link
Collaborator

tspivey commented Mar 26, 2016

Why does this say formatting 2 or 3 times?

IO - inputCore.InputManager.executeGesture (19:48:49):
Input: kb(laptop):NVDA+f
IO - speech.speak (19:48:49):
Speaking [u'Dark Gray on Black']
IO - inputCore.InputManager.executeGesture (19:48:49):
Input: kb(laptop):NVDA+f
IO - speech.speak (19:48:49):
Speaking [u'Formatting']
IO - speech.speak (19:48:50):
Speaking [u'Formatting']
IO - speech.speak (19:48:50):
Speaking [IndexCommand(1), u'Dark Gray on Black']

@nishimotz
Copy link
Contributor

The 'center' option for HTML dialog is specified, however, the actual dialog position is not center on screen.
"center:desktop" seems work.

diff --git a/source/ui.py b/source/ui.py
index c91d0d0..862c782 100644
--- a/source/ui.py
+++ b/source/ui.py
@@ -23,7 +23,7 @@ import braille
 URL_MK_UNIFORM = 1

 # Dialog box properties
-DIALOG_OPTIONS = "dialogWidth:350px;dialogHeight:140px;resizable:yes;center:yes;help:no"
+DIALOG_OPTIONS = "dialogWidth:350px;dialogHeight:140px;resizable:yes;center:desktop;help:no"

 #dwDialogFlags for ShowHTMLDialogEx from mshtmhst.h
 HTMLDLG_NOUI = 0x0010

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants