New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 810160 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

When a menu is opened using the keyboard, focus lands where the mouse pointer is

Project Member Reported by nek...@chromium.org, Feb 7 2018

Issue description

On Windows and while running a screen reader.
What steps will reproduce the problem?
(1) Via the mouse, open Chrome's main menu, one of its submenus or (I suspect) any context menu on the page.
(2)Point on one of the menu items.
(3) Press esc to close the menu and re-open it using the keyboard, e.g. for the main menu press "Alt+F", for the Bookmarks submenu press "Alt+f then B".

What is the expected result?
Focus to land on the first menu item.

What happens instead?
Focus lands on the menu item where the mouse is pointing.


 
Labels: OS-Windows

Comment 2 by groby@google.com, Feb 13 2018

Status: Assigned (was: Asigned)

Comment 3 by pbos@chromium.org, Feb 27 2018

Cc: pbos@chromium.org
Owner: davidbienvenu@chromium.org
This reproduces on windows and linux, w/o a screen reader. I traced it down to an ET_MOUSE_MOVED event getting generated when the context menu is opened, and causing the context menu code to select the item where the mouse moved to. This event gets received even if the mouse isn't under the menu when it appears. I haven't been able to figure out what is causing the event to get posted. My theory was that some Chrome UI code was synthesizing the event, since the issue is cross platform, but I haven't found any code doing this, that gets triggered in this scenario.
void WindowEventDispatcher::OnWindowVisibilityChanged(Window* window,

calls PostSynthesizeMouseMove with this stack:

 	aura.dll!aura::WindowEventDispatcher::PostSynthesizeMouseMove() Line 821	C++
>	aura.dll!aura::WindowEventDispatcher::OnWindowVisibilityChanged(aura::Window * window, bool visible) Line 704	C++
 	aura.dll!aura::Window::NotifyWindowVisibilityChangedAtReceiver(aura::Window * target, bool visible) Line 1052	C++
 	aura.dll!aura::Window::NotifyWindowVisibilityChangedDown(aura::Window * target, bool visible) Line 1059	C++
 	aura.dll!aura::Window::NotifyWindowVisibilityChanged(aura::Window * target, bool visible) Line 1040	C++
 	aura.dll!aura::Window::SetVisible(bool visible) Line 805	C++
 	aura.dll!aura::Window::Show() Line 231	C++
 	views.dll!views::DesktopNativeWidgetAura::ShowWithWindowState(ui::WindowShowState state) Line 764	C++
 	views.dll!views::Widget::ShowInactive() Line 658	C++
 	views.dll!views::MenuHost::ShowMenuHost(bool do_capture) Line 160	C++
 	views.dll!views::MenuHost::InitMenuHost(views::Widget * parent, const gfx::Rect & bounds, views::View * contents_view, bool do_capture) Line 148	C++
 	views.dll!views::SubmenuView::ShowAt(views::Widget * parent, const gfx::Rect & bounds, bool do_capture) Line 391	C++
 	views.dll!views::MenuController::OpenMenuImpl(views::MenuItemView * item, bool show) Line 1834	C++
 	views.dll!views::MenuController::OpenMenu(views::MenuItemView * item) Line 1805	C++
 	views.dll!views::MenuController::CommitPendingSelection() Line 1770	C++
 	views.dll!views::MenuController::SetSelection(views::MenuItemView * menu_item, int selection_types) Line 1198	C++
 	views.dll!views::MenuController::Run(views::Widget * parent, views::MenuButton * button, views::MenuItemView * root, const gfx::Rect & bounds, views::MenuAnchorPosition position, bool context_menu, bool is_nested_drag) Line 481	C++
 	views.dll!views::internal::MenuRunnerImpl::RunMenuAt(views::Widget * parent, views::MenuButton * button, const gfx::Rect & bounds, views::MenuAnchorPosition anchor, int run_types) Line 139	C++
 	views.dll!views::MenuRunner::RunMenuAt(views::Widget * parent, views::MenuButton * button, const gfx::Rect & bounds, views::MenuAnchorPosition anchor, ui::MenuSourceType source_type) Line 73	C++
 	chrome.dll!AppMenu::RunMenu(views::MenuButton * host) Line 834	C++
 	chrome.dll!AppMenuButton::ShowMenu(bool for_drop) Line 133	C++
 	chrome.dll!ToolbarView::OnMenuButtonClicked(views::MenuButton * source, const gfx::Point & point, const ui::Event * event) Line 353	C++
 	views.dll!views::MenuButton::Activate(const ui::Event * event) Line 129	C++
 	chrome.dll!BrowserView::ShowAppMenu() Line 1308	C++
 	chrome.dll!chrome::ShowAppMenu(Browser * browser) Line 1104	C++

Comment 6 by pbos@chromium.org, Mar 30 2018

Cc: sky@chromium.org
sky@: My hunch of this is that: When bringing up a new window the pointer artificially moves to where it is even if the mouse is not used to bring up this window.

This is a problem when a menu dropdown is triggered by a keyboard event as the cursor event will hover over the menu item under the cursor rather than let you navigate from top to bottom. This is especially a problem for screenreader users who don't know where their cursor are, so their keyboard navigation becomes inconsistent because of it.

This sounds like a scary core thing to remove though, do you have some thoughts on how to proceed here?
The root cause is that setting the selection on the menu on menu open is eventually synthesizing a mouse move event, due to window visibility change, and the menu controller sees that mouse move as a reason to change the selection. I don't think the code synthesizing the mouse move event (WindowEventDispatcher) has enough context to not to do this in this situation, so, perhaps the menu controller could ignore mouse move events while it's already setting the selection. 

Comment 8 by pbos@chromium.org, Mar 30 2018

Won't the posted event trigger after MenuController::SetSelection has returned? I assume it needs to return all the way back to the event loop before this resolves. Is that not the case?

Conceptually to me having a synthetic mouse move event on window visibility assumes mouse usage, a keyboard user probably wouldn't want their pointer considered. I'd assume this to be true outside menus as well, but it might not have as significant consequences.

I think a keyboard user might never want any of these syntesized events in the window event dispatcher, but that likely has complex implications. I'm not sure either what the implications of removing them is (will mouse clicks without movement happen at the wrong position for some windows because the cursor hasn't moved there first?).

Comment 9 Deleted

directly checking if we're setting the selection won't work because the synthesized event happens asynchronously. 
autofill_popup_base_view looks like it's dealing with this issue by explicitly checking for a synthesized mouse move event and ignoring it
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/autofill_popup_base_view.cc?rcl=381f71a41755e3af9039b26a716e37a163e400eb&l=195
right, the event was posted. I don't think the event dispatcher knows if the mouse or keyboard is being used in this case - it just knows window visibility changed.

Comment 12 by pbos@chromium.org, Mar 30 2018

Ignoring ui::EF_IS_SYNTHESIZED events in the event handler might be reasonable then. I'd cook up a patch with that and try to get it reviewed (probably by sky@) and see if they can poke holes in that approach.

Comment 13 by pbos@chromium.org, Mar 30 2018

If you still want the synthesized event to count you might be able to store how the menu invocation happened and only ignore it for keyboard.
I looked at storing how the menu invocation happened this morning - it happens at a very high level, and the command is kicked off with just the command id, independent of   what triggered the command, so it didn't seem promising.

Ignoring EF_IS_SYNTHESIZED events seems necessary but not sufficient for fixing this bug. Even after ignoring that event, I'm still seeing a mouse moved WM that isn't synthesized, and thus is hard to track down the origin of. Perhaps using the show_time_ delay approach that auto_fill_base_popup_view uses would work, as long as it didn't make the menu feel unresponsive.


Ah, Windows is doing this, so on Windows, we'd need the delay tactic.
https://bugs.chromium.org/p/chromium/issues/detail?id=458300#c12
 Issue 797019  has been merged into this issue.

Comment 17 by pbos@chromium.org, Apr 2 2018

If this is OS behavior as well, fixing it sounds way more finicky. I'd like to avoid timers if we can (but I'm not claiming that we can).

It doesn't seem system wide as Looking at VS2017 their menus don't have this behavior, neither does the OS-wide alt+space window menu. I wonder if there's a WM_ flag that could be set to let Windows know that the window is a menu. I don't know how Windows menus work.
I have a fix for normal menus. It doesn't work for buttons within menus, like the zoom buttons in the app menu. The menu button gets a VisibilityChanged notification, and thinks the mouse is hovering over the button, so it gets selected. In reality, the menu was opened over the mouse. I'm going to leave that issue for a separate CL since that's going to involve completely different code, but I'm recording this for posterity since it took me a while to figure out - one possibility is to override the VisibilityChanged handling for the menu button class.
 
views.dll!views::Button::SetState(views::Button::ButtonState state) Line 126
	at C:\src\chromium\src\ui\views\controls\button\button.cc(126)
views.dll!views::Button::VisibilityChanged(views::View * starting_from, bool visible) Line 415
	at C:\src\chromium\src\ui\views\controls\button\button.cc(415)

Project Member

Comment 19 by bugdroid1@chromium.org, Jun 21 2018

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

commit 508b19e8e860ff221499ec365c9be04f62b6cd1d
Author: David Bienvenu <davidbienvenu@chromium.org>
Date: Thu Jun 21 23:27:17 2018

Don't select popup menu item simply because mouse is under it.

WindowEventDispatcher synthesizes a mouse move event when a window's visibility changes, and Windows OS
generates a mouse move event if a window opens the current mouse position.

For Windows, make menu_controller ignore mouse move events for the first 100ms after the menu opens.
On other platforms, make menu_controller ignore synthesized mouse move events.

BUG= 810160 

Change-Id: I234376efb4c635a0345b4ef7ebdbca7727684263
Reviewed-on: https://chromium-review.googlesource.com/988800
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569448}
[add] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/chrome/browser/ui/views/menu_interactive_uitest.cc
[modify] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/chrome/test/BUILD.gn
[modify] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/ui/views/controls/menu/menu_controller.cc
[modify] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/ui/views/controls/menu/menu_controller.h
[modify] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/ui/views/controls/menu/menu_controller_unittest.cc
[modify] https://crrev.com/508b19e8e860ff221499ec365c9be04f62b6cd1d/ui/views/controls/menu/menu_item_view.h

Status: Fixed (was: Assigned)

Sign in to add a comment