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

Disable UIA text change events outside of Word, Windows Console, and Windows Terminal #14067

Merged
merged 8 commits into from
Aug 29, 2022

Conversation

codeofdusk
Copy link
Contributor

@codeofdusk codeofdusk commented Aug 25, 2022

Link to issue number:

Mitigation for #11002.
Blocking #14047.

Summary of the issue:

UIA textChange NVDA events are seldom (if ever) used outside of a few specific situations, but have an extreme performance impact (see #11002).

Description of user facing changes

Improved performance/less chance of NVDA hanging in UIA applications.

Description of development approach

Explicitly do not process UIA textChange events outside of Windows Console, Terminal, and Word. The eventual end goal is to remove TermControl/TermControl2 from UIAHandler.textChangeUIAClassNames in #14047, which will very greatly improve performance in Windows Terminal. (conhost will remain, as there don't seem to be any plans to add notifications, especially as wt is becoming the default).

I'm very reluctant to add a mechanism by which add-ons/app modules can request textChange events unless someone requests it, especially given #11002.

Testing strategy:

  • Tested in a self-signed build of NVDA over a few days.
  • Tested that conhost and wt remain functional.
  • Removed TermControl from textChangeUIAClassNames and verified that textChange events are not received.

Known issues with pull request:

None known.

Change log entries:

== Changes for Developers ==

Code Review Checklist:

  • Pull Request description:
    • description is up to date
    • change log entries
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • API is compatible with existing add-ons.
  • Documentation:
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • Security precautions taken.

@codeofdusk codeofdusk requested a review from a team as a code owner August 25, 2022 10:13
@codeofdusk codeofdusk requested a review from seanbudd August 25, 2022 10:13
@codeofdusk codeofdusk force-pushed the drop-uia-textchange branch from ec92a87 to 4762bd3 Compare August 25, 2022 10:17
@codeofdusk codeofdusk marked this pull request as draft August 25, 2022 10:36
@codeofdusk codeofdusk closed this Aug 25, 2022
@LeonarddeR
Copy link
Collaborator

Note that you need to add "_WwG" as well since word also relies on text change events

@codeofdusk codeofdusk reopened this Aug 25, 2022
@codeofdusk
Copy link
Contributor Author

@LeonarddeR Done. For some reason, start menu suggestions aren't reading with this PR.

@LeonarddeR
Copy link
Collaborator

May be @josephsl knows a reason for this, though I don't see how textChange events are used there.

@codeofdusk
Copy link
Contributor Author

Never mind, even when running master from source they don't read. I think it's a build environment issue.

@codeofdusk codeofdusk marked this pull request as ready for review August 25, 2022 11:12
@AppVeyorBot
Copy link

See test results for failed build of commit 820eb60520

seanbudd
seanbudd previously approved these changes Aug 26, 2022
@codeofdusk
Copy link
Contributor Author

@seanbudd Why was the build cancelled for this PR?

@codeofdusk codeofdusk force-pushed the drop-uia-textchange branch from e93544d to 6bf9745 Compare August 26, 2022 06:53
@seanbudd
Copy link
Member

@codeofdusk - please don't force push to the branch after approval

@seanbudd seanbudd dismissed their stale review August 26, 2022 06:55

stale after force push

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Aug 26, 2022

@seanbudd OK. I just tried doing that to see if it'd re-trigger the build. There are no code changes.

@seanbudd
Copy link
Member

@codeofdusk - the build was cancelled intentionally. Releases take priority for builds.
You can build and run tests locally.
The build checks are for final checks before merging.

@codeofdusk
Copy link
Contributor Author

codeofdusk commented Aug 26, 2022

Oh OK. Will you restart the build for this PR after the release?

seanbudd
seanbudd previously approved these changes Aug 26, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit 5c330d899c

@AppVeyorBot
Copy link

See test results for failed build of commit 5c330d899c

@codeofdusk codeofdusk changed the title Disable UIA text change events outside Windows Console and Terminal Disable UIA text change events outside Word, Windows Console, and Windows Terminal Aug 28, 2022
@seanbudd seanbudd dismissed their stale review August 29, 2022 01:45

stale, changes pushed

Co-authored-by: Sean Budd <seanbudd123@gmail.com>
@codeofdusk codeofdusk changed the title Disable UIA text change events outside Word, Windows Console, and Windows Terminal Disable UIA text change events outside of Word, Windows Console, and Windows Terminal Aug 29, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit 04337168d0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants