Skip to content

more useful text content is shown in braille#15731

Merged
seanbudd merged 7 commits into
nvaccess:masterfrom
burmancomp:brailleMoreUsefulTextDisplayed
Nov 7, 2023
Merged

more useful text content is shown in braille#15731
seanbudd merged 7 commits into
nvaccess:masterfrom
burmancomp:brailleMoreUsefulTextDisplayed

Conversation

@burmancomp

@burmancomp burmancomp commented Nov 2, 2023

Copy link
Copy Markdown
Contributor

Link to issue number:

fixes #15605

Summary of the issue:

Although object contains useful text, and it is navigable with review cursor, all these objects are not handled as navigable objects in braille.

Description of user facing changes

Users who use merely braille can read content of more objects.

Description of development approach

Modified braille.NVDAObjectHasUsefulText function.

Testing strategy:

Tested in sasm 3.14.0 editor help document.

Known issues with pull request:

none at this moment

Code Review Checklist:

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

@burmancomp

Copy link
Copy Markdown
Contributor Author

@Emil-18 could you test how this works?

@Emil-18

Emil-18 commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

As far as i know, this works, but shouldn't it check if the objects text info is NVDAObject text info, and if so, return False, because the NVDAObject text info is showing properties that will be shown in braille regardless?

@LeonarddeR

Copy link
Copy Markdown
Collaborator

I think I prefer this to be part of _get__hasNavigableText as suggested earlier by @Emil-18.
Probably the most effective way to fix #15605 and other instances of this issue is is expanding hasNavigableText to return True when the object uses DisplayModelTextInfo.

@burmancomp

Copy link
Copy Markdown
Contributor Author

I think I prefer this to be part of _get__hasNavigableText as suggested earlier by @Emil-18. Probably the most effective way to fix #15605 and other instances of this issue is is expanding hasNavigableText to return True when the object uses DisplayModelTextInfo.

There is condition handling DisplayModelTextInfo in NVDAObjectHasUsefulText. I do not know what should be added to _get__hasNavigableText for displaymodel.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Ah you are actually correct. That puzzles me why this does not work automatically then. I'll investigate.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

I looked into the SASM editor, and there, __get_hasNavigableText returns False. On first thought, I'd say that's wrong, but thinking about it more and more, I don't think so. An UIA object with a TextPattern doesn't necessarily have to be navigable.
On second thought, I think it would indeed be better to extend NVDAObjectHasUsefulText. It can safely return True when the object is an instance of NVDAObjects.behaviors.EditableTextBase.

@burmancomp

Copy link
Copy Markdown
Contributor Author

@Emil-18 told that problem was in help window (when pressing f1). But are there more problems with sasm?

One solution to this help window issue would be that __get_hasNavigableText returns True when readonly is in states (I think I tested it some days ago). If object state is read only why could not it be read? But I do not know if there were some side effects.

@Emil-18

Emil-18 commented Nov 4, 2023

Copy link
Copy Markdown
Contributor

The sasm help window can be read, but what if there are other instances of this problem where the control isn't read only? I also think it is better to check if obj is an instance of EditableTextBase, instead of checking if it is read only

@burmancomp

burmancomp commented Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

Thank you.

When using NVDAObjects.behaviors.EditableTextBase is there need for current additional conditions? Is this code then redundant:

if ( textInfoObj.text != obj.name and textInfoObj.text != obj.description and textInfoObj != obj.TextInfo ):

Why not use editableText.EditableText instead of NVDAObjects.behaviors.EditableTextBase?

@burmancomp

Copy link
Copy Markdown
Contributor Author

This should fix original issue. Do you think there are some side effects?

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This llooks good to me.

@burmancomp

Copy link
Copy Markdown
Contributor Author

Thank you @LeonarddeR

@Emil-18 do you agree?

@Emil-18

Emil-18 commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

I agree

@burmancomp

Copy link
Copy Markdown
Contributor Author

It should solve #15755 also.

@Emil-18

Emil-18 commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

As it is now, it doesn't solv #15755, because it is unrelated. The reason that NVDA don't update braille is because it thinks the two FakeTableCell objects is equal, and thus thinks it should move the review cursor instead of the navigator object. A way to solv it could be to implement _isEqual on FakeTableCell and return self.columnNumber == other.columNumber

@burmancomp

Copy link
Copy Markdown
Contributor Author

When using object navigation braille display was updated. But no problem, if you prefer, I remove that part.

@burmancomp

Copy link
Copy Markdown
Contributor Author

Thank you @Emil-18
lines related to #15755 removed.

I really recommend that you start soon with your pull requests.

@burmancomp burmancomp marked this pull request as ready for review November 7, 2023 21:21
@burmancomp burmancomp requested a review from a team as a code owner November 7, 2023 21:21
@burmancomp burmancomp requested a review from seanbudd November 7, 2023 21:21

@seanbudd seanbudd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @burmancomp

Comment thread source/braille.py Outdated
@seanbudd seanbudd merged commit 1c24290 into nvaccess:master Nov 7, 2023
@nvaccessAuto nvaccessAuto added this to the 2024.1 milestone Nov 7, 2023
@Emil-18

Emil-18 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

@burmancomp When you tested to see if #15755 was fixed, did you try to open the add-on store, focus on an add-on in the list, object navigate to the first child, and then object navigate to the next sibling?

@burmancomp

Copy link
Copy Markdown
Contributor Author

@burmancomp When you tested to see if #15755 was fixed, did you try to open the add-on store, focus on an add-on in the list, object navigate to the first child, and then object navigate to the next sibling?

I suppose yes.

I tested also with _isEqual, and it worked.

@burmancomp burmancomp deleted the brailleMoreUsefulTextDisplayed branch November 8, 2023 21:21
@Emil-18

Emil-18 commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

@burmancomp Ok. After reviewing the code more thoroughly, I see why it works, but I don't think that is the best approach for fixing #15755, because FakeTableCell.basicText is equal to what the review cursor can read, and because of this, FakeTableCell objects has no reason to be reviewable in my opinion. Plus that the user has to scroll back to see information about role/column number/heading/etc. I am going to try to create a pull request that fixes #15755, but as of now, when I try to run scons.bat to run NVDA from source, I get the following output
Ensuring NVDA Python virtual environment
call py -m SCons
scons: Reading SConscript files ...
Warning: Building with 1 concurrent job while 8 CPU threads are available. Running SCONS with the parameter '-j8' may lead to a faster build.

scons: warning: MSVC version '14.3' working host/target script was not found.
Host = 'x86', Target = 'arm64'
Visual Studio C/C++ compilers may not be set correctly
File "C:\Users\e-hes\OneDrive\Skrivebord\nvdaSRC\nvda\SConstruct", line 208, in
scons: done reading SConscript files.
scons: Building targets ...
MIDL.EXE /nologo /arm64 /tlb build\arm64\ia2.tlb /h build\arm64\ia2.h /iid build\arm64\ia2_i.c /proxy build\arm64\ia2_p.c /dlldata build\arm64\ia2_data.c build\arm64\ia2.idl
'MIDL.EXE' is not recognized as an internal or external command,
operable program or batch file.
scons: *** [build\arm64\ia2.tlb] Error 1
scons: building terminated because of errors.
Deactivating NVDA Python virtual environment. I don't know how to fix it. Because of this, I don't think I can test my code before creating the pull request

@burmancomp

Copy link
Copy Markdown
Contributor Author

I suppose your approach is better.

I would recheck environment:

https://github.com/nvaccess/nvda/blob/master/projectDocs/dev/createDevEnvironment.md

@Emil-18

Emil-18 commented Nov 10, 2023

Copy link
Copy Markdown
Contributor

@burmancomp I am 99% sure that I have all dependencies. I am going to look at it again later to day. Could you test this, and see if it fixes #15755?
https://github.com/Emil-18/nvda

@seanbudd

Copy link
Copy Markdown
Member

You may need to do a scons --clean source && scons --no-cache source to clear cached build files

@Emil-18

Emil-18 commented Nov 11, 2023

Copy link
Copy Markdown
Contributor

@seanbudd I did that, but now I get this error instead
Ensuring NVDA Python virtual environment
call py -m SCons
scons: Reading SConscript files ...
Warning: Building with 1 concurrent job while 8 CPU threads are available. Running SCONS with the parameter '-j8' may lead to a faster build.
scons: done reading SConscript files.
scons: Building targets ...
link /nologo /incremental:no /WX /subsystem:windows,6.03 /release /OPT:REF /export:DllGetClassObject,private /export:DllCanUnloadNow,private /export:GetProxyDllInfo,private /manifest:embed /manifestinput:build\x86\IAccessible2proxy.manifest /dll /out:build\x86\IAccessible2proxy.dll /implib:build\x86\IAccessible2proxy.lib rpcrt4.lib oleaut32.lib ole32.lib /PDB:build\x86\IAccessible2proxy.dll.pdb /DEBUG build\x86\ia2_i.obj build\x86\ia2_p.obj build\x86\ia2_data.obj
LINK : fatal error LNK1104: cannot open file 'LIBCMT.lib'
scons: *** [build\x86\IAccessible2proxy.dll] Error 1104
scons: building terminated because of errors.
Deactivating NVDA Python virtual environment
Thanks for the suggestion though

@burmancomp

Copy link
Copy Markdown
Contributor Author

I have also problems with visual studio 2022 environment. I have started discussion in nvda developers mailing list. I suggest you join to that list.

I had also similar error, and error likely has something to do with spectre mitigation (related to some vulnerabilities I suppose). Installing spectre mitigation components seem to manage this error but what exactly should be installed, I do not know.

Ensure with git status that there are no deleted files. If there are, undelete them.

Maybe you are after that in the same situation with me. I am getting now error "LINK : fatal error LNK1104: cannot open file 'msvcprt.lib'"

I have reported that error to developer list. I considered if some part/parts of environment might have references to visual studio 2019, and that error would be due to outdated references but this is just consideration.

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Please stop discussing a pr after it is merged. If there are any issues caused by a pr, a new issue should be opened instead.

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

Labels

None yet

Projects

None yet

5 participants