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

refactor: Improve accessibility of menus #15302

Merged
merged 10 commits into from Oct 29, 2018

Conversation

Projects
None yet
5 participants
@brenca
Copy link
Member

commented Oct 20, 2018

Description of Change

There are a couple of accessibility problems related to menus that I've found:

  • The Windows Narrator couldn't announce any menu items
  • The menu bar on Windows (and I guess Linux too) was not accessible by keyboard
  • #11587

The first fix was given to us by the Chromium 69 upgrade, which allowed the Windows Narrator to read the menus. This is because chromium's UIAutomation API implementation is not yet complete, and that's what the narrator uses on Windows (but the implementation progressed from 68 to 69 to allow the menus to be read).

The menu bar was not accessible because we basically didn't focus the View, which in turn didn't give it keyboard events. This was done because some commands in the menu (like copy) depend on the actual content being focused, instead of the bar. I fixed this by returning the focus before the command executes. Also, focusing the menu bar was not enough, since we used a plain View for the bar, which didn't implement any keyboard navigation of it's items. I changed the base class to AccessiblePaneView, which implements that navigation (should fix #2504).

electron-acc-1

#11587 was caused by the fact that the tray icon's context menu was spawned without a Widget, since it's not necessary to have a BrowserWindow to have an icon in the tray. The Widget is important because that's what actually captures keyboard events, and the menus leech off their parent Widgets for those events. I added a temporary non-visual Widget to the menu that gets destroyed when the menu closes, which fixed the issue.

electron-acc-2

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: improved tray icon context menu and menu bar accessibility

@brenca brenca requested review from ckerr and codebytere Oct 20, 2018

@brenca brenca requested review from as code owners Oct 20, 2018

public views::MenuButtonListener,
public views::FocusChangeListener {
public atom::MenuDelegate::Observer {

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 22, 2018

Member

I think this may introduce a minor bug on Linux.

Focused windows have a different color menubar than unfocused windows. In the old code, MenuBar unconditionally listened for focus to change, either gaining or losing focus, and updated the colors accordingly.

AccessiblePaneView seems to set up focus events in SetPaneFocus() and only use the change listener for losing focus, so it's not clear to me that the menubar colors will be set properly here.

Maybe if UpdateViewColors(); were called in our override of SetPaneFocus?

This comment has been minimized.

Copy link
@brenca

brenca Oct 22, 2018

Author Member

Hmm, I'm not sure about this, but we still have the OnDidChangeFocus method override, that calls the AccessiblePaneView implementation, and then does what it did before (updates the colors AFAICS). Won't that be enough to preserve the old functionality?

This comment has been minimized.

Copy link
@ckerr

ckerr Oct 23, 2018

Member

I might be mistaken about this, but I think that even though we still override OnDidChangeFocus(), that's only called when the focus change listener is registered via AddFocusChangeListener(). We used to call that in MenuBar's constructor but no longer do, and AccessiblePaneView only keeps it registered as long as the pane has focus.

So we would be notified on focus lost, but (I think) not notified on focus added.

Could you test this by adding a LOG message in our OnDidChangeFocus() and see if it's called when focus is gained?

This comment has been minimized.

Copy link
@brenca

brenca Oct 26, 2018

Author Member

You were right, thanks for catching this!
I've pushed a fix, and rebased on top of master.

@brenca brenca changed the title [WIP] refactor: Improve accessibility of menus refactor: Improve accessibility of menus Oct 23, 2018

@brenca brenca force-pushed the brenca/improve-accessibility branch from 9f9fa1f to 5c729d2 Oct 26, 2018

@ckerr

ckerr approved these changes Oct 29, 2018

Copy link
Member

left a comment

Thanks for the changes!

@daviwil

This comment has been minimized.

Copy link

commented Oct 29, 2018

Thanks so much for addressing this issue! Atom users have been reporting it for at least 2 years: atom/atom#5998. Really excited to get this into a future release!

@codebytere
Copy link
Member

left a comment

🚀

@ckerr ckerr merged commit 894ae1b into master Oct 29, 2018

26 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP Legacy commit status override — see details
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181026.4 succeeded
Details
electron-arm64-testing Build #20181026.4 succeeded
Details
electron-lint Build #20181027.10 succeeded
Details
electron-mas-testing Build #20181027.7 succeeded
Details
electron-osx-testing Build #20181027.7 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Oct 29, 2018

Release Notes Persisted

Improved tray icon context menu and menu bar accessibility

@marisademeglio

This comment has been minimized.

Copy link

commented Jan 4, 2019

Thanks for these changes! Any idea which release they'll be in?

ckerr added a commit that referenced this pull request Feb 27, 2019

fix: check for pane focus before removing it.
Fixes #16883. This bug seems to have been introduced in the #15302's
menu a11y refactor and is new in 5-0-x.

@ckerr ckerr referenced this pull request Feb 27, 2019

Merged

fix: check for pane focus before removing it. #17164

3 of 4 tasks complete

trop-bot pushed a commit to trop-bot/electron that referenced this pull request Feb 27, 2019

fix: check for pane focus before removing it.
Fixes electron#16883. This bug seems to have been introduced in the electron#15302's
menu a11y refactor and is new in 5-0-x.

ckerr added a commit that referenced this pull request Mar 4, 2019

fix: check for pane focus before removing it. (#17164)
Fixes #16883. This bug seems to have been introduced in the #15302's
menu a11y refactor and is new in 5-0-x.

trop-bot pushed a commit to trop-bot/electron that referenced this pull request Mar 4, 2019

fix: check for pane focus before removing it.
Fixes electron#16883. This bug seems to have been introduced in the electron#15302's
menu a11y refactor and is new in 5-0-x.

codebytere added a commit that referenced this pull request Mar 4, 2019

fix: check for pane focus before removing it. (#17215)
Fixes #16883. This bug seems to have been introduced in the #15302's
menu a11y refactor and is new in 5-0-x.

@GulajavaMinistudio GulajavaMinistudio referenced this pull request Mar 5, 2019

Merged

Refactor function for kubeconfig #40

0 of 6 tasks complete

Kiku-git added a commit to Kiku-git/electron that referenced this pull request May 16, 2019

fix: check for pane focus before removing it. (electron#17164)
Fixes electron#16883. This bug seems to have been introduced in the electron#15302's
menu a11y refactor and is new in 5-0-x.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.