nvaccess / nvda

NVDA, the free and open source Screen Reader for Microsoft Windows
Other
2.06k stars 625 forks source link

Restrict most event processing to objects of interest to the user #11077

Open jcsteh opened 4 years ago

jcsteh commented 4 years ago

problem

Some web apps, such as Twitter and Slack, fire a lot of events, most of which aren't things we need to report to the user. For example, when you focus the message list in Slack, it changes the name for every item in the list and adds the Message actions grouping to every item in the list. This results in noticeable performance problems. On my system with Firefox Nightly, focusing the Slack message list with 32 items in the list takes > 700 ms to respond. This is quite a bit faster in Chrome, so clearly there's some work to do in Firefox (and I'm looking into that as well). However, processing less of these events makes things significantly faster (~500 ms or less with my prototype).

We've had similar problems with UIA. For example, 1Password fires many property change events when you switch items in the list (#10508).

High Level Solution

NVDA already drops events for background windows, etc. However, there can be a lot of events even in the foreground window that aren't important to the user. For example, a nameChange on an object that isn't focused and isn't in the focus ancestry isn't reported in any way. Previously, these events were processed, NVDAObjects were instantiated for them and the events were queued, but they simply did nothing when they were finally handled by the NVDAObject.

To address this, I propose that NVDA only process most IAccessible events for the focus, focus ancestry, navigator object and desktop object. For UIA, we could do the same, but it probably makes sense to only listen for events on specific objects; see below.

Issues

I've prototyped two versions of this for IAccessible.

  1. Do all of this in IAccessibleHandler. Problems:
    • This doesn't integrate well with eventHandler.requestEvent. If a plugin requests an event, it will get dropped before shouldAcceptEvent is called. And we can't do this stuff after shouldAcceptEvent because it will return True for cases we want to block.
    • We'd need a completely separate implementation for UIA; we can't use any common code.
  2. Integrate it into shouldAcceptEvent. Pass raw API event params to shouldAcceptEvent and let it ask the relevant NVDAObjects whether the event is for them.
    • The problem is that for UIA, it makes more sense to just only listen for events on relevant objects, rather than listening for all events and dropping the ones we don't care about. In contrast, we can't specify specific objects for IAccessible.

It's really hard to come up with a framework that works well for both. On the other hand, as can be seen from the complexity of shouldAcceptEvent, duplicating all of the logic between IAccessible and UIA wouldn't be sustainable.

I think addressing this for both APIs is probably going to involve a hybrid approach. We'll need some common logic in eventHandler, but this will need to be split up so we can call different pieces in different places. For example, we need to be able to separately ask "did a plugin request this?" and "is this event for the foreground window?", rather than this all being handled in one call. We'll also need to be able to ask "what objects are of interest to the user?", since IAccessible and UIA will handle that differently (filtering vs selective registration).

@michaelDCurran, @feerrenrut, thoughts appreciated. I can push both of my prototype patches somewhere if you're interested (draft pull requests perhaps?), but I didn't want to spam with in-progress work without asking/discussing first.

feerrenrut commented 4 years ago

There is some ongoing work in the same area: "External win event limiter #10556" I'll be looking at this again shortly. When I do I'll take a look at this issue as well.

LeonarddeR commented 4 years ago

@jcsteh: I think a related approach, though a bit controvercial, could imply move most of the now time consuming logic to c++ code. I think that's the intend of #10556 as well, and therefore I really dig @feerrenrut's initiative to investigate this further within the scope of that work.

codeofdusk commented 4 years ago

Why not both – move this code to C++ (#10556) and add additional filtering for events from objects not in focus?

jcsteh commented 4 years ago

There are certainly good reasons to move win event listening and filtering code to C++. On the other hand, that in itself won't solve the problem here. The time consuming part of processing win events (in this particular case) is not the listening and filtering, but more the instantiation of NVDAObjects and the contention on the main thread caused by queuing them as NVDA events. And that part has to be done within NVDA core, since we're talking about NVDAObjects.

LeonarddeR commented 4 years ago

@jcsteh wrote:

I can push both of my prototype patches somewhere if you're interested.

If that's not too much difficulty, if you could just share the branches you created that would be interesting to look at. May be it inspires me or others.

As you also wrote in https://github.com/nvaccess/nvda/issues/11077#issuecomment-622562779, the object instantiation is the part that slows things down. I can think of several ways to limit this for UIA. For example, several event handlers in de UIAHandler still create a new NVDAObject when compareElements returns True. There's a comment that says compareElements can return True even when the elements are not equal, but that comment is over 9 years old and I wonder whether that might have been fixed since then.

jcsteh commented 4 years ago

I've prototyped two versions of this for IAccessible.

1. Do all of this in IAccessibleHandler.

See the iaccSpecificObjEventsIAccessibleHandler branch of my fork.

2. Integrate it into shouldAcceptEvent.

See the iaccSpecificObjEventsEventHandler branch of my fork.

LeonarddeR commented 4 years ago

Thanks for this! Here are some thoughts again.

1. Do all of this in IAccessibleHandler. Problems:

   * This doesn't integrate well with eventHandler.requestEvent. If a plugin requests an event, it will get dropped before shouldAcceptEvent is called. And we can't do this stuff after shouldAcceptEvent because it will return True for cases we want to block.

I think the way how eventHandler.requestEvent is used should be reconsidered anyway. For UIA, it doesn't make much sense as many applications have only one or a few windows (with same window class, of course). For WPF, Window Classes are generated dynamically, which makes scripting for event requests pretty hacky. In most situation, requesting an event will apply to all events of that name in the app.

   * We'd need a completely separate implementation for UIA; we can't use any common code.

I don't think that's a big problem, though more work. If UIA would only register for specific events for specific objects by default and eventHandler.requestEvent would add an UIA event handler under the hood, I think that's fine and the most efficient.

2. Integrate it into shouldAcceptEvent. Pass raw API event params to shouldAcceptEvent and let it ask the relevant NVDAObjects whether the event is for them.

   * The problem is that for UIA, it makes more sense to just only listen for events on relevant objects, rather than listening for all events and dropping the ones we don't care about. In contrast, we can't specify specific objects for IAccessible.

Though I'm an advocate of the smallest UIA event handler registration possible, I guess this could be a major improvement, as much less dropped events are queued to the events queue.

I think addressing this for both APIs is probably going to involve a hybrid approach.

Agreed. As per my points above, I'd suggest:

  1. Going with option 2
  2. update eventHandler.shouldAcceptEvent and eventHandler.requestEvents to take a hashable data class with event params.
  3. Keep listening for all UIA events for now, but ensure that eventHandler.shouldAcceptEvent returns False for many more of them.
  4. Eventually strip down default UIA event registration and create an UIA specific extension to eventHandler.requestEvents that does appropriate UIA event registration.
josephsl commented 4 years ago

Hi, what about JAB? I think Leonard is right when we consider long-term implications (2021 and beyond) and ideally make things API agnostic (although IAccessible handler method is easier to read). For UIA, one possible thing to try is letting NVDA consider framework ID’s (WPF, Direct UI, XAML, etc.). Thanks.

LeonarddeR commented 4 years ago

what about JAB?

That event handling is pretty old and doesn't use eventHandler.shouldAcceptEvent at all, it seems.

LeonarddeR commented 4 years ago

@jcsteh: in the iaccSpecificObjEventsEventHandler on my fork I extended on your second approach for UIA. I will test the test case for #11109 tomorrow, as I need to spend lots of time in the nuget package manager. We could also try #11002, but I doubt whether that's fixed by this prototype. Same might apply to #9465

codeofdusk commented 4 years ago

leonardder@c0f4791 does not appear to resolve #11002.

LeonarddeR commented 4 years ago

Ugh, this doesn't fix #11109 either. I think this proves that we really need to limit registration to only the necessary cases at the UIA level or implement event limiting as well.

codeofdusk commented 4 years ago

or implement event limiting as well.

Wouldn't event coalescing help with this, at least in the #11002 case?

LeonarddeR commented 4 years ago

We already enable event coalescing by default.

codeofdusk commented 4 years ago

I think it's worth reopening this until a similar solution for IA2 is merged.