nvaccess / nvda

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

Rely on IA2 caret events for Chrome and Firefox editors #11545

Open LeonarddeR opened 3 years ago

LeonarddeR commented 3 years ago

Is your feature request related to a problem? Please describe.

Currently, we don't rely on the IA2_EVENT_TEXT_CARET_MOVED event to determine when the caret has moved in Firefox and Chrome. This might be the cause for intense lag in VS Code, see #11537 In the past, I did an attempt to enable this behavior, but it causedNVDA to speak the previous line instead of the current line when moving by line, for example. Thanks to the event logging added by @michaelDCurran in #11521, I was able to track down the underlying issue. NVDA is treating the location change event on the object as a caret event. This behavior might have been there from the birth of NVDA's MSAA code.

Describe the solution you'd like

I see several options.

  1. No longer treat locationchange events as caret events, but as a new event locationChange. I wonder whether this could also help with #11286 and #10672. We can then treat location events as caret events in the cases where necessary, e.g. on edit controls like Notepad, avoiding it for IA2 objects that fire IA2_EVENT_TEXT_CARET_MOVED
  2. In processGenericWinEvent, don't treat EVENT_OBJECT_LOCATIONCHANGE as caret event for events emitted from IA2 objects. The problem with this is, how could we find out whether we're dealing with an IA2 object in this case?

Feedback from either @michaelDCurran or @jcsteh would be appreciated :)

akash07k commented 3 years ago

Could be good if the base issue #11533 Could be fixed by this. @leonardder If you send a pull request, then please provide me the test build so that I can test it.

LeonarddeR commented 3 years ago

Alright, this isn't going to help Visual Studio Code, see https://github.com/nvaccess/nvda/issues/11533#issuecomment-687635985

Adriani90 commented 10 months ago

@leonardder I see some Pull requests have been merged both in NVDA and VS Code to handle performance issues in the past years, is this still needed to improve performance further?

Adriani90 commented 4 days ago

@LeonarddeR is there any benefit NVDA would still have from your proposed solution? cc: @seanbudd for priority triaging.

jcsteh commented 4 days ago

Currently, we don't rely on the IA2_EVENT_TEXT_CARET_MOVED event to determine when the caret has moved in Firefox and Chrome.

Ug. I'd forgotten about this.

NVDA is treating the location change event on the object as a caret event.

We only do that for OBJID_CARET, though, so that should only be happening if the system caret moved, which in turn should only happen once the accessibility info is up to date. Maybe Chrome moves the system caret too early, before it updates accessibility? Regardless, we should definitely trust IA2 caret events over system caret location changes.

In the past, I did an attempt to enable this behavior, but it causedNVDA to speak the previous line instead of the current line when moving by line, for example.

@LeonarddeR, do you recall whether this happened in Firefox too? I'd at least like to get this enabled for Firefox, as it would make caret movement slightly faster. And if there are bugs in Firefox preventing that, I'd like to get them fixed.

  1. In processGenericWinEvent, don't treat EVENT_OBJECT_LOCATIONCHANGE as caret event for events emitted from IA2 objects. The problem with this is, how could we find out whether we're dealing with an IA2 object in this case?

When we translate LOCATIONCHANGE to caret, we fire it on the focus. We could return early if isinstance(focus, NVDAObjects.IAccessible.IAccessible) and isinstance(focus.IAccessibleObject, IA2.IAccessible2).

jcsteh commented 4 days ago

Note that caret movement detection using either bookmarks or events is already a lot faster after #14708, so this probably won't make a big difference, but it's still worth investigating, since less querying is always better, especially when running on battery or the like where cross-process calls are slower.

LeonarddeR commented 4 days ago

I think there are some false assumptions in the initial description, probably because things have changed. Here is what I found:

  1. We definitely handle the IA2_EVENT_TEXT_CARET_MOVED event as a caret event
  2. In Firefox, it looks like the location change event comes before IA2_EVENT_TEXT_CARET_MOVED
  3. In FireFox, Caret detection occurs using bookmarks most of the time, regardless whether event handling is enabled.
jcsteh commented 4 days ago
2. In Firefox, it looks like the location change event comes before IA2_EVENT_TEXT_CARET_MOVED

Ah interesting. I can see that in the code. FWIW, we move the system caret immediately before the IA2 caret event is fired, so they should both se the same state; it's just a matter of the ordering.

3. In FireFox, Caret detection occurs using bookmarks most of the time, regardless whether event handling is enabled.

Yuck. That's rather suggestive that win event firing/delivery is rather slow, since Firefox updates the accessibility tree just before it fires the events, but there's probably not much we can do about that.