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

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

Open
jcsteh opened this issue May 1, 2020 · 15 comments · Fixed by #11214 · May be fixed by #12622
Open

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

jcsteh opened this issue May 1, 2020 · 15 comments · Fixed by #11214 · May be fixed by #12622

Comments

@jcsteh
Copy link
Contributor

jcsteh commented May 1, 2020

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
Copy link
Member

feerrenrut commented May 1, 2020

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
Copy link
Collaborator

leonardder commented May 1, 2020

@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
Copy link
Contributor

codeofdusk commented May 1, 2020

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

@jcsteh
Copy link
Contributor Author

jcsteh commented May 1, 2020

@leonardder
Copy link
Collaborator

leonardder commented May 23, 2020

@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 #11077 (comment), 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
Copy link
Contributor Author

jcsteh commented May 24, 2020

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
Copy link
Collaborator

leonardder commented May 25, 2020

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
Copy link
Collaborator

josephsl commented May 25, 2020

@leonardder
Copy link
Collaborator

leonardder commented May 25, 2020

what about JAB?

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

@leonardder
Copy link
Collaborator

leonardder commented May 25, 2020

@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
Copy link
Contributor

codeofdusk commented May 25, 2020

leonardder/nvda@c0f4791 does not appear to resolve #11002.

@leonardder
Copy link
Collaborator

leonardder commented May 26, 2020

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
Copy link
Contributor

codeofdusk commented May 26, 2020

or implement event limiting as well.

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

@leonardder
Copy link
Collaborator

leonardder commented May 26, 2020

We already enable event coalescing by default.

@codeofdusk
Copy link
Contributor

codeofdusk commented Jul 6, 2020

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

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