New issue
Advanced search Search tips

Issue 867559 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 841020



Sign in to add a comment

Shortcut Viewer app doesn't support some touch interactions

Project Member Reported by msw@chromium.org, Jul 25

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
 
Cc: sadrul@chromium.org
I suspect this is the same as 866991. I suspect with --mash something is happening that is causing the GestureRecognizer in remote apps not to recognize gestures. This may be related to events in remote apps having a target, where as in ash (and classic ash) that isn't the case.

I suggest looking into GestureRecognizerImpl and GestureProviderAura. My one minute investigation seemed to indicate both of these are being using in the remote app (which we want), but no doubt something isn't happening right.
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.
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.
Correction, the app gets ET_POINTER_* with POINTER_TYPE_TOUCH, not ET_TOUCH.
Does the ET_POINTER_* events get converted to ET_TOUCH* events before it reaches aura in the client?
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.
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.
Labels: Pri-1
Release blockers should be P1 or P0
Thanks, I am making good progress on a solution: https://chromium-review.googlesource.com/c/chromium/src/+/1152102
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.
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Project Member

Comment 13 by bugdroid1@chromium.org, 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

That CL didn't fix the capture issue with the text edit handles...
Labels: -ReleaseBlock-Stable -M-69 M-70
Summary: Shortcut Viewer app doesn't support some touch interactions (was: Shortcut Viewer app doesn't support some textfield touch interactions)
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.
Labels: Proj-Mash-KSV
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)
Status: Fixed (was: Started)

Sign in to add a comment