Shortcut Viewer app doesn't support some touch interactions |
|||||
Issue description
Shortcut Viewer app doesn't support some textfield touch interactions
(1) Run Chrome OS ToT (eg. #577173), running on linux desktop is ok.
(on linux desktop I'm using --touch-devices=10 --show-taps)
(2) Launch the Shortcut Viewer app with Ctrl-Alt-/
(3) Type in the search textfield
(4) Tap somewhere in the typed text
(5a) Try to double-tap (or triple-tap) the typed text
Expected: A selection is made for the tapped word (or whole line)
Actual: double- and triple-tapping does nothing
(5b) Try to drag the blue touch selection handle
Expected: Able to drag the selection handle to change caret position
Actual: Unable to move the selection handle to change caret position
Currently, the Shortcut Viewer app is on-by-default on M-69 and ToT
We'll want a fix by M-69 stable or disable the app by default there.
See about:flags "Keyboard Shortcut Viewer mojo app" / --disable-features=KeyboardShortcutViewerApp
,
Jul 26
Also, I thought this would be related to 867483, but it isn't. I have a fix for 867483 and it doesn't impact this one way or the other.
,
Jul 26
Agreed, it's probably related to Issue 866991 , the textfield gets ET_GESTURE_SCROLL_* events when dragging the selection handle in classic, but in the app I get ET_TOUCH_* events (I haven't tested web content with Mash). I'll look more closely at GestureRecognizer.
,
Jul 26
Correction, the app gets ET_POINTER_* with POINTER_TYPE_TOUCH, not ET_TOUCH.
,
Jul 26
Does the ET_POINTER_* events get converted to ET_TOUCH* events before it reaches aura in the client?
,
Jul 26
We should be doing there here: https://chromium.googlesource.com/chromium/src/+/master/ui/aura/mus/window_tree_client.cc#138
,
Jul 26
Yes, the client's GestureProviderAura is getting TouchEvent objects (per the conversion Scott cited) and the client's EventDispatcher is getting GestureEvent objects, AFAICT something is going wrong withing EventDispatcher::ProcessEvent, that's where I'm investigating now.
,
Jul 26
I have a hacky workaround making DesktopWindowTreeHostMus's window pass all events to the content window: https://chromium-review.googlesource.com/c/chromium/src/+/1152102 (patch set 1) I'll investigate why the content window isn't targeted in the first place.
,
Aug 1
Release blockers should be P1 or P0
,
Aug 1
Thanks, I am making good progress on a solution: https://chromium-review.googlesource.com/c/chromium/src/+/1152102
,
Aug 3
I fixed the placement of touch selection text editing handles on secondary displays, by using ScreenPositionClient in WindowTree::SetWindowBoundsImpl: https://chromium-review.googlesource.com/c/chromium/src/+/1152102/15/services/ui/ws2/window_tree.cc#1026 Unfortunately, this means clients must specify the screen-coordinates for bounds of their top-level windows, not root/display-coordinates. Since Ash/ws decides which display new top-level windows should be shown on, clients cannot currently determine their screen coordinates during widget/window initialization. I have some workarounds for this (clients ask the service to center the window on init), but it would be nice if clients could synchronously determine the appropriate display for their new top-level windows during widget/window init. James has a patch from May that he might revive to add display::Screen::GetDisplayForNewWindows or similar, and that would be perfect for this scenario, and potentially others. I think I'll aim to land my workaround and clean it up once James' CL has landed.
,
Aug 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3 commit 05b3a3f7cada9f1fe67ef2ff4f73f013525975f3 Author: James Cook <jamescook@chromium.org> Date: Tue Aug 07 00:38:55 2018 Add Screen::GetDisplayForNewWindows to support remote mojo apps Chrome OS supports the concept of a "root window for new windows", which allows new windows to be automatically placed on the correct display. This knowledge lives in ash, currently inside the browser process. Remote processes that use views::Widget (e.g. the shortcut viewer app) sometimes need to know what window they are going to open on, for example to do initial bounds computations. Change Screen to provide set and get for the display for new windows. Use mojo to send the information to other processes via ScreenProvider (on the ash side) and ScreenMus (on the remote side). Send the display id as part of display updates so that ScreenMus gets it on startup, and also so updates are atomic with other display changes. Once this lands we'll be able to port browser windows to use this mechanism and delete the ash::ShellState / chrome ShellStateClient code I built as a one-off for browser windows under mash. Bug: 867559 , 764009 , 826569 Test: added unit tests Change-Id: I6b5beb2610243f30072c3d73180b3ce21a310ca6 Reviewed-on: https://chromium-review.googlesource.com/1162980 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#581055} [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/display/screen_ash_unittest.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/shell_state.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ash/ws/window_service_owner.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/public/interfaces/screen_provider_observer.mojom [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/screen_provider_unittest.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/test_screen_provider_observer.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/services/ui/ws2/window_service.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/aura/mus/window_tree_client_delegate.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/display/screen_unittest.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/mus_client.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.cc [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus.h [modify] https://crrev.com/05b3a3f7cada9f1fe67ef2ff4f73f013525975f3/ui/views/mus/screen_mus_unittest.cc
,
Aug 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5613aca3bca8598bc814598bb8c90ddc5cd001b1 commit 5613aca3bca8598bc814598bb8c90ddc5cd001b1 Author: Mike Wasserman <msw@chromium.org> Date: Thu Aug 09 02:11:07 2018 ws: Fix top-level window placement and touch gesture handling. This fixes most known Keyboard Shortcut Viewer app touch defects: Only target the root when touch events are outside the associated display. (in clients, root windows are top-level app windows, not display-sized roots) Fix placement of top-level client app windows by using screen coordinates. (use ScreenPositionClient and screen bounds for top-levels; add test) Use GetDisplayForNewWindows in CreateInitParamsForTopLevel. (place window service top-level windows on the correct display) TODO: Fix capture release from EditingHandleView gesture event handling. (it continues consuming events even after touch drag/scroll release) Bug: 867559 Test: KSV (Ctrl-Alt-/) window placement and touch interaction. No regression of bezel touch gestures, etc. Change-Id: I4fabcb9ac787743b1e80ec54e615bd5443e580d8 Reviewed-on: https://chromium-review.googlesource.com/1152102 Commit-Queue: Michael Wasserman <msw@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#581760} [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/services/ui/ws2/BUILD.gn [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/services/ui/ws2/client_root.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/services/ui/ws2/window_tree.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/services/ui/ws2/window_tree_unittest.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/aura/client/screen_position_client.h [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/aura/mus/window_tree_client.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/aura/mus/window_tree_host_mus_init_params.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/aura/window_targeter.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/views/mus/desktop_window_tree_host_mus.cc [modify] https://crrev.com/5613aca3bca8598bc814598bb8c90ddc5cd001b1/ui/views/mus/desktop_window_tree_host_mus_unittest.cc
,
Aug 9
That CL didn't fix the capture issue with the text edit handles...
,
Aug 9
Here are all the remaining touch issues I know about with the KSV app:
(1) Double/triple tapping on text does not select the word/line.
(2) After touching one text selection handle, it does not release capture.
(3) Touching the inactive window content does not activate the app window.
(4) The very first touch on the window cannot be used scroll the shortcuts.
(this tangential defect also reproduces when running the non-app KSV)
We're disabling the app for M-69, so this is no longer blocking.
,
Aug 13
I filed bugs for the remaining touch issues (so I'm closing this one): (1) Double/triple tapping on text does not select the word/line. Issue 872891 (2) After touching one text selection handle, it does not release capture. Issue 873743 (3) Touching the inactive window content does not activate the app window. Issue 873763 (4) The very first touch on the window cannot be used scroll the shortcuts. Issue 873770 (this tangential defect also reproduces when running the non-app KSV)
,
Aug 13
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sky@chromium.org
, Jul 26