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

Check harder before enabling Accessibility support on Win32 #7611

Merged
merged 2 commits into from Oct 14, 2016

Conversation

Projects
None yet
4 participants
@paulcbetts
Contributor

paulcbetts commented Oct 13, 2016

Fixes #7208

TODO:

  • Verify with a touch screen
  • Verify that NVDA still works
@@ -18,6 +18,8 @@
#include "atom/browser/ui/win/message_handler_delegate.h"
#include "atom/browser/ui/win/taskbar_host.h"
#include "base/win/scoped_gdi_object.h"
#include "ui/base/win/accessibility_misc_utils.h"

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 13, 2016

Contributor

Just curious, what is this include used for?

This comment has been minimized.

@paulcbetts

paulcbetts Oct 13, 2016

Contributor

@kevinsawicki Without it, the next header barfs a thousand lines of COM nonsense

}
if (checked_for_a11y_support_) return false;
checked_for_a11y_support_ = true;

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 14, 2016

Contributor

On this branch (compared with master), I'm not seeing the 'accessibility-support-changed' event fire when you launch NVDA after launching your app:

To reproduce:

  • Launch Electron
  • Open the dev tools and run the following
require('electron').remote.app.on('accessibility-support-changed', function(e, axs) {console.log('changed', axs)})
  • Launch NVDA
  • 'changed true` is not logged on this branch (it is logged on master)

Should the checked_for_a11y_support_ = true; be done after the if (obj_id != OBJID_CLIENT) { check? If I move it below that check, then I do see it fire.

This comment has been minimized.

@paulcbetts

paulcbetts Oct 14, 2016

Contributor

We can do that, I was trying to make this as efficient as possible but I didn't know we had an event for this, 👍

@@ -228,6 +230,9 @@ class NativeWindowViews : public NativeWindow,
// In charge of running taskbar related APIs.
TaskbarHost taskbar_host_;
// Memoized version of a11y check
bool checked_for_a11y_support_;

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 14, 2016

Contributor

This should be initialized to false in the constructor like enabled_a11y_support_ is in atom/browser/native_window_views.cc

@kevinsawicki kevinsawicki self-assigned this Oct 14, 2016

const auto axState = content::BrowserAccessibilityState::GetInstance();
if (axState && !axState->IsAccessibleBrowser()) {
axState->OnScreenReaderDetected();
enabled_a11y_support_ = true;

This comment has been minimized.

@kevinsawicki

kevinsawicki Oct 14, 2016

Contributor

I think this variable can be removed now, it looks like it is no longer read anywhere.

This comment has been minimized.

@paulcbetts

paulcbetts Oct 14, 2016

Contributor

👍

@kevinsawicki kevinsawicki merged commit 3dd8377 into master Oct 14, 2016

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-mas-x64 Build #2632 succeeded in 8 min 2 sec
Details
electron-osx-x64 Build #2643 succeeded in 9 min 27 sec
Details
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 14, 2016

Thanks so much for fixing this 💻 🔍

@kevinsawicki kevinsawicki deleted the only-enable-msaa-if-needed branch Oct 14, 2016

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

@paulcbetts, why didn't you do it like this?

static bool IsScreenReaderActive()
{
  UINT screenReader = 0;
  SystemParametersInfo(SPI_GETSCREENREADER, 0, &screenReader, 0);

  return screenReader && UiaClientsAreListening();
}

...
      if (obj_id == OBJID_CLIENT && IsScreenReaderActive()) {
...

That would be much cleaner.

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

@paulcbetts did you try running Electron first and then running the screen reader? You only check if it's running once.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Oct 14, 2016

did you try running Electron first and then running the screen reader?

I did try this and it worked as expected but I wasn't on a touch screen. I launched Electron, then opened NVDA, and saw the accessibility state changed event fire.

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

@kevinsawicki, I am experimenting with this. Looks like the whole WM_GETOBJECT handling code may not be even necessary. JAWS, NVDA and Window-Eyes trigger the accessible mode without that. Narrator and ZoomText only work if they were running before Electron was launched.

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

Also the accessibility-support-changed event should be fired when BrowserAccessibilityState::IsAccessibleBrowser() value changes, BrowserAccessibilityState::OnScreenReaderDetected() does not have to be called by Electron code (https://github.com/adobe/chromium/blob/cfe5bf0b51b1f6b9fe239c2a3c2f2364da9967d7/content/browser/renderer_host/render_widget_host_view_win.cc#L2307)

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

Unfortunately there is no notification for BrowserAccessibilityState::IsAccessibleBrowser() changes

@miniak

This comment has been minimized.

Contributor

miniak commented Oct 14, 2016

Also, on my Windows 10 in Parallels on Mac, UiaClientsAreListening returns true even if I don't have any screen reader running.

@alexandrudima

This comment has been minimized.

alexandrudima commented Oct 19, 2016

@paulcbetts Thank you very much for the fix! ❤️

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