Fast browse mode v2

[mltony]

This PR is work in progress and is not ready for review. I'm publishing it for now just for the reference. I will send an explanation email to nvda-devel mailing list in just a few minutes.

Edit by @LeonarddeR: I embedded the mentioned explanation e-mail below

E-mail

Hello NVDA devs and especially Michael,

I am trying to work on fast browse mode PR (a.k.a. focus follows browse
mode off).
Just a quick recap: In v1 of this PR there were some  bugs related to
the fact that some logic was asynchronous and it relied on gainFocus
event being received from the operating system. However in some cases
for unknown reason operating system wouldn't send this event, causing
bugs that are hard to deal with, such as inability to activate form
elements on web pages.
I am now working on v2:
#9360
The code is by no means ready to be reviewed, but it's just for the
reference.
The main change in v2 is replacing asynchronous logic with synchronous
logic there, and in general it works, but there is one new bugI'm facing
that I would like to discuss and maybe ask for some help.

Basic description of the bug:
When unchecking a checkbox on a web page, its new state is not being
announced. The bug only reproduces for unchecking - that is checking
previously blank checkbox works as expected.
Here are the concrete steps to reproduce the bug.

1. Enter Fast browse mode by pressing NVDA+num row 8.
2. In Chrome or Firefox navigate to
   https://www.ironspider.ca/forms/checkradio.htm
3. Check the first 3 checkboxes on the page
4. Go back to the first check box
5. Press space to uncheck it. The check box will be successfully
   unchecked, but the new state will not be announced immediately.

I spent some time debugging this , and here are my findings so far.
Announcing state change works like this. First
speech.speakObject(obj,controlTypes.REASON_ONLYCACHE) is called to cache
the previous state. In traditional browse mode this function is called
when user navigates to that checkbox, and therefore system focus is
switched to that checkbox as well. This happens in
BrowseModeDocumentTreeInterceptor.event_gainFocus.
Then, assuming that user presses space bar on a checkbox,
BrowseModeTreeInterceptor._activatePosition is triggered, which
activates this checkbox. Shortly, in response to that we receive a state
changed event, that is processed in NVDAObject.event_stateChange(),
which in turn calls speech.speakObjectProperties(self,states=True,
reason=controlTypes.REASON_CHANGE).
So we make two calls to speech module: the first one to cache object
properties, the second one to speak whatever properties have changed.
Cached properties are stored inside NVDAObject, so it is very important
that the same object is passed to both these calls.

Now back to fast browse mode v2. Since we don't update system focus on
every browse mode focus change, we need to update system focus right
before activating the checkbox. This is accomplished in
BrowseModeTreeInterceptor.maybeSyncFocus(). And in the same function,
right after I call obj.setFocus(), I also call
speech.speakObject(obj,controlTypes.REASON_ONLYCACHE).
However, here is the problem. When we receive state changed event, the
function NVDAObject.event_stateChange is called on a different NVDA
object instance. So we completely miss the cached state.
When I say a different NVDAObject instance, I only mean a different
python object. So there are apparently (at least) two NVDAObjects, I'll
call them NVDAObject1 and NVDAObject2. Both represent the same checkbox
in the browser, both have the same IA2UniqueID and windowHandle. Yet in
the first call (cacheonly) I see NVDAObject1, and the state changed
event comes in with NVDAObject2.
To illustrate this, here is the example from my logs (slightly formatted):
cacheonly from maybeSyncFocus():
    <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x07A6A370>
    uniqueID = (67548, -33555216)
speakObject on stateChanged
    <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x07A6A1F0>
    uniqueID = (67548L, -33555216)

So the big question I have is why there are two separate NVDAObjects
created for the same checkbox in my scenario? And why they are not
created Without my PR? What is logic behind creating new NVDA Objects? I
did some cursory debugging of that and I saw that most of NVDAObjects
are created using function getNVDAObjectFromEvent() defined in
IAccessible2_init_.py, and there doesn't seem to be any caching logic
there.
It seems there are two potential approaches of tackling this issue.

1. We need to figure out why two separate NVDA objects are created.
2. Potentially another approach is to allow creating multiple python
   NVDAObjects for the same Checkbox (or any other real IAccessible2
   object), and have a properties cache that would take this into account.
   Some kind of weakKeyDictionary might work. Property caching logic in
   speech.py might have to be redesigned though.

And also another thing worth noticing in the logs that I pasted above.
UniqueID is a tuple of WindowHandle and IA2UniqueID . If you looked at
both entries carefully, you would notice that WindowHandle is slightly
different: even though the value is the same, in the first entry it is
int, and in the second entry it is long. This looks very suspicious to
me. Do you think this might be the reason why two copies of NVDAObjects
are created? On the other hand it doesn't explain why two objects are
not created in stock NVDA.

Anyway, this is current state of my second attempt to implement fast
browse mode. Any suggestions will be appreciated!
Best
--Tony

[LeonarddeR]

@mltony: could you please merge master to fix the failing tests?

[mltony]

Leonard, this is a draft pull request, it is not ready to be merged or even reviewed. I only published it as a reference - I was hoping that you or Michael could take a look at it and figure out why multiple NVDAObjects are being created for the same checkbox in browsers - for more details see my thread in NVDA-devel mailing list.

…

--Tony

On 3/21/2019 12:13 AM, Leonard de Ruijter wrote: @mltony <https://github.com/mltony>: could you please merge master to fix the failing tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9360 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AhMRnDS20G5slNoA6kH2gmXbv7orD0ciks5vYzEOgaJpZM4bm8nN>.

[dpy013]

hi
@mltony
could you please merge master to fix the failing tests?

[LeonarddeR]

@mltony: Another kind request to merge master. This ensures that reviewers are able to test your code. We don't have a working snapshot at this moment, so we can't test the code in action, unless running from source.

[LeonarddeR]

First, I know that this is still a draft. However, for the first instance of this pr, I haven't had the time to closely look at it. I'm pretty sure these issues were already there, and I would have commented on these areas of the code if I'd have reviewed it.

[LeonarddeR]

Could you rename focusFollowsBrowse to focusFollowsBrowseMode across the whole code? While it is longer, the code is never referring to browse mode with only the word browse.

[LeonarddeR]

I wonder how often window handles get reused.

Suggested change

	return self.windowHandle
	return (self.processID, self.windowHandle)

[LeonarddeR]

Could you please add uniqueID to the base NVDAObject, along with a doc string saying what it is supposed to be? I guess for the vanilla NVDAObject, hash(self) might suffice.

[LeonarddeR]

@mltony: I've been able to fix the issue as you described on NVDA Devel. Probably not the most elegant solution, however as it only takes two additional lines of code, I think it is by far the most effective.
You noted on NVDA Devel that you currently do not have the time to work on this. I guess we have three options:

1. We will use this pull request for the work, and I will push my changes directly to your branch
2. I will close this one and open a new pull request. This will make it easier to use this pull request as a reference.
3. I can file a pull request against your branch that incorporates the necessary changes. This, however, requires more work on your end, since you have to approve the changes and merge them.

Please let me know whatever you prefer. I personally prefer option 2.

[LeonarddeR]

Here is a try build

[mltony]

@LeonarddeR, Since you prefer #2, please go ahead with it. I am not that familiar with github and it would take me some time to figure out how to grant you permissions to push into my repo. So I prerfer #2 as well. Also could you share with me your branch - I'm curious how you fixed it in just 2 lines.
Anyway, thanks for your help!

[LeonarddeR]

Thanks, I'll close this for now and will discuss with @michaelDCurran how to bring this further. I will open a draft pr for my code changes later this weekend or next week.

In short, I added these lines to event_gainFocus in browseMode:

				if (
					not config.conf["virtualBuffers"]["focusFollowsBrowse"]
					and obj == self._lastFocusableObj
					and obj is not self._lastFocusableObj
				):
					speech.speakObject(self._lastFocusableObj,controlTypes.REASON_CHANGE)

This covers exactly this case. I agree it is not very elegant though, but it has to be discussed with NV Access how to bring this to a success without disturbing future work. Speech refactor and python 3 are really important things that have to be done in the very near term.

[mltony]

Thanks @LeonarddeR, I really appreciate your help!

[LeonarddeR]

You're welcome

[mltony]

@LeonarddeR, I tried your test build and it works beautifully for me. All the fast browse mode functionality is working plus the bug with checkboxes is fixed indeed. Looking forward towards having this PR in the next release of NVDA!