If navigate through buttons on toolbar — everything is ok, extension buttons are titled. Problem persists only with hidden in chrome menu

ser.melipharo@ -- Could you please confirm whether the expected is to read the extensions name displayed when clicked on the Chrome settings menu.

Attached is the screen cast for your reference.

Let us know if we have missed anything.

Thanks!

Deleted: 835738.webm 4.6 MB

	835738.webm 4.6 MB View Download

the item in the hidden extension's menu which is read as the extension's name actually goes to that extension's page in the Chrome Webstore. So, it should be labelled something like "Extension name in the Chrome Webstore."

Another issue with the extension in the Chrome menu is that it doesn't indicate to screen reader users that it has a context menu available.

It is confusing that this item should be activated with context menu instead of right arrow so perhaps some help text would be helpful here.

I picked this up for a bit as my first screenreader-related a11y bug.  Setting it down again since I am going to be OOO, so I'm summarizing what I've explored so far.  Please bear with my confusion :P

The hidden extension menu is an ExtensionToolbarMenuView; it contains a BrowserActionsContainer, which contains a collection of ToolbarActionViews, each of which largely delegates GetAccessibleNodeData to its parent MenuButton class.

The BrowserActionsContainer has very extensive variations between the toolbar and app menu scenarios (these really shouldn't both be in the same class...).  I don't think any of them are relevant since they mostly seem focused around layout, resizing, and drag&drop.  However, ToolbarActionViews have different focus behavior when they're in the hidden extension menu vs. the main one (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/toolbar/toolbar_action_view.cc?type=cs&g=0&l=76) which might not be irrelevant...

Looking at the accessibility behavior via Microsoft's Inspect tool, it looks like the current accessibility node isn't changing as I navigate between the buttons.  This lines up with what I hear when using Narrator ('up arrow', 'down arrow' instead of announcing the newly focused element).  The tree view doesn't show the extension buttons, only a single entry for their containing menu item, the ExtensionToolbarMenuView.

So all this doesn't get us very far.  All we really know is that the accessibility tree doesn't know about these buttons when they're inside an ExtensionToolbarMenuView.

I also spent some time in the debugger exploring the relevant code paths:

When focus switches within the hidden extension menu, that goes through MenuController::IncrementSelection, which switches to the next 'hot tracked' button for that MenuItem (aka the ExtensionToolbarMenuView).  This triggers a selection accessibility event which makes its way out to the OS (not sure what, if anything, is done with it from there).

When focus switches into the hidden extension menu (i.e. between menu items), that again goes through MenuController::IncrementSelection, which switches to the first (or last) hot tracked button.  This follows a similar code path to trigger a selection accessibility event.

When switching into or within the main extension menu, these entry points call into the ToolbarActionView's GetAccessibleNodeData:
AXPlatformNodeWin::get_accState
AXPlatformNodeWin::get_accRole
This doesn't happen in the hidden extension menu case, which I would hazard a guess is because they aren't in the accessibility tree.  So that's probably another symptom of the problem.

Taking this over

Multiple things going on here. Notes before long weekend:

When BrowserActionsContainer is in the menu it has ExtensionToolbarMenuView as a parent. MenuItem::GetAccessibleNodeData correctly delegates to ExtensionToolbarMenuView::GetAccessibleNodeData, but the latter is not defined, so it returns an unknown node with no name.

Unfortunately, it's not clear what the definition of ExtensionToolbarMenuView::GetAccessibleNodeData should look like. It can't delegate to BrowserActionsContainer::GetAccessibleNodeData; that just reads out "Extensions" because it's expecting the a11y system to target the child ToolbarActionView instead.

I tried hacking it to call GetAccessibleNodeData on MenuController::hot_button_, which is updated when selection changes. That works, but if you have multiple extensions the button is only read out when you first enter the extensions area, not when switching between extensions.

Fundamentally, the problem seems to be that these "composite" menu items are confusing the targeting. I'm not really sure how to fix it without breaking the item up into multiple MenuItem instances (like Cut/Copy/Paste), which would be a really big change. Though like comment #8 says, maybe we shouldn't be overloading BrowserActionsContainer in the first place.

Also, we have a similar problem with the Zoom plus/minus buttons, which I don't see a separate bug for; I bet it's the same root cause.

cc dmazzoni@ and robliao@, any ideas?

Okay, I think I figured it out. When you're in a menu and you focus something that doesn't have a role of kMenuItem, the screen reader goes "that's weird" and doesn't read anything. So we need to mark all the buttons in the app menu as menu items (this also fixes the zoom buttons).

The fix is conceptually simple but since all the code is shared with the toolbar version of the extensions it might be a bit tricky to actually write.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6a9827421a4059369e5b846ee26b048e4f4c931

commit f6a9827421a4059369e5b846ee26b048e4f4c931
Author: Bret Sepulveda <bsep@chromium.org>
Date: Thu Nov 29 19:17:23 2018

Fix screen reader not reading extensions and zoom buttons in app menu.

At least on Windows, the screen reader will not read non-menu-item
Views in a menu. Currently, extensions hidden in the app menu and
the zoom plus and minus buttons are not read. This patch tags them as
kMenuItem, which fixes the bug.

Bug:  835738 
Change-Id: I507c9595f6aa02d8768330f18a8b725ef1b440e4
Reviewed-on: https://chromium-review.googlesource.com/c/1354482
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612302}
[modify] https://crrev.com/f6a9827421a4059369e5b846ee26b048e4f4c931/chrome/browser/ui/views/toolbar/app_menu.cc
[modify] https://crrev.com/f6a9827421a4059369e5b846ee26b048e4f4c931/chrome/browser/ui/views/toolbar/browser_actions_container.h
[modify] https://crrev.com/f6a9827421a4059369e5b846ee26b048e4f4c931/chrome/browser/ui/views/toolbar/toolbar_action_view.cc

Tried testing the issue on latest chrome #72.0.3626.0 using Windows 10 by following steps below steps. 

Steps:
=====
1.Launched chrome.
2.Added extensions to chrome.
3.Hidden the extensions in the Chrome menu.
4.Navigated to the Chrome menu.
5.Observed that the screen reader did not read the hidden extensions.
Note: Could not check the issue on Windows 7 due to some technical issues.

Attached screencast for reference.
@Bret Sepulveda: As per comment#12 says "This patch tags them as kMenuItem, which fixes the bug".But we could not find any difference in behavior in chrome version #65.0.3325.181 and latest chrome #72.0.3626.0. Could you please help us in knowing what exactly is being fixed here and also review attached screencast and let us know if anything is being missed here.
Thanks.!

Deleted: 835738.mp4 3.1 MB

	835738.mp4 3.1 MB View Download

#14: Interesting, I didn't change the mouse behavior because my screen reader reads the tooltips when they pop up, which I figured was fine. If that's not sufficient, we can reopen the bug. Otherwise use the keyboard to actually focus the extension, and it should read it.