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

Treeview items: NvDA does not report item count and other object navigation oddities due to bad MSAA implementation in some tree views #7041

Open
fernando-jose-silva opened this issue Apr 1, 2017 · 43 comments
Labels
p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority

Comments

@fernando-jose-silva
Copy link

Using the nvda master 13960 and windows 10 15063 when navigating through an ierarchical list, nvda does not correctly count the existing items in the level that is being traversed.
As an example, you can use the windows registry.
Test:
Open windows registry regedit.
Expand the computer level.
Press a down arrow, see that nvda will announce:
Level 1 HKEY_CLASSES_ROOT collected 1 of 1
When it would be expected that the nvda pronounces:
HKEY_CLASSES_ROOT level 1 collected 1 of 5
Since there are 5 items in this level.
I persisted this with other ierarchical lists in windows, as the list where you can choose the events to customize sounds in windows like (open program, close program).

@jcsteh
Copy link
Contributor

jcsteh commented Apr 3, 2017 via email

@feerrenrut
Copy link
Contributor

Is this a regression (is there some past version of nvda where this behaved as expected with the steps to reproduce)?

@fernando-jose-silva
Copy link
Author

Sorry, unfortunately I can not remember from what moment the problem started.
I persisted the problem when I upgraded to windows 10 15063.
He was wearing the nvda next 13949 at the time.

@jcsteh
Copy link
Contributor

jcsteh commented Apr 3, 2017

This issue has existed in the Registry Editor for years now. I wasn't aware it also happened elsewhere. @fernando-jose-silva, have you seen it anywhere else other than the two examples you gave?

Technical: Some findings from a quick investigation:

  • TVM_GETNEXTITEM is returning null.
  • TVM_MAPACCIDTOHTREEITEM does return a value.
  • This does not happen if you use 32 bit regedit (c:\windows\syswow64\regedit.exe).
  • Conclusion: This is probably because we're a 32 bit app, so our SendMessage can only return a 32 bit LRESULT. So, the result we get from TVM_MAPACCIDTOHTREEITEM is probably wrong.
  • Cross-process reading/writing of 64 bit memory #3339 will probably fix this.
  • That said, I'm wondering whether we can just use UIA for 64 bit SysTreeView32 items (though we'll have to find some way to differentiate from the broken commctrl v5 ones). However, 0591187 disabled UIA for SysTreeView32 controls because of the Windows Features treeview in Windows 7. I'm not quite sure what the bug was, though.

@michaelDCurran, any thoughts?

P3 because this does not occur in many places as far as I know. Also, while item counts are useful, they're not essential.

@jcsteh jcsteh added the p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority label Apr 3, 2017
@fernando-jose-silva
Copy link
Author

Thank you very much for your attention.
I have not located any other ierarchical list with this problem, if I locate any other problem I report.

@Adriani90
Copy link
Collaborator

Places where it occurs as well are: CCleaner, Eclipse.

@Adriani90
Copy link
Collaborator

@jcsteh I have tested back until NVDA 2009.1. Until back to 2010.2, NVDA says 1 of 1 although there are more than 1 elements in the tree. In 2010.1 and earlier, NVDA says 0 of 1 everytime. But there is no version where NVDA counts correctly the item in such tree views. Tested with Narator, it works properly.

@ABuffEr
Copy link
Contributor

ABuffEr commented Jan 6, 2019

I generally use master/alpha, and I'm quite sure that for a short period the counting in registry worked well. I cannot provide more details, unfortunately.

@Adriani90
Copy link
Collaborator

Adriani90 commented Jan 6, 2019 via email

@josephsl
Copy link
Collaborator

josephsl commented Jan 6, 2019

Hi,

Update: Testing with 32-bit and 64-bit Registry editor in Windows 10 version 1809 confirmed something I've been meaning to track down for a while: the root of this family of issues is IPC (inter-process communication) issue between a 32-bit screen reader and a 64-bit binary. It was resolved last year when Robert's pull request was merged for a few days, then it disappeared when development branch reorganization took place in July 2018. Regarding Eclipse tree view problem, it is blocked not only by this issue, but also because of JAB.

Can we boost the priority of this and integrate Robert's pull request into Core as soon as possible, provided that there are no issues with that PR?

Thanks.

@Adriani90
Copy link
Collaborator

the pull request from @Robert-J-H is #8190. It seems there is an assertion error. Thanks.

@josephsl
Copy link
Collaborator

josephsl commented Jan 6, 2019

Hi,

Correct, added due to location helper work.

Upon further testing, it appears Robert's pull request won't help here, as list views and tree views are two different controls. Also, for affected tree view items, position info property returns wrong item count (perhaps that might be the key we're looking for).

Thanks.

@Robert-J-H
Copy link
Contributor

Hi
I've studied only the regedit case and I think that there is a solution.
Most of the constants for the TM_GETNEXTITEM message are not working, i.e. they return 0.
Only those with an X in front return a hItem:
X TVGN_ROOT 0x0000
TVGN_NEXT 0x0001
TVGN_PREVIOUS 0x0002
TVGN_PARENT 0x0003
TVGN_CHILD 0x0004
X TVGN_FIRSTVISIBLE 0x0005
TVGN_NEXTVISIBLE 0x0006
TVGN_PREVIOUSVISIBLE 0x0007
TVGN_DROPHILITE 0x0008
X TVGN_CARET 0x0009
X TVGN_LASTVISIBLE 0x000A

But some do work and thus we don't have a 64-bit issue here but rather a poor implementation through MS.

The consequence is that all object navigation won't work.
However, this can all be replaced by the functions provided through accNavigate since those constants work there:

NAVDIR_DOWN
Navigate to the sibling object that is located below the starting object.
NAVDIR_LEFT
Navigate to the sibling object located to the left of the starting object.
NAVDIR_NEXT
Navigate to the next logical object; generally, it is a sibling of the starting object.
NAVDIR_PREVIOUS
Navigate to the previous logical object; generally, it is a sibling of the starting object.
NAVDIR_RIGHT
Navigate to the sibling object that is located to the right of the starting object.
NAVDIR_UP
Navigate to the sibling object that is located above the starting object.

Note that the descriptions here do not correspond to tree views, e.g. Up and Down is what we would use for previous/next.
Thus, a bit of logic and you'll get the right amount of items at a given level.
The ChildIDs are breadth first, i.e. the top level is first filled with ascending IDs, then it goes back to the start, one level deeper.
(The following directions are meant for the navigate constants, not to be confused with the familiar next/previous)
Let's say the ID is 20, then you would search for the parent (NAVIGATE_LEFT) which might yield 4.
Then you navigate to the right (or next, i.e. to the first child) with the 4 as ID.
This might give 16 and voila, you have the current position of the focused item, namely 5.
Similar thing for the total of elements at this level.

  • Navigate down from the parent (the 4) which gives 5, at least for regedit.
    Navigate right/next until you're at the same level as the focused element. This might give 23 and thus the total amount of items is 7 (23-16).

@josephsl
Copy link
Collaborator

josephsl commented Jan 7, 2019

Hi,

This fails on Device Manager, as ID's can be randomly assigned, and MMC (the program that hosts Device Manager) is a 64-bit program on 64-bit Windows.

Thanks.

@Robert-J-H
Copy link
Contributor

Robert-J-H commented Jan 7, 2019 via email

@josephsl
Copy link
Collaborator

Hi all,

As a starting point, I'll create an app module for Registry Editor that'll incorporate the algorithm described by Robert to be used in 64-bit RegEdit.exe. Once we find more tree views with bad MSAA implementation and if we can locate a pattern for them, then we should consider creating a built-in overlay class.

Thanks.

@Robert-J-H
Copy link
Contributor

Other tree views with similar symptoms:

  • Internet Explorer (e.g. History view)
  • CCleaner -> Cleaner
  • Sound settings/profiles

Thanks to @josephsl for taking that over.
I would have assigned myself but that's not possible without write privileges to the repo. GitHub should add a "Volunteer" label for outsiders. ;-)

@Robert-J-H
Copy link
Contributor

Besides, could one correct the title of this issue? Thanks.

@josephsl josephsl changed the title Nvda is not counting items correctly in an ierarchical list Treeview items: NvDA does not report item count and other object navigation oddities due to bad MSAA implementation in some tree views Jan 11, 2019
@josephsl
Copy link
Collaborator

Hi,

I have incorporated the algorithm proposed by Robert in internal descriptors used in object navigation routines (obj.next/previous/parent/firstChild), thus resolving object nav issue. At the moment I'm trying to figure out the efficient way to calculate child count, as there is a noticeable lag when opening a large top-level subkey (hive) such as HKCR (HKEY_CLASSES_ROOT) in 64-bit Registry Editor (that hive alone contains over 5000 subkeys on my computer).

Also, the implementation detail might be changed:

  • Instead of an app module for RegEdit alone, I'll add a more generic IAccessible-based overlay class. This also allows other app modules besides RegEdit to take advantage of the workaround, and eventually will lead to a more generic solution without requiring app modules.
  • With the generic overlay class in place, RegEdit app module will simply identify the offending tree view and let the overlay class take care of this.

Thanks.

@Robert-J-H
Copy link
Contributor

Hi
Have you tried the procedure that I've described earlier?
Basically:
obj.parent.next.firstChild.IAccessibleChildID - obj.parent.firstChild.IAccessibleChildID
if expressed with your high level methods.
For regedit, you could of course cheat by using _winreg:
_winreg.QueryInfoKey( _winreg.HKEY_USERS)[0]`
gives you the number of sub keys.
It should at least be worthwhile for checking our workaround.

@josephsl
Copy link
Collaborator

Hi,

No, obj.parent... method won't work if:

  1. One is located at the root of the tree and one of its immediate children.
  2. If the item next to its parent isn't expanded since IAccessible object resource would be NULL.

Thanks.

@josephsl
Copy link
Collaborator

Hi,

Regarding winreg method (renamed to just winreg in Python 3, by the way), one needs to construct the path to a specific key/subkey by traversing through ancestors. Although it'll work for 64-bit RegEdit, it isn't quite generic enough at this point.

As for my statement on child count inefficiency, childCount descriptor as defined in NVDAObjects.IAccessible.IAccessible will work. This works for small number of children, but it fails for items with hundreds and thousands of children because it needs to build an array of children (very slow in MSAA, faster via UIA but UIA implementation for these tree views is broken in a major way such as not giving us state change information). Also, accChildCount may or may not work because it could return bad child count (overestimating and underestimating in some cases).

Note that inefficient child count for thousands of subitems is not unique to 64-bit Registry Editor: there is a slight lag when obtaining child count for HKCR in 32-bit RegEdit, too (HKEY_CLASSES_ROOT holds information about file extensions and such, and is a completely different hive than HKEY_USERS).

Thanks.

@Robert-J-H
Copy link
Contributor

Robert-J-H commented Jan 11, 2019 via email

@josephsl
Copy link
Collaborator

Hi,

As for parent navigation: a while loop will work (that's what I have tried).

As for a repo for regedit.py, I'm attaching the progress so far. At this point object navigation works (for the most part; there exists a bug where NvDA may appear to freeze when obtaining children of large tree nodes, but it shouldn't cause issues until we get item count sorted).

The progress and the roadmap are as follows:

  1. Object navigation is working. I consider this the topmost priority because we can't deal with other parts of this issue until this fundamental problem is resolved.
  2. More efficient way of locating child count. Only then fixing position info description makes sense.
  3. Position info descriptor fix sees the light of day.
  4. A series of try builds must be created so others can test our work in real life.
  5. Once we locate generic patterns, the new overlay class should become part of NvDAObjects.IAccessible.sysTreeView32.

Note: the attached file is a text file. As far as content is concerned, it is a Python file, so you must rename the file extension.

Thanks.

@josephsl
Copy link
Collaborator

regedit.txt

@Robert-J-H
Copy link
Contributor

Also, there is a struct defined in sysTreeView32 which doesn't seem to be used.

class TVItemStruct(Structure):
	_fields_=[
		('mask',c_uint),
		('hItem',c_void_p),
		('state',c_uint),
		('stateMask',c_uint),
		('pszText',LPWSTR),
		('cchTextMax',c_int),
		('iImage',c_int),
		('iSelectedImage',c_int),
		('cChildren',c_int),
		('lParam',LPARAM),
	]

We should exploit its capabilities.
In its current form, it can only be used for 32-bit though.
For 64-bit, the alignment would be wrong because of the long pointer needed.
And we have probably to embed it in the process with virtual memory management (same as in sysListView32).

@josephsl
Copy link
Collaborator

josephsl commented Jan 11, 2019 via email

@Robert-J-H
Copy link
Contributor

@josephsl
I'm not sure if the extended structure is appropriate.
On one hand, we have this comment in comctrl.h:

// only used for Get and Set messages.  no notifies
typedef struct tagTVITEMEXA {

which sounds right.
But then we have this macro:

#define TreeView_GetItem(hwnd, pitem) \
    (BOOL)SNDMSG((hwnd), TVM_GETITEM, 0, (LPARAM)(TV_ITEM *)(pitem))

which uses the simpler structure. I've also read a notify-message that explicitly uses TVITEMEX (in contrast to the comment).

However, who am I to judge, I will have to try both.
I don't expect any gain for our problem since the cChildren member only says if there are children and not how many, but it might help with other issues such as #6887 (probability very small).

@Robert-J-H
Copy link
Contributor

OK, I've tested the appModule a bit.
It works certainly much better than before.
Some things I've noticed:
The object navigation from Computer to the root window and back is not the same.
(Computer -> Registry Editor <- Window <- Tree Control <- Computer)

The accNavigate with direction right should give us the first child.
However, this doesn't work all the time, the key must have been opened previously at some point. I'm not sure if this is per session, per logon or whatever. It probably makes some sense for the registry which is enormously huge and there is some internal caching and file streaming active behind the scene.
Fortunately, this doesn't interfere with implementing the position info since the list in question is already open.

I've noticed also that Narrator cheats. The position info is only given for short level-groups and not for e.g. file extensions.
Should we adopt this strategy?
Cheers

@LeonarddeR
Copy link
Collaborator

Regarding winreg method (renamed to just winreg in Python 3, by the way), one needs to construct the path to a specific key/subkey by traversing through ancestors. Although it'll work for 64-bit RegEdit, it isn't quite generic enough at this point.

Even if it was generic enough, this is not a pathway I would accept when reviewing.

UIA implementation for these tree views is broken in a major way such as not giving us state change information).

I don't think the UIA implementation is broken. This is most likely caused by #8785. @michaelDCurran and I were aware of this while respectively writing/reviewing this patch, but decided that the advantages of introducing it were much more important than disadvantages like this. I guess you could find the SysTreeView32 entry in the UIA proxyFactoryMapping and ignore it when unregistering win events.

@Robert-J-H
Copy link
Contributor

Even if it was generic enough, this is not a pathway I would accept when reviewing.

It was only meant for checking the results. It is of course not generic since it doesn't handle other tree views.

UIA implementation for these tree views is broken in a major way such as not giving us state change information).

I don't think the UIA implementation is broken. This is most likely caused by #8785. @michaelDCurran and I were aware of this while respectively writing/reviewing this patch, but decided that the advantages of introducing it were much more important than disadvantages like this. I guess you could find the SysTreeView32 entry in the UIA proxyFactoryMapping and ignore it when unregistering win events.

Not being informed about state changes (especially checked/unchecked) is a very dangerous thing IMHO, e.g. when you're cleaning your system.

However, from what I see with e.g. inspect.exe, UIA doesn't give you any advantages for e.g. regedit. I haven't tried the UIA checker from the Windows SDK though.

@LeonarddeR
Copy link
Collaborator

However, from what I see with e.g. inspect.exe, UIA doesn't give you any advantages for e.g. regedit.

Does UIA also expose the wrong information MSAA is suffering from?

@Robert-J-H
Copy link
Contributor

It returns for the most methods Null, e.g. parent, child and so on.
I've not tested individual patterns but there are not a lot anyway (e.g. legacy, scroll pattern).
And I was also not able to create a tree walker on the last UIA element (e.g. "control host" in a explorer Window, it produced only Null pointers.
But that was out of curiosity, I'm no specialist in UIA.

@ABuffEr
Copy link
Contributor

ABuffEr commented Apr 10, 2019

Hi,
I received some recent citations from this issue, so I sadly confirm that bug is stilll here, testing regedit on Windows 10 64-bit with NVDA 2019.1.1rc1.

@Qchristensen
Copy link
Member

Just confirming this issue still exists in both 2019.2.1 and the latest alpha 19128,df0211c4 (7th November 2019).

Also, object navigation does not work in the tree view of the registry editor, it appears as if the treeview is a single object with no objects inside.

@Robert-J-H
Copy link
Contributor

Robert-J-H commented Nov 8, 2019 via email

@Qchristensen
Copy link
Member

Which custom module are you using @Robert-J-H?

@Robert-J-H
Copy link
Contributor

Robert-J-H commented Nov 20, 2019 via email

@amirsol81
Copy link

This is still duplicable with NVDA alpha-20005,14c2e2ec and Windows 10 Pro 64-Bit Version 2004 (OS Build 19041.207). As another example, the RSS feed tree view in Internet Explorer, accessible via Control+G, displays it -- all items there are "1 of 1 level 1" regardless of their position.

@OzancanKaratas
Copy link
Collaborator

We can provide a proxy DLL to call 64-bit UİA in the lib64.

@Robert-J-H
Copy link
Contributor

Robert-J-H commented Aug 3, 2020 via email

@OzancanKaratas
Copy link
Collaborator

Unfortunately no, but I think @michaelDCurran can help.

@ultrasound1372
Copy link

This bug also occurs with the 64-bit, I.E> installed, versions of Team Talk classic, but not the 32-bit ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4 https://github.com/nvaccess/nvda/blob/master/projectDocs/issues/triage.md#priority
Projects
None yet
Development

No branches or pull requests