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

Issue 701815 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Mar 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Accelerator handling in mash doesn't match classic ash resulting in some accelerators not working

Project Member Reported by sky@chromium.org, Mar 15 2017

Issue description

When focus is on the renderer non-preferred/non-reserved accelerators don't work.

Classic ash handles accelerators in two distinct ways:

wm::AcceleratorFilter is an EventHandler and installed as a pre-target-handler of Shell. Being a pre-target-handler means AcceleratorFilter sees keyevents very early on. AcceleratorFilter ends up calling AcceleratorRouter::Process, which may handle the accelerator.

The second pass happens from Chrome. When focus is on a web page BrowserView::PreHandleKeyboardEvent is called. PreHandleKeyboardEvent indirectly calls through to ash by way of calling to the FocusManager, which ash hooks into by way of the FocusManagerDelegate.

It's the second pass that is not wired correctly in mash. In mash there is the notion of pre accelerators (which matches the first pass in classic ash) and post-accelerators (handled after the target is given the event and only if the target didn't consume the event). The post-accelerator phase was designed for the second pass in chrome. The problem is when focus is on a webpage chrome always consumes the event, so that the second phase never happens.

The second phase is what happens control-t and the lik.

 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 15 2017

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

commit 7bb882c8557f2b9e77f2c283a676478e11d30235
Author: sky <sky@chromium.org>
Date: Wed Mar 15 19:49:38 2017

Renames ui::mojom::Accelerator to ui::mojom::WmAccelerator

I'm doing this as I want to create a mojom for ui::Accelerator, and
this name conflicts with it.

BUG= 701815 
TEST=covered by tests
R=tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2754593003
Cr-Commit-Position: refs/heads/master@{#457174}

[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/ash/mus/accelerators/accelerator_controller_registrar.cc
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/ash/mus/accelerators/accelerator_controller_registrar.h
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/services/ui/common/accelerator_util.cc
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/services/ui/common/accelerator_util.h
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/services/ui/public/interfaces/window_manager.mojom
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/services/ui/ws/window_tree.cc
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/services/ui/ws/window_tree.h
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/ui/aura/mus/window_manager_delegate.h
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/7bb882c8557f2b9e77f2c283a676478e11d30235/ui/aura/mus/window_tree_client.h

Cc: mfomitchev@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 16 2017

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

commit d56e56c7ea299f4b1f565f57022e9341265d3d28
Author: sky <sky@chromium.org>
Date: Thu Mar 16 18:10:42 2017

Converts ui::Accelerator::type to an enum

Accelerator::type_ was an EventType, but this is overkill as the type
can only be pressed or released. So, this converts the type to an enum
with those two values.

I'm doing this as I'm going to write a mojom for Accelerator and it's
better to restrict the mojom to the actual values.

BUG= 701815 
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2751323002
Cr-Commit-Position: refs/heads/master@{#457492}

[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ash/common/accelerators/accelerator_controller.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ash/mus/accelerators/accelerator_controller_registrar.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/chrome/browser/ui/ash/chrome_screenshot_grabber_unittest.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ui/base/accelerators/accelerator.h
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ui/base/accelerators/accelerator_manager_unittest.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ui/content_accelerators/accelerator_util.cc
[modify] https://crrev.com/d56e56c7ea299f4b1f565f57022e9341265d3d28/ui/views/controls/textfield/textfield.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 16 2017

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 22 2017

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

commit c0c862a8b28d6afe8880049610ecb2320fd2b5b3
Author: sky <sky@chromium.org>
Date: Wed Mar 22 15:14:16 2017

Adds ability for accelerators to add key/value pairs to KeyEvent

pre-target accelerators are run before the KeyEvent is passed to the
target. If the pre-target accelerator doesn't handle the event the
event is passed to the target. This patch adds the ability for the
window-manager to add key-value pairs to the KeyEvent during the
pre-target phase. This will be used so that ash can inform chrome of
state about the KeyEvent that is necessary during Chrome's processing
the KeyEvent.

BUG= 701815 
TEST=covered by tests
R=erg@chromium.org, tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2761233003
Cr-Commit-Position: refs/heads/master@{#458752}

[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ash/mus/accelerators/accelerator_controller_registrar.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ash/mus/accelerators/accelerator_controller_registrar.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ash/mus/accelerators/accelerator_handler.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ash/mus/window_manager.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ash/mus/window_manager.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/demo/mus_demo_internal.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/demo/mus_demo_internal.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/public/interfaces/window_manager.mojom
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/test_change_tracker.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/test_change_tracker.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/test_utils.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_manager_client_unittest.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_manager_state.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_manager_state.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_manager_state_unittest.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_server_test_base.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_server_test_base.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_tree.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/services/ui/ws/window_tree.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ui/aura/mus/window_manager_delegate.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ui/aura/mus/window_manager_delegate.h
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ui/aura/mus/window_tree_client.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ui/aura/test/aura_test_base.cc
[modify] https://crrev.com/c0c862a8b28d6afe8880049610ecb2320fd2b5b3/ui/aura/test/aura_test_base.h

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 24 2017

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

commit f65d9bb4555976b047385b392d40fc62e4df9f27
Author: sky <sky@chromium.org>
Date: Fri Mar 24 02:26:39 2017

Adds the ability for WebContentsDelegate to decide if event should be updated

Currently the keyboard processing code for aura always marks the event
as handled. This means no other EventHandlers see the event if a page
has focus. For mus+ash we need to be able to control whether the
event should be marked handled or not. This patch changes
PreHandleKeyboardEvent() to return an enum indicating whether the
event should be marked handled or not.

BUG= 701815 
TEST=covered by tests
R=jam@chromium.org, sadrul@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2775553002
Cr-Commit-Position: refs/heads/master@{#459341}

[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/devtools/devtools_window.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/devtools/devtools_window.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/extensions/api/tab_capture/offscreen_tab.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/extensions/api/tab_capture/offscreen_tab.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/extensions/extension_view_host.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/extensions/extension_view_host.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/extensions/global_shortcut_listener_chromeos.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/browser.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/browser.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/cocoa/browser_window_cocoa.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/cocoa/browser_window_cocoa.mm
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/test/base/test_browser_window.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/chrome/test/base/test_browser_window.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/frame_host/interstitial_page_impl.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/frame_host/interstitial_page_impl.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_delegate.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_delegate.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_unittest.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_view_aura.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_view_aura.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_view_event_handler.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/renderer_host/render_widget_host_view_event_handler.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/public/browser/BUILD.gn
[add] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/public/browser/keyboard_event_processing_result.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/public/browser/web_contents_delegate.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/extensions/browser/app_window/app_window.cc
[modify] https://crrev.com/f65d9bb4555976b047385b392d40fc62e4df9f27/extensions/browser/app_window/app_window.h

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 24 2017

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

commit 62349ad779505d73be7c1dbd3db83dc9a38dc99f
Author: sky <sky@chromium.org>
Date: Fri Mar 24 18:30:21 2017

Adds kWillProcessAccelerator_KeyEventProperty

Ash sets this on KeyEvents that ash has an accelerator for and that
ash doesn't process immediately. This allows chrome to not forward
these events to the renderer so that ash will end up processing them
(in certain situations).

BUG= 701815 
TEST=covered by tests
R=sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2769213005
Cr-Commit-Position: refs/heads/master@{#459484}

[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/ash/common/accelerators/accelerator_controller.cc
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/ash/common/accelerators/accelerator_controller.h
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/ash/mus/accelerators/accelerator_controller_registrar.cc
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/ash/public/interfaces/event_properties.mojom
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/chrome/browser/ui/ash/ash_util.cc
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/chrome/browser/ui/ash/ash_util.h
[modify] https://crrev.com/62349ad779505d73be7c1dbd3db83dc9a38dc99f/chrome/browser/ui/views/frame/browser_view.cc

Comment 10 by sky@chromium.org, Mar 26 2017

Status: Fixed (was: Started)

Comment 11 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 13 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment