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

Re-introduce support for reporting line/column/section numbers in MS Word via UIA, using UIA remote ops #13387

Merged
merged 8 commits into from
Feb 24, 2022

Conversation

michaelDCurran
Copy link
Member

@michaelDCurran michaelDCurran commented Feb 23, 2022

Link to issue number:

This reintroduces the commits in pr #13283 which had been reverted in pr #13350. However it also addresses build issues.

Summary of the issue:

PR #13283 introduced support for reporting line/column/section numbers in Microsoft word via UI Automation, by using the UI Automation Remote Operations Library.
However, this PR had to be reverted in #13350 as

  1. UIA would not initialize properly in binary builds due to a missing manifest file. (Improper initialization of UIA during NVDA start #13347)
  2. NVDA failed to build on Visual Studio 2022. Specifically when building the Remote Ops library with msbuild, midl would fail with an error about a system environment variable being missing.
  3. NVDA could no longer be built on Windows 7. (Impossible to build NVDA after introduction of UIA remote library #13346)

Description of how this pull request fixes the issue:

This PR reintroduces all of the original changes, but:

  • setup.py now includes *.manifest files in the lib directory.
  • NVDA again now requires Visual Studio 2019. However, builds will not fail if a newer version of Visual Studio (E.g. 2022) is installed along side 2019. this is managed by setting MSVC_VERSION in scons before it looks for Visual Studio, so that it specifically selects VS 2019 (VC 14.2).
    Although building on Windows 7 could not be fixed, the readme now notes that only building on Windows 10 and higher is supported.

Testing strategy:

Known issues with pull request:

Pr #13033 allowed NVDA to be built with Visual Studio 2022, though keeping VS 2019 as the official version (which is used on Appveyor). This PR again limits building to Visual Studio 2019. Obviously we do wish to support VS 2022 in the future, but as the support was recently introduced, it was not official, and the UIA Remote Ops library does not yet support VS 2022, we had to make the decision to drop this support for now. At least until we have time to find a work around in NVDA or get the underlying issue fixed in the Remote Ops library.
However, unlike before, it is at least now possible to build NVDA with both VS 2019 and 2022 installed at the same time which should solve some of the original issues.
Although NVDA can run on Windows 7 Service Pack 1 and higher, NVDA cannot be built on Windows 7. Windows 10 or higher is now required.

Change log entries:

Already included in pr.
New features
Changes
Bug fixes
For Developers

Code Review Checklist:

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

…t column number and section number (#13283)

Supersedes pr #13149

Summary of the issue:
The following error is in the log when viewing annotations in the NVDA elements list:
```
ERROR - queueHandler.flushQueue (19:47:20.811) - MainThread (9476):
Error in func ElementsListDialog.initElementType
Traceback (most recent call last):
  File "queueHandler.pyc", line 55, in flushQueue
  File "browseMode.pyc", line 1054, in initElementType
  File "browseMode.pyc", line 1074, in filter
  File "NVDAObjects\UIA\wordDocument.pyc", line 139, in label
  File "NVDAObjects\UIA\wordDocument.pyc", line 95, in getCommentInfoFromPosition
AttributeError: 'WordBrowseModeDocument' object has no attribute '_UIACustomAnnotationTypes'
```
This is because getCommentInfoFromPosition tries to fetch custom annotation types off its obj property, which it incorrectly expects to be a UIA NVDAObject. But in this case it is a TreeInterceptor.
PR #13149 was opened to address this by specifically creating a UIA NVDAObject rather than getting it from the position TextInfo. However, Further investigation has found that it is actually not necessary to even support custom annotation types (draft comment, resolved comment) in getCommentInfoFromPosition as:
1. Quicknav / Elements List iteration was never updated to specifically jump to / list those custom comment types. See CommentUIATextInfoQuickNavItem.wantedAttribValues
2. Although the IUIAutomationTextRange implementation in MS Word does return these custom comment types in GetAttributeValue when given UIA_AnnotationTypesAttributeId, the elements returned with UIA_AnnotationObjectsAttributeId only ever return the standard Comment annotation type via their UIA_AnnotationAnnotationTypeId property. In fact for draft comments, no annotation objects are returned at all.
In short, supporting custom comment annotation types in getCommentInfoFromPosition was not necessary in the first place.

Description of how this pull request fixes the issue:
Remove support for custom comment annotation types from getCommentInfoFromPosition. It again only supports the standard comment annotation type.
…ry distribution. Required for Microsoft UI Automation Remote Operations library.
…should not fail if a newer version of Visual Studio (E.g. 2022) is installed along side 2019.
@AppVeyorBot
Copy link

See test results for failed build of commit 0ba8ab6e35

Copy link
Contributor

@lukaszgo1 lukaszgo1 left a comment

Choose a reason for hiding this comment

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

Given that merging this would reintroduce #13346 it would be nice to document in the readme that Windows 7 is no longer a supported platform for building NVDA. Please also document the newly added sub-modules in the readme. While at it it would be good to explain why you're using your own fork of UIA library.

nvdaHelper/UIARemote/UIARemote.cpp Outdated Show resolved Hide resolved
nvdaHelper/UIARemote/UIARemote.cpp Outdated Show resolved Hide resolved
nvdaHelper/readme.md Outdated Show resolved Hide resolved
nvdaHelper/readme.md Outdated Show resolved Hide resolved
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

I have reviewed this PR from 6632275 filtered view.

LGTM, other than fixing the changelog entry. I am assuming the earlier commits from the reverted PR do not need to be re-reviewed.

user_docs/en/changes.t2t Outdated Show resolved Hide resolved
@@ -68,7 +69,7 @@ What's New in NVDA

== Changes for Developers ==
- Note: this is a Add-on API compatibility breaking release. Add-ons will need to be re-tested and have their manifest updated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"a add-on" s/b "an add-on", and "manifest" s/b "manifests". Also, should the first "Add-on" be capitalized?

Suggested change
- Note: this is a Add-on API compatibility breaking release. Add-ons will need to be re-tested and have their manifest updated.
- Note: this is an add-on API compatibility breaking release. Add-ons will need to be re-tested and have their manifests updated.

Co-authored-by: Sean Budd <sean@nvaccess.org>
Copy link
Member

@seanbudd seanbudd left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelDCurran michaelDCurran merged commit 2583f8f into master Feb 24, 2022
@michaelDCurran michaelDCurran deleted the fix_customPatterns branch February 24, 2022 06:43
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 24, 2022
@LeonarddeR
Copy link
Collaborator

AH, never mind my initial comment, it was based on wrong assumptions. I'm sorry, probably wasn't entirely awake yet.

@LeonarddeR
Copy link
Collaborator

I have Word 16.0.14827.20198 installed, yet line number reporting doesn't work for me.

@michaelDCurran
Copy link
Member Author

@LeonarddeR and you're on Windows 11 with use UIA to access MS Word NVDA setting enabled?
This may be an issue with this feature not being necessarily rolled out to all installs of MS Word perhaps.
I could look at providing you with a try build with greater logging to help debug if you like.
Certainly working on my machine though.

@michaelDCurran
Copy link
Member Author

@LeonarddeR Do you see the following lines in your log:

INFO - NVDAHelperLocal (19:43:26.419) - UIAHandler.UIAHandler.MTAThread (15144):
Thread 15144, build\x86\UIARemote\UIARemote.cpp, initialize, 145:
Registered C:\Program Files (x86)\NVDA\lib\alpha-24809,2583f8fe\Microsoft.UI.UIAutomation.dll.manifest

INFO - NVDAHelperLocal (19:43:26.436) - UIAHandler.UIAHandler.MTAThread (15144):
Thread 15144, build\x86\UIARemote\UIARemote.cpp, initialize, 150:
Microsoft.UI.UIAutomation is available

@LeonarddeR
Copy link
Collaborator

Yes, these log lines are in my log and UIA in word is enabled. Some logging might be helpful indeed. Alternatively, may be I should switch to another release channel?

@LeonarddeR
Copy link
Collaborator

This may be an issue with this feature not being necessarily rolled out to all installs of MS Word perhaps.

I think this is the cause indeed. I was on the current branch and now updated to the current (preview) branch, build 16.0.14931.20094. All works fine now.

@LeonarddeR LeonarddeR mentioned this pull request Mar 23, 2022
5 tasks
michaelDCurran pushed a commit that referenced this pull request Mar 24, 2022
Fix-up of #13387
Fixes #13033

Summary of the issue:
Pr #13387 introduced support for custom UIA extensions, there by using the Microsoft UIA Automation remote operations library. This pr disabled building support for Visual Studio 2022. This has now been solved.

Description of how this pull request fixes the issue:
Fixes build on Visual Studio 2022 by ensuring that the windir environment variable is available. This should eventually be fixed in scons itself, though there's a note in their source code stating that adding environment variables to the basic set that's imported from os.environ should be considered very carefully.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants