New issue
Advanced search Search tips

Issue 896947 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug


Sign in to add a comment

mash: Support ui::EventHandlers that handle events outside the client

Project Member Reported by msw@chromium.org, Oct 19

Issue description

mash: Support ui::EventHandlers that handle events outside the client

This is similar to Issue 647781 regarding EventRewriters.
The Window Service only notifies clients about events targeting each clients' windows.
However, some Chrome code assumes it can observe/handle events targeting Ash and mojo app windows.
We'll need to update some ui::EventHandlers for SingleProcessMash and more for multi-process Mash.
This issue will serve as a tracking bug for the work needed, and I'll file blocking bugs.

Here's a concise first pass over the code that may need attention (some window pre-target handlers are questionable):

*** env.*AddPreTargetHandler - https://cs.chromium.org/search/?q=env.*AddPreTargetHandler&type=cs
6 interesting cases, 3 need SingleProcessMash fixes (convert 2 to EventObserver, unknown action for ui_devtools)

ImmersiveGestureHandler - marks events as handled (SingleProcessMash ok, needs Mash impl).
Reveals top chrome, allows dragging window? ImmersiveHandlerFactoryMus::CreateGestureHandler is NOTIMPLEMENTED.
See  Issue 640365  and  Issue 791148 

MenuPreTargetHandlerAura - stops event propagation (SingleProcessMash ok w/warning?, needs Mash impl to handle null widget owner/root?).
Consumes key events while a menu is showing.
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_pre_target_handler_aura.cc?rcl=c0f0cb93c2e34208e09114032ea0ae9ecf92f51d&l=35

RenderWidgetHostViewAura::EventFilterForPopupExit - observes events (SingleProcessMash broken, convert to EventObserver)
Closes web popups on mouse/touch press outside the window/parent.
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_widget_host_view_aura.cc?rcl=b0e3983f7a7a814c285c2302b16ffd345ba37815&l=203

OverlayAgentAura - marks events as handled (SingleProcessMash broken? needs system priority?).
A ui_devtools feature for inspecting native views ui
https://cs.chromium.org/chromium/src/components/ui_devtools/views/overlay_agent_aura.cc?rcl=57d7dded44f76ed86c845232d22b678fdb441e46&l=390

TouchSelectionControllerClientAura::EnvPreTargetHandler - observes events (SingleProcessMash broken, convert to EventObserver)
Closes selection/handles/menu on events outside the client window, fix like TouchSelectionControllerImpl
https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/touch_selection_controller_client_aura.cc?rcl=a563e29ee53b9b2f5e8a35da7c52f96310636a02&l=75


*** shell.*AddPreTargetHandler - https://cs.chromium.org/search/?q=shell.*AddPreTargetHandler&type=cs
3 interesting cases, no SingleProcessMash action needed

exo::WmHelper::AddPreTargetHandler/PrependPreTargetHandler/AddPostTargetHandler - exo::Keyboard sets events as handled (SingleProcessMash ok, needs Mash impl)
Used for exo keyboard/pointer/touch input; see exo::Seat,
https://cs.chromium.org/chromium/src/components/exo/wm_helper.cc?rcl=745d385ecf5ac39ca2fd604dd5451e61e47d980b&l=203

TabScrubber - stops event propagation (SingleProcessMash ok, needs Mash impl?)
Used for 3-finger swiping to switch/preview other tabs
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/tab_scrubber.cc?rcl=30fe2105ff384d8485ab2da6bc0be124dcb4c44f&l=95
See Issue 796366

InputEventsBlocker - stops event propagation (SingleProcessMash ok, needs Mash impl)
Used to temporarily block all user input
See Issue 854323 - https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/ui/input_events_blocker.cc?rcl=51c372b43f79848c0fe7ae2a2b5983d78b843a19&l=17


*** Other "AddPreTargetHandler -f:test f:cc$ -f:src/ash" https://cs.chromium.org/search/?q=AddPreTargetHandler+-f:test+f:cc$+-f:src/ash&type=cs

webrunner::FrameImpl (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/webrunner/browser/frame_impl.cc?rcl=bef94ee00fcb2b8a94fbd9b0323094fbc37e9601&l=193

WindowModalityController (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/ui/wm/core/window_modality_controller.cc?rcl=fd9098da9479af98825bdc1bdcaf596aceacb081&l=97

PreMenuEventDispatchHandler (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/ui/views/controls/menu/menu_host.cc?rcl=fd9098da9479af98825bdc1bdcaf596aceacb081&l=47

CastSystemGestureEventHandler (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/chromecast/graphics/gestures/cast_system_gesture_event_handler.cc?rcl=5a8004ef413b75defefa1202584392a6b06db038&l=30

chromecast::shell::TouchBlocker (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/chromecast/browser/cast_content_window_aura.cc?rcl=5a8004ef413b75defefa1202584392a6b06db038&l=24

SwitchAccessEventHandler - marks events as handled and stops event propagation (SingleProcessMash ok, needs Mash impl)
Proceses key events as switch access events...
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/switch_access_event_handler.cc?rcl=5a8004ef413b75defefa1202584392a6b06db038&l=19

ShellDesktopControllerAura (For app_shell, test only? Unclear if this is applicable or needs events outside the client's windows)
https://cs.chromium.org/chromium/src/extensions/shell/browser/shell_desktop_controller_aura.cc?rcl=2d39a8f3a2619183d32205f748a7a35d63539a6b&l=376

MouseCursorOverlayController::Observer (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/content/browser/media/capture/mouse_cursor_overlay_controller_aura.cc?rcl=c51a63f653d3104ce53f7fa378ce662e7271c50d&l=39

chromecast::PartialMagnificationController (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/chromecast/graphics/accessibility/partial_magnification_controller.cc?rcl=c51a63f653d3104ce53f7fa378ce662e7271c50d&l=199

LoginDisplayHostWebUI::KeyboardDrivenOobeKeyHandler (Unclear if this needs events outside the client's windows)
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/ui/login_display_host_webui.cc?rcl=c51a63f653d3104ce53f7fa378ce662e7271c50d&l=384
 
Blockedon: 896948
Blockedon: 896973
Blockedon: 896977
Blockedon: 884394
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 25

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

commit 51d03801aaa0c56cd2917df7c07bd2adef2ecb72
Author: Mike Wasserman <msw@chromium.org>
Date: Thu Oct 25 16:48:50 2018

mash: Convert MenuPreTargetHandlerAura owner warning to DCHECK.

Menus should always have an owner when using the Window Service.
Excuse a test that hits this, we want to find any non-test cases.
Test context: https://codereview.chromium.org/2208113002

Bug: 896947
Test: DCHECK not hit.
Change-Id: I9331e531543b11c7e344a235dca7765610b40d3c
Reviewed-on: https://chromium-review.googlesource.com/c/1298321
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602755}
[modify] https://crrev.com/51d03801aaa0c56cd2917df7c07bd2adef2ecb72/ui/views/controls/menu/menu_controller_unittest.cc
[modify] https://crrev.com/51d03801aaa0c56cd2917df7c07bd2adef2ecb72/ui/views/controls/menu/menu_pre_target_handler_aura.cc

Labels: -Proj-Mash-SingleProcess
The only outstanding work here for SingleProcessMash is Issue 896977 and I have a CL out for review:
  https://chromium-review.googlesource.com/c/chromium/src/+/1324373
I'm going to mark this as only blocking multi-process Mash for now.
Testing the virtual keyboard in SingleProcessMash (with https://chromium-review.googlesource.com/c/chromium/src/+/1300553 applied), I am running into NOTREACHED() in TouchSelectionControllerImpl::GetSelectedText().

Can you provide repro steps? I'm not sure why the virtual keyboard would trigger a DCHECK in ui for the touch selection handles rendered over textfields. Further, I'm not sure if the problem you're hitting is related to this issue about EventHandlers.
Status: Assigned (was: Started)

Sign in to add a comment