nvda icon indicating copy to clipboard operation
nvda copied to clipboard

Selectively register for UIA events by default on SV2

Open codeofdusk opened this issue 2 years ago • 12 comments

Link to issue number:

Partial mitigation for #11002. Follow-up of #11214.

Summary of the issue:

NVDA 2020.3 introduced support for selective registration for UIA events based on the currently-focused provider, but this is disabled by default due to bugs in the Windows 10 task manager and emoji panel.

Description of how this pull request fixes the issue:

Convert the selective UIA registration option to a ternary setting. By default, enable it on Windows 11 Sun Valley 2 (22H2).

Testing strategy:

Enable by default in master snapshots to allow for wider testing.

Known issues with pull request:

Do any plugins depend on global registration?

Change log entries:

=== Bug Fixes ===

  • Improved performance and stability in in Microsoft Visual Studio, Windows Terminal, and other UI Automation based applications on Windows 11 Sun Valley 2 (version 22H2). (#11077, #11209)
    • Selective registration for UI Automation events and property changes now enabled by default.

Code Review Checklist:

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

codeofdusk avatar Feb 01 '22 19:02 codeofdusk

There is a known issue that task manager is misbehaving when selective registration is on. I guess we'll have to take that for ok or fix it somehow.

LeonarddeR avatar Feb 03 '22 08:02 LeonarddeR

@leonardder - you are referring to #11599, correct?

I searched NVDA issues for "selective registration"

@codeofdusk - do you mind investigating how this PR effects #11599, #11354, #11109, #8624?

It looks like this will improve the latter two, but make behaviour worse for the first two.

seanbudd avatar Feb 09 '22 03:02 seanbudd

Hi, note that enabling selective UIA event registration will break Windows 10 emoji panel support (does not affect Windows 11) due to the fact that emoji panel interface is an overlay that does not take system focus at all. Thanks.

josephsl avatar Feb 09 '22 03:02 josephsl

  • #11599: Can still reproduce.
  • #11354: cannot reproduce.
  • #11109 and #8624: more specific instances of #11002 (which is improved).

codeofdusk avatar Feb 09 '22 07:02 codeofdusk

Hi, note that enabling selective UIA event registration will break Windows 10 emoji panel support (does not affect Windows 11) due to the fact that emoji panel interface is an overlay that does not take system focus at all. Thanks.

@codeofdusk - merging this is definitely blocked by the above, and if a fix for #11599 is possible.

I suggest we try and find a mechanism to disable this for the Windows 10 emoji panel. If we can disable this on an appModule level, the same could be done for task manager (and Firefox, if it is still necessary).

Another path is to create a third state for the setting, "Windows 11 only" which only enables this on Windows 11. We can probably suppress the Task Manager verbosity depending on if the setting is enabled or disabled.

seanbudd avatar Feb 09 '22 23:02 seanbudd

Another path is to create a third state for the setting, "Windows 11 only" which only enables this on Windows 11.

@seanbudd This would require a config spec schema upgrade. If we can fix #11599 I think that could be reasonable.

codeofdusk avatar Jul 20 '22 07:07 codeofdusk

Actually, it looks like I can no longer reproduce #11599 at least on Nickel. I'll proceed with the config spec upgrade.

codeofdusk avatar Jul 20 '22 07:07 codeofdusk

Can we use the new feature flag approach while at it?

LeonarddeR avatar Jul 20 '22 07:07 LeonarddeR

Can we use the new feature flag approach while at it?

I'm not sure. This seems closer to the "diff algorithm" or "Windows Console support" option (choice of two approaches or an "automatic" option that lets NVDA decide based on runtime environment) than an opt-in/opt-out feature with set NVDA developer default.

codeofdusk avatar Jul 20 '22 07:07 codeofdusk

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/h8vib6yqhoc9k0wt/artifacts/output/nvda_snapshot_pr13297-25948,f2926f5a.exe

See test results for failed build of commit f2926f5ac2

AppVeyorBot avatar Jul 20 '22 09:07 AppVeyorBot

Given the uncertainty around #11599 and the emoji panel, I'd like to limit this to SV2 as I know that everything works as expected.

Blocked by #13867.

codeofdusk avatar Jul 29 '22 08:07 codeofdusk

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • PASS: Lint check.
  • FAIL: System tests. See test results for more information.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/hoeadogw3b345f3b/artifacts/output/nvda_snapshot_pr13297-26046,5874a772.exe

See test results for failed build of commit 5874a772e9

AppVeyorBot avatar Jul 29 '22 09:07 AppVeyorBot

#13867 has been merged, so this PR is no longer blocked.

CC @seanbudd.

codeofdusk avatar Aug 15 '22 07:08 codeofdusk

@codeofdusk can you update the PR description? it appears out of date

seanbudd avatar Aug 16 '22 00:08 seanbudd

@seanbudd What do you mean? It looks up-to-date to me.

codeofdusk avatar Aug 16 '22 00:08 codeofdusk

@codeofdusk the summary of the issue reads as if the bugs are still an issue. Can you add that these bugs are fixed in Windows 11 SV2?

seanbudd avatar Aug 16 '22 00:08 seanbudd

@seanbudd For some reason, adding your type hints to the config upgrade step seems to prevent NVDA from starting on my machine.

codeofdusk avatar Aug 16 '22 00:08 codeofdusk

@seanbudd Do you think it's alright to keep the config spec upgrade and behaviour change in one PR, and in the event of revert follow the process outlined in the known issues? Or should I separate them similar to UIA console?

codeofdusk avatar Aug 16 '22 00:08 codeofdusk

@codeofdusk - that would be ideal, but I believe there is less risk of reverting for this PR

seanbudd avatar Aug 16 '22 00:08 seanbudd

@seanbudd Done. I'll make a follow-up once this is merged.

codeofdusk avatar Aug 16 '22 00:08 codeofdusk

  • PASS: Translation comments check.
  • PASS: Unit tests.
  • FAIL: Lint check. See test results for more information.
  • PASS: System tests.
  • Build (for testing PR): https://ci.appveyor.com/api/buildjobs/b5ua8o0phn02c83v/artifacts/output/nvda_snapshot_pr13297-26179,5caf543b.exe

See test results for failed build of commit 5caf543b4c

AppVeyorBot avatar Aug 16 '22 01:08 AppVeyorBot

Thanks @seanbudd. I've opened #14018.

codeofdusk avatar Aug 16 '22 03:08 codeofdusk