Input stops working after pressing Assistant Key |
|||||||||||
Issue descriptionChrome version: 67.0.3381.0 dev OS: Chrome Repro steps: 1. Open Linux app using go/crostini-dogfood 2. Tap Assistant Key on Pixelbook keyboard 3. Try to continue typing in app Expected: Characters continue to appear in app Actual: Characters are no longer received From reveman: It looks like https://cs.chromium.org/chromium/src/components/exo/keyboard.cc?l=56 starts returning true for all Crostini windows after this key is pressed. Doesn't happen for ARC++ windows for some reason. We basically enter a state where Chrome thinks IME is enabled and stops sending standard key events to the client. That's at least what it looks like from analyzing the behavior on the client side and looking at the code in Chrome. From kinaba: So far I agree with David's guess that ConsumedByIme() is unexpectedly eating the keys. Current ARC IME impl starts consuming keys when (exo surface is focused && text input is active inside Android worked), and I assumed the latter was always false when the focus was not on an ARC app (because the dummy ARC home app should be the one active in Android world). But apparently Assistant looks hanging around even when ARC is unfocused.
,
Apr 9 2018
> Looks like IsArcWindow implementation in arc_ime_service.cc is incorrect. Yes, that's one of the cause, and limiting it strictly to ARC window indeed fixes the issue. Invocation in OnWindowInitialized is just an installation of the observer to the root window, so it shouldn't cause trouble even if it checked wider type of windows. Fixing only the other call site OnWindowFocused() should resolve the current issue. I think we should anyway first apply the fix. yhanada@, could you work on this direction? The other cause of the issue is that somehow Assistant keeps being considered as focused (by ArcImeService) in the Android side even when it's not active. That is totally unexpected and breaks our assumptions... we need to investigate what's going on in this case as well (probably in a separate issue from the current one.)
,
Apr 9 2018
I agree. Two issues here. Let's fix them both.
,
Apr 9 2018
,
Apr 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82b897f9b746cac54251b077874b60c596a07f87 commit 82b897f9b746cac54251b077874b60c596a07f87 Author: Yuichiro Hanada <yhanada@chromium.org> Date: Mon Apr 09 15:46:04 2018 Fix IsArcWindow() to check an application id of a window. ARC windows were the only one kind of clients, but it changed. We have to check an application id to distinguish ARC windows and other exo windows. Bug: 829383 Test: Manual. Confirm that input is still working on Linux apps after opening Android apps with a text input. Change-Id: Ib58a6e5b41cdd2a3c68d76b3baacd93f11602f70 Reviewed-on: https://chromium-review.googlesource.com/1001086 Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Reviewed-by: David Reveman <reveman@chromium.org> Reviewed-by: Hidehiko Abe <hidehiko@chromium.org> Cr-Commit-Position: refs/heads/master@{#549183} [modify] https://crrev.com/82b897f9b746cac54251b077874b60c596a07f87/components/arc/ime/arc_ime_service.cc [modify] https://crrev.com/82b897f9b746cac54251b077874b60c596a07f87/components/arc/ime/arc_ime_service.h [modify] https://crrev.com/82b897f9b746cac54251b077874b60c596a07f87/components/arc/ime/arc_ime_service_unittest.cc [modify] https://crrev.com/82b897f9b746cac54251b077874b60c596a07f87/components/exo/shell_surface_base.cc [modify] https://crrev.com/82b897f9b746cac54251b077874b60c596a07f87/components/exo/shell_surface_base.h
,
Apr 16 2018
,
Apr 16 2018
I realized IsArcWindow() returns false on notification surfaces.
,
Apr 16 2018
+yoshiki@, Do you know how to distinguish Surfaces from a notification and Surfaces from other windows?
,
Apr 17 2018
I think setting a window property to notification surface is one way. Dominik or David may have better idea.
,
Apr 17 2018
Notification surfaces are not used outside arc++ so detecting this type of surface based on if window name is ExoNotificationSurface is fine as a short term fix. Setting the application ID would be a better long term solution.
,
Apr 17 2018
Checking a window name sgtm as for now
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f66fc4e3d7da3cfa2afed5f8fd82f70fa5c28b6 commit 6f66fc4e3d7da3cfa2afed5f8fd82f70fa5c28b6 Author: Yuichiro Hanada <yhanada@chromium.org> Date: Wed Apr 18 15:49:13 2018 Make IsArcWindow() return true for notification surfaces. Checking a window name is OK for now because NotificationSurfaces are not used outside ARC++. We will set an application id for a notification surface and use it in a follow up CL. Bug: 829383 , 833259 Test: Manual. Inline reply on ARC notifications works. Change-Id: Icf3d43df527988f92ca88f7b89b673fdd02b00bc Reviewed-on: https://chromium-review.googlesource.com/1015440 Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org> Reviewed-by: David Reveman <reveman@chromium.org> Reviewed-by: Yusuke Sato <yusukes@chromium.org> Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#551694} [modify] https://crrev.com/6f66fc4e3d7da3cfa2afed5f8fd82f70fa5c28b6/components/arc/ime/arc_ime_service.cc
,
Apr 25 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/395c5df65759b1c8790b16dbd3b1f28fd47395ea commit 395c5df65759b1c8790b16dbd3b1f28fd47395ea Author: Yuichiro Hanada <yhanada@chromium.org> Date: Wed Apr 25 21:38:46 2018 Make IsArcWindow() return true for notification surfaces. Checking a window name is OK for now because NotificationSurfaces are not used outside ARC++. We will set an application id for a notification surface and use it in a follow up CL. Bug: 829383 , 833259 Test: Manual. Inline reply on ARC notifications works. Change-Id: Icf3d43df527988f92ca88f7b89b673fdd02b00bc Reviewed-on: https://chromium-review.googlesource.com/1015440 Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org> Reviewed-by: David Reveman <reveman@chromium.org> Reviewed-by: Yusuke Sato <yusukes@chromium.org> Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#551694}(cherry picked from commit 6f66fc4e3d7da3cfa2afed5f8fd82f70fa5c28b6) Reviewed-on: https://chromium-review.googlesource.com/1028890 Reviewed-by: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#310} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/395c5df65759b1c8790b16dbd3b1f28fd47395ea/components/arc/ime/arc_ime_service.cc
,
Apr 26 2018
Fix for M-67 is done.
,
May 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9527bb4d9c79ac8bbc342153e3166d06188bee2 commit d9527bb4d9c79ac8bbc342153e3166d06188bee2 Author: Yuichiro Hanada <yhanada@chromium.org> Date: Wed May 09 00:54:36 2018 Add new request to notification_surface to set an application id. An application id of notification surfaces is needed to distinguish notification surfaces from ARC++ apps from surfaces from other exo clients. IsArcNotificationWindow() in arc_ime_service.cc will be updated after client side code is updated. Bug: 829383 , 834027 Test: Build pass & Inline reply on ARC++ notifications works corretly. Change-Id: I471d8f6be2fdd40e02178cdeb1a27b4e71b03842 Reviewed-on: https://chromium-review.googlesource.com/1039505 Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Reviewed-by: David Reveman <reveman@chromium.org> Cr-Commit-Position: refs/heads/master@{#557035} [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/components/exo/notification_surface.cc [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/components/exo/notification_surface.h [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/components/exo/wayland/server.cc [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/third_party/wayland-protocols/include/protocol/remote-shell-unstable-v1-client-protocol.h [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/third_party/wayland-protocols/include/protocol/remote-shell-unstable-v1-server-protocol.h [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/third_party/wayland-protocols/protocol/remote-shell-protocol.c [modify] https://crrev.com/d9527bb4d9c79ac8bbc342153e3166d06188bee2/third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml
,
May 9 2018
,
May 10 2018
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a4ed14123d1f6f2593eb80ca9fed35ac88c98396 commit a4ed14123d1f6f2593eb80ca9fed35ac88c98396 Author: Yuichiro Hanada <yhanada@chromium.org> Date: Tue May 15 22:31:10 2018 Increase zcr_remote_shell_v1 version and zcr_notification_surface_v1 version to 16. The version of the global interface should be increased when the protocol is modified, as it is how we detect a version incompatibility immediately on bootup. For consistency, the version of zcr_notification_surface_v1 is also increated in this CL. Bug: 829383 , 834027 Change-Id: I33e8fbdbe0798e70743ad2af83e4ada80e7797dc Reviewed-on: https://chromium-review.googlesource.com/1055030 Reviewed-by: David Reveman <reveman@chromium.org> Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#558862} [modify] https://crrev.com/a4ed14123d1f6f2593eb80ca9fed35ac88c98396/third_party/wayland-protocols/include/protocol/remote-shell-unstable-v1-client-protocol.h [modify] https://crrev.com/a4ed14123d1f6f2593eb80ca9fed35ac88c98396/third_party/wayland-protocols/include/protocol/remote-shell-unstable-v1-server-protocol.h [modify] https://crrev.com/a4ed14123d1f6f2593eb80ca9fed35ac88c98396/third_party/wayland-protocols/protocol/remote-shell-protocol.c [modify] https://crrev.com/a4ed14123d1f6f2593eb80ca9fed35ac88c98396/third_party/wayland-protocols/unstable/remote-shell/remote-shell-unstable-v1.xml
,
May 21 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eba861a06284fbf60bb0e6b6bddcdadf98f900e5 commit eba861a06284fbf60bb0e6b6bddcdadf98f900e5 Author: Yuichiro Hanada <yhanada@chromium.org> Date: Mon May 21 08:49:17 2018 Update actually used zcr_remote_surface_v1 interface version to 16. Bug: 829383 , 834027 Change-Id: I440f2b11b8cfe8499af913b508dcda535c554cf4 Reviewed-on: https://chromium-review.googlesource.com/1060873 Reviewed-by: David Reveman <reveman@chromium.org> Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#560249} [modify] https://crrev.com/eba861a06284fbf60bb0e6b6bddcdadf98f900e5/components/exo/wayland/server.cc
,
May 23 2018
,
May 30 2018
This appears to be fixed on 69.0.3443.0 / 10729.0.0 canary. Please re-open if needed.
,
Jun 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b8061fef925a967ceede0eb302d3256ab0bc8dd commit 9b8061fef925a967ceede0eb302d3256ab0bc8dd Author: Yuichiro Hanada <yhanada@chromium.org> Date: Wed Jun 06 22:51:27 2018 Use application id in NotificationSurface to distinguish surfaces of ARC notifications. Bug: 829383 , 834027 Test: IME works on ARC++ apps and notifications from ARC++ apps. Change-Id: I98d7cb43845c668ad0afb273c623b49abf08f595 Reviewed-on: https://chromium-review.googlesource.com/1051433 Reviewed-by: Yusuke Sato <yusukes@chromium.org> Commit-Queue: Yuichiro Hanada <yhanada@chromium.org> Cr-Commit-Position: refs/heads/master@{#565075} [modify] https://crrev.com/9b8061fef925a967ceede0eb302d3256ab0bc8dd/components/arc/arc_util.cc [modify] https://crrev.com/9b8061fef925a967ceede0eb302d3256ab0bc8dd/components/arc/arc_util.h [modify] https://crrev.com/9b8061fef925a967ceede0eb302d3256ab0bc8dd/components/arc/ime/arc_ime_service.cc |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by reve...@chromium.org
, Apr 6 2018