Disable winhooks if screen reader is active #3896

ethindp · 2019-11-14T20:44:04Z

This PR adds a detection mechanism that, while primitive, prevents windows hooking when a screen reader is found to be running when Mumble is executed. This is accomplished by enumerating the process list and checking for a (currently hardcoded) set of (only two right now) screen reader binaries. I know that NVDA (denoted by image name nvda.exe) is affected when Mumble loads winhooks, but I don't know if JAWS (denoted with image name jfw.exe) is. Rather than add every screen reader I know of to this list, I feel it might be better to just add them when we come across them.
Note: I have not built this code yet. I am working on automating the build process since I am unable to do it on my local machine at this time, so expect errors. (I tried not to make any, but you know...) Also, I did my best to mantain formatting, though it wasn't easy. :)

davidebeatrici · 2019-11-14T20:52:25Z

Executables are only detected when Mumble is started, but it's certainly better than not handling them at all.

Aside from that, the code looks good!

I would use CreateToolhelp32Snapshot() instead of EnumProcesses():

mumble/plugins/mumble_plugin_win32.h

Lines 30 to 49 in 2004de9

    
           static inline procid_t getProcess(const wchar_t *exename) { 
        
           	PROCESSENTRY32 pe; 
        
           	procid_t pid = 0; 
        
           	pe.dwSize = sizeof(pe); 
        
           	HANDLE hSnap = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); 
        
           	if (hSnap != INVALID_HANDLE_VALUE) { 
        
           		BOOL ok = Process32First(hSnap, &pe); 
        
           		while (ok) { 
        
           			if (wcscmp(pe.szExeFile, exename)==0) { 
        
           				pid = pe.th32ProcessID; 
        
           				break; 
        
           			} 
        
           			ok = Process32Next(hSnap, &pe); 
        
           		} 
        
           		CloseHandle(hSnap); 
        
           	} 
        
           	return pid; 
        
           }

src/mumble/GlobalShortcut_win.h

ethindp · 2019-11-14T21:00:44Z

Also, I used the Query* process functions because I wasn't sure how to do this with the snapshot API. Is there a way I can call the plugin frim the GlobalShortcut_win.cpp file?

davidebeatrici · 2019-11-14T21:02:56Z

You can use the snapshot functions in GlobalShortcut_win.cpp, you just have to include <tlhelp32.h> instead of <pasapi.h>.

ethindp · 2019-11-14T21:11:43Z

Should I just #include "mumble_plugin_win32.h" from the .cpp file? Or would that break things? Trying to fix the test failures.

davidebeatrici · 2019-11-14T21:13:30Z

No, that's the header we use for plugins, you should include <tlhelp32.h> (a Windows header).

ethindp · 2019-11-14T21:23:29Z

I just pretty much copy-pasted (and then modified) the getProcess() function from the win32 plugin header file. Let's hope the checks pass this time. I'll fix them if they don't. :)

davidebeatrici · 2019-11-14T21:25:00Z

// Disable hooking if a screen reader is running
bool bFoundSr = false;
const QStringList srList = { QLatin1String("nvda.exe"), QLatin1String("jfw.exe") };

const auto snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
if (snapshot != INVALID_HANDLE_VALUE) {
	PROCESSENTRY32 processEntry;
	processEntry.dwSize = sizeof(processEntry);
	auto ok = Process32First(snapshot, &processEntry);
	while (ok) {
		if (stList.contains(QString::fromWCharArray(processEntry.szExeFile))) {
			bFoundSr = true;
			break;
		}

		ok = Process32Next(snapshot, &processEntry); 
	}

	CloseHandle(snapshot);
}

// "winhooks" is a hidden setting to disable hooking
bHook = g.qs->value(QLatin1String("winhooks"), true).toBool() || bFoundSr;

ethindp · 2019-11-14T21:27:25Z

That works too... sorry -- I'm not exactly familiar with this API, so thanks for the help. I'll put that in my function though -- keep all of that code nice and clean.

davidebeatrici · 2019-11-14T21:34:41Z

No problem, you're welcome.

ethindp · 2019-11-14T21:36:15Z

There we go, committed (and credited you of course). Now we wait for these checks to pass and we should be all good to go.

davidebeatrici · 2019-11-14T21:38:34Z

Don't worry about the credits, it's fine.

src/mumble/GlobalShortcut_win.cpp

davidebeatrici · 2019-11-14T21:53:11Z

I assume you're editing the files on GitHub, right?

Let me squash the commits for you.

ethindp · 2019-11-14T21:53:44Z

Sure thing. And I'm editing them on my computer, not on git hub, but I push every time I make an edit. Never squashed a commit before though.
Edit: I cannot type today. 🗡

davidebeatrici · 2019-11-14T21:55:09Z

Oh, sorry, I assumed you were editing the files on GitHub because all commits are signed, but I just noticed it's your own GPG key.

Would you like to squash the commits?

ethindp · 2019-11-14T22:08:05Z

Yes

src/mumble/GlobalShortcut_win.cpp

src/mumble/GlobalShortcut_win.h

davidebeatrici · 2019-11-14T22:13:40Z

Assuming there are 5 commits:

git rebase -i HEAD~5
Change pick to squash (or s) near the commits you want to squash, save and exit from the editor.
Choose a message for the new commit, save and exit from the editor.

ethindp · 2019-11-14T23:16:22Z

Rebased. Was a bit confused so the rebase commit is a bit confusing, but undersand it now. :)

davidebeatrici · 2019-11-14T23:27:54Z

Did you force-push after rebasing the commits? I still see all commits.

About the merge commit: I always recommend to enable autorebase, so that when you pull from a remote Git doesn't create it:

git config --global pull.rebase true

ethindp · 2019-11-14T23:28:36Z

had to pull and then push. The server disallowed me from pushing until I pulled.

ethindp · 2019-11-14T23:29:46Z

Just saw some checks fail. I'll fix those when I get back from dinner.

davidebeatrici · 2019-11-14T23:31:51Z

had to pull and then push. The server disallowed me from pushig until I pulled.

Because when you rewrite history you have to force-push, either with git push --force or with the more recommended git push --force-with-lease.

Just saw some checks fail. I'll fix those when I get back from dinner.

No problem, see #3896 (review).

ethindp · 2019-11-15T00:31:50Z

OK, just pushed new changes, let's see if checks pass this time.

src/mumble/GlobalShortcut_win.cpp

ethindp · 2019-11-15T00:50:34Z

Submitted that fix about 5-6 minutes (or more, dunno) ago. Waiting for checks to pass now.

ethindp · 2019-11-15T00:53:49Z

Working on fixing that failure now.

Krzmbrzl · 2020-01-20T17:06:44Z

Yes - it worked ^^

Note however that while looking through the code I encountered that I think you are doing the opposite of what you wanted to achieve (I think). Please see my code-review for details :)

src/mumble/GlobalShortcut_win.cpp

ethindp · 2020-01-20T18:26:52Z

Yes, that's what I want. :) Thanks for picking that up -- I would've missed it! :)

…

On 1/20/20, Robert Adam ***@***.***> wrote: Krzmbrzl commented on this pull request. > @@ -129,7 +132,8 @@ GlobalShortcutWin::GlobalShortcutWin() #endif { // Hidden setting to disable hooking - bHook = g.qs->value(QLatin1String("winhooks"), true).toBool(); + // Also disable hooking if a screen reader is running + bHook = g.qs->value(QLatin1String("winhooks"), true).toBool() || areScreenReadersActive(); Also if you want to force-disable winhooks if a screenreader is active, you'll have to use ``` bHook = g.qs->value(QLatin1String("winhooks"), true).toBool() && !areScreenReadersActive(); ``` -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #3896 (comment)

-- Signed, Ethin D. Probst

ethindp · 2020-01-20T18:32:04Z

Updated and fixed. Hopefully this works.

… of winhooks if a screen reader was found to be running at app startup

Krzmbrzl · 2020-01-20T19:03:43Z

I squashed the commits together again.

Btw.: What does this change have to do with the overlay? I only see that this is related to shortcuts, but I don't see a relationship to the overlay-feature of mumble 🤔

ethindp · 2020-01-20T19:04:33Z

The global shortcuts files, if I'm not mistaken, deal with the overlay as well, since the overlay uses windows hooks for it to work, no?

Krzmbrzl · 2020-01-20T19:14:40Z

Hm could be. Dunno though. That's something we'll have to ask @davidebeatrici

In the meantime I'd ask you to force-pull (git pull - f) the current state from your branch to your local machine and test whether this now works as expected :)

ethindp · 2020-01-20T19:16:12Z

It should, not sure why tests are failing.

Krzmbrzl · 2020-01-20T19:19:11Z

CI is failing because it has trouble installing dependencies. That happens from time to time and is not related to the code changes.

I'm not referring to whether the code compiles. I'm talking about testing if the code does what it is supposed to do. Compiling is only half the story.
So please compile the code and verify that the hooking is disabled if you are using Mumble with a screen reader (and that it is enabled if opening it without a reader).

ethindp · 2020-01-20T19:25:03Z

That will be kinda hard. I can't test it without a screen reader (as I myself anvisually impaired) and I can't build it on the windows side so will need to build with MinGW. But once I've done that I can test to see if winhooks are disabled when a screen reader is active.

Krzmbrzl · 2020-01-20T19:36:47Z

Ah okay - no problem I guess that testing whether the hooks are disabled if a screen reader is active is enough. And MinGW should be fine :)

ethindp · 2020-01-20T19:43:35Z

It'll take me a bit, but I'll get back to you today.

davidebeatrici · 2020-01-22T20:04:44Z

Thank you very much for pushing this forward!

I confirm that the changes only afflict global shortcuts.

Krzmbrzl · 2020-01-22T20:08:08Z

In that case, I'll adapt the commit message accordingly before this will be merged eventually :)

Krzmbrzl · 2020-01-30T20:57:31Z

@ethindp did you already get to test your changes?

ethindp · 2020-01-31T19:18:53Z

I am not able to right now. MXE takes far too long to build on Linux for me (and when I got all the required packages to build it couldn't find MXE's version of qmake, which apparently wasn't built). I have the mingw-w64 versions of all of that on here but I don't know how to get it to statically link everything. Sorry! I've also come down with the flue recently, so I don't have much energy to do this stuff at the moment.

…

On 1/30/20, Robert Adam ***@***.***> wrote: @ethindp did you already get to test your changes? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3896 (comment)

-- Signed, Ethin D. Probst

Krzmbrzl

Now that I have had a bit more experience with the code base I'm confident that the changes made here should be fine.

Krzmbrzl · 2020-05-26T18:37:11Z

Thank you for your contribution :)
(And sorry that it took so long to process this PR)

ethindp · 2020-05-26T20:43:08Z

I was wondering when this would happen. I would be quite happy to continue to assist in Mumbles development where I can. It would be nice to see the pace of the project increase -- it seems as though its in a sort-of stasis mode.

…

On 5/26/20, Robert Adam ***@***.***> wrote: Thank you for your contribution :) (And sorry that it took so long to process this PR) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3896 (comment)

-- Signed, Ethin D. Probst

Krzmbrzl · 2020-05-27T05:43:53Z

it seems as though its
in a sort-of stasis mode.

*was
We're already starting to increase the pace of development again :)

ethindp · 2020-05-27T16:35:41Z

Glad to hear that! I've been trying to sync my fork with the latest upstream (I had to add that as a remote, you know, which is weird). I can't cloen it properly at the moment: Submodule path '3rdparty/celt-0.7.0-src': checked out '5a16cda6d78cda0cd14eb13c56c65d82724842e5' Submodule path '3rdparty/mach-override-src': checked out '919148f94db54fc04d287eb6a42c0c36b166bbfa' Submodule path '3rdparty/minhook-src': checked out '5e338f86bc29ff9de49a274ae6cbc0c21cdc1159' Submodule path '3rdparty/opus-src': checked out 'ad8fe90db79b7d2a135e3dfd2ed6631b0c5662ab' Submodule path '3rdparty/rnnoise-src': checked out 'b30f2bb8049601c3b7253d47f75dc6a68c5f32fd' Submodule path '3rdparty/sbcelt-src': checked out '045493d60b269ddb0096121552fd4d912704d4bf' Submodule 'celt-0.7.0' (https://git.xiph.org/celt.git/) registered for path '3rdparty/sbcelt-src/celt-0.7.0' Cloning into 'C:/Users/ethin/source/mumble/3rdparty/sbcelt-src/celt-0.7.0'... fatal: unable to access 'https://git.xiph.org/celt.git/': Failed to connect to git.xiph.org port 443: Connection refused fatal: clone of 'https://git.xiph.org/celt.git/' into submodule path 'C:/Users/ethin/source/mumble/3rdparty/sbcelt-src/celt-0.7.0' failed Failed to clone 'celt-0.7.0'. Retry scheduled Cloning into 'C:/Users/ethin/source/mumble/3rdparty/sbcelt-src/celt-0.7.0'... fatal: unable to access 'https://git.xiph.org/celt.git/': Failed to connect to git.xiph.org port 443: Connection refused fatal: clone of 'https://git.xiph.org/celt.git/' into submodule path 'C:/Users/ethin/source/mumble/3rdparty/sbcelt-src/celt-0.7.0' failed Failed to clone 'celt-0.7.0' a second time, aborting Submodule path '3rdparty/speex-src': checked out 'ecc63b360cbf6675d905ae278776c628fdfda00a' Submodule path '3rdparty/speexdsp-src': checked out 'a2133f5904ddb616b6578920bd2199f5accb484d' Submodule path 'themes/Mumble': checked out '9c68c1453837b82f69e93d7df2dbdd086d96f27d' Failed to recurse into submodule path '3rdparty/sbcelt-src'

…

On 5/27/20, Robert Adam ***@***.***> wrote: > it seems as though its in a sort-of stasis mode. *was We're already starting to increase the pace of development again :) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #3896 (comment)

-- Signed, Ethin D. Probst

Krzmbrzl · 2020-05-27T16:43:16Z

Yeah we had to remove sbcelt for that reason. You should be able to just go to a branch you haven't made any changes and then run

git pull https://github.com/mumble-voip/mumble.git master

to update to the latest and greatest version :)

davidebeatrici added GlobalShortcuts client labels Nov 14, 2019

davidebeatrici reviewed Nov 14, 2019

View reviewed changes

src/mumble/GlobalShortcut_win.h Outdated Show resolved Hide resolved

src/mumble/GlobalShortcut_win.h Outdated Show resolved Hide resolved

davidebeatrici reviewed Nov 14, 2019

View reviewed changes

src/mumble/GlobalShortcut_win.cpp Outdated Show resolved Hide resolved

src/mumble/GlobalShortcut_win.cpp Outdated Show resolved Hide resolved

davidebeatrici reviewed Nov 14, 2019

View reviewed changes

davidebeatrici reviewed Nov 15, 2019

View reviewed changes

src/mumble/GlobalShortcut_win.cpp Outdated Show resolved Hide resolved

Krzmbrzl reviewed Jan 20, 2020

View reviewed changes

src/mumble/GlobalShortcut_win.cpp Outdated Show resolved Hide resolved

Overlay, winhooks: add screen reader detection to prevent the setting…

a310c7f

… of winhooks if a screen reader was found to be running at app startup

Krzmbrzl force-pushed the master branch from 1e86d15 to a310c7f Compare January 20, 2020 19:02

Krzmbrzl changed the title ~~Overlay, winhooks: add screen reader detection to prevent the setting…~~ Disable winhooks if screen reader is active Jan 22, 2020

Krzmbrzl added the needs-testing A feature or fix that is ready but still needs testing before it can be used. label Feb 8, 2020

Krzmbrzl approved these changes May 26, 2020

View reviewed changes

Krzmbrzl merged commit 1d7fbd9 into mumble-voip:master May 26, 2020

davidebeatrici mentioned this pull request Feb 5, 2021

NVDA screen reader slowdown (Windows) #976

Closed

davidebeatrici mentioned this pull request Apr 22, 2021

FEAT(client): Implement raw input, get rid of hooks and DirectInput (Windows) #4941

Merged

Hartmnt added the a11y label Jan 21, 2024

Disable winhooks if screen reader is active #3896

Disable winhooks if screen reader is active #3896

Conversation

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019 • edited Loading

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019 • edited Loading

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 14, 2019 • edited Loading

ethindp commented Nov 14, 2019

davidebeatrici commented Nov 14, 2019

ethindp commented Nov 15, 2019

ethindp commented Nov 15, 2019

ethindp commented Nov 15, 2019 • edited Loading

Krzmbrzl commented Jan 20, 2020

ethindp commented Jan 20, 2020 via email

ethindp commented Jan 20, 2020

Krzmbrzl commented Jan 20, 2020

ethindp commented Jan 20, 2020

Krzmbrzl commented Jan 20, 2020

ethindp commented Jan 20, 2020

Krzmbrzl commented Jan 20, 2020

ethindp commented Jan 20, 2020

Krzmbrzl commented Jan 20, 2020

ethindp commented Jan 20, 2020

davidebeatrici commented Jan 22, 2020

Krzmbrzl commented Jan 22, 2020

Krzmbrzl commented Jan 30, 2020

ethindp commented Jan 31, 2020 via email

Krzmbrzl left a comment

Choose a reason for hiding this comment

Krzmbrzl commented May 26, 2020

ethindp commented May 26, 2020 via email

Krzmbrzl commented May 27, 2020

ethindp commented May 27, 2020 via email

Krzmbrzl commented May 27, 2020

davidebeatrici commented Nov 14, 2019 •

edited

Loading

ethindp commented Nov 14, 2019 •

edited

Loading

ethindp commented Nov 14, 2019 •

edited

Loading

ethindp commented Nov 15, 2019 •

edited

Loading