New issue
Advanced search Search tips

Issue 876589 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Team-Accessibility



Sign in to add a comment

CrOS Shortcut Viewer a11y: search box doesn't bring up onscreen keyboard

Project Member Reported by leberly@chromium.org, Aug 22

Issue description

70.0.3524.2 (Official Build) canary (64-bit)
Google_Eve.9584.160.0

Steps to repro:
# Enable On-screen keyboard in Settings > Manage Accessibility Features
# Launch keyboard shortcuts app using ctrl + alt + /
# Click or double click in the search field
Expected: on-screen keyboard expands 
Actual: on-screen keyboard says collapsed

Once you manually expand the on-screen keyboard, the input is received 
 
Status: WontFix (was: Available)

Checking again with flag disabled: 

70.0.3524.2 (Official Build) canary (64-bit)
Google_Eve.9584.160.0
#ash-keyboard-shortcut-viewer-app set to 'Disabled' 

Does not reproduce using above steps; closing as WontFix. 
Status: Available (was: WontFix)
Actually, reopening since I'll let the team decide whether or not to close this bug based on the above finding. 
Cc: msw@chromium.org
Labels: Proj-Mash-KSV
Yes, please keep any bugs specific to the app open, thanks!
(if you see bugs that are app-only, please also use the label "Proj-Mash-KSV")
Owner: msw@chromium.org
Status: Started (was: Available)
I have a WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/1185736
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 23

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

commit d10527cab466ac073da1c572a63b2a34d59015ef
Author: Mike Wasserman <msw@chromium.org>
Date: Thu Aug 23 07:10:54 2018

ws: Add InputMethodMus plumbing for ShowVirtualKeyboardIfEnabled

TODO: Get the keyboard to close when closing the KSV window.

Bug:  876589 
Test: a11y virtual keyboard shows on second KSV textfield click.
Change-Id: I57a09acb9cf8b784a9c40763028c4733dd43d474
Reviewed-on: https://chromium-review.googlesource.com/1185736
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585418}
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/chrome/browser/chromeos/input_method/input_method_manager_impl.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.h
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/chrome/browser/ui/views/ime_driver/simple_input_method.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/chrome/browser/ui/views/ime_driver/simple_input_method.h
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/services/ui/ime/test_ime_driver/test_ime_driver.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/services/ui/public/interfaces/ime/ime.mojom
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/ui/aura/mus/input_method_mus.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/ui/aura/mus/input_method_mus.h
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/ui/aura/mus/input_method_mus_unittest.cc
[modify] https://crrev.com/d10527cab466ac073da1c572a63b2a34d59015ef/ui/base/ime/input_method_base.cc

AFAICT, ui::KeyboardController only observes one InputMethod (the one supplied by KeyboardUI):
https://cs.chromium.org/chromium/src/ui/keyboard/keyboard_controller.cc?rcl=6bd345017a9cf81abd12c271e6a635a084ed9252&l=198

So, it isn't aware of the InputMethodChromeOS instance created by InputMethodBridge:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc?rcl=75a4d21e24696dfb7ed4934257d8c83a0307d656&l=27

Normally it closes via OnTextInputStateChanged (also triggered on DetachTextInputClient).
Here are the good callstacks when closing a browser window (posing task and task execution):

#1 0x7f7f79627690 keyboard::KeyboardController::HideKeyboardImplicitlyBySystem()
#2 0x7f7f79627c72 keyboard::KeyboardController::OnTextInputStateChanged()
#3 0x7f7f7aaf5a85 ui::InputMethodBase::NotifyTextInputStateChanged()
#4 0x7f7f7aaf58db ui::InputMethodBase::DetachTextInputClient()
#5 0x7f7f791735bf views::Textfield::OnBlur()
#6 0x55957631447f OmniboxViewViews::OnBlur()
#7 0x7f7f7919897f views::View::Blur()
#8 0x7f7f79181159 views::FocusManager::SetFocusedViewWithReason()
#9 0x7f7f791a0b83 views::Widget::Close()

#1 0x7fe8dd958ed7 keyboard::KeyboardController::HideKeyboard()
#2 0x7fe8dd95d42b _ZN4base8internal7InvokerINS0_9BindStateIMN8keyboard18KeyboardControllerEFvNS4_10HideReasonEEJNS_7WeakPtrIS4_EES5_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#3 0x7fe8e3404e55 base::debug::TaskAnnotator::RunTask()

Maybe we should avoid multiple InputMethod instances, and just notify the one InputMethodChromeOS of RemoteTextInputClient changes?
Otherwise, I guess KeyboardController will need to track multiple InputMethod instances (for the active/focused window?)
I have a naive WIP solution, but it might not handle all corner cases (activating another mojo window with a focused textfield on window close, etc.
https://chromium-review.googlesource.com/c/chromium/src/+/1188453
Cc: jamescook@chromium.org sky@chromium.org dxie@chromium.org xiy...@chromium.org steve...@chromium.org
CC'ing some other folks that might be interested, I'll be out for two weeks.

This would be nice to fix for M-70, but not critical for launching KSV, IMO.
Hopefully there aren't any other IME issues with the KSV app codepath...
To be clear, comment #5 fixed showing the virtual keyboard when focusing the textfield, the keyboard just doesn't close when closing the window.
Cc: shuchen@chromium.org shend@chromium.org
+shuchen/shend - It seems like this is a conflict between mash/mustash creating more than one InputMethodChromeOS (the "shared" one plus one for the shortcut viewer app) and the VK assuming there is only one.

Do we know how InputMethodChromeOS will end up after the IME mojo-ification effort?

Owner: xiy...@chromium.org
Xiyuan, could you take this over as Mike is out.
Status: Fixed (was: Started)
CL in #11 landed on trunk. VK should close after KSV window close.
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 5

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

commit dfb07b25e104013694de5bacd87f73a9692fadfe
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Tue Sep 04 21:44:38 2018

cros: Fix VK not closing when KSV closes

- Make KeyboardController update its observed IME when it is
  enabled/activated/shown. There could be multiple IME instances
  with mojo apps. This allows KeyboardController to follow
  the active IME;
- Change ChromeKeyboardUI to return the active IME from IMEBridge
  instead of the globally shared IME;

Bug:  876589 
Change-Id: Ieb4cde7131969da3c9182d6bd5447dbe604440fd
Reviewed-on: https://chromium-review.googlesource.com/1194986
Reviewed-by: Shu Chen <shuchen@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588653}
[modify] https://crrev.com/dfb07b25e104013694de5bacd87f73a9692fadfe/chrome/browser/ui/ash/chrome_keyboard_ui.cc
[modify] https://crrev.com/dfb07b25e104013694de5bacd87f73a9692fadfe/ui/keyboard/keyboard_controller.cc
[modify] https://crrev.com/dfb07b25e104013694de5bacd87f73a9692fadfe/ui/keyboard/keyboard_controller.h

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 26

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

commit 0f427007daca2dd5cbbac4908aaffc93d37b184e
Author: Mike Wasserman <msw@chromium.org>
Date: Wed Dec 26 15:23:26 2018

mash: Fix a11y magnifier following caret bounds in SingleProcessMash and KSV

Make IMEBridge support multiple observers (per xiyuan@'s crrev.com/c/1197846)
Make MagnificationController observe the active app's IME instance.

Bug:  876589 
Test: Magnifier (Search-Ctrl-m) follows caret focus in SPM and KSV (Ctrl-Alt-/)
Change-Id: Id97210ece4c3ad359cb4b31b45c02f5fb020b8e4
Reviewed-on: https://chromium-review.googlesource.com/c/1387669
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Shu Chen <shuchen@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618927}
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/ash/magnifier/magnification_controller.h
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/chrome/browser/extensions/api/input_ime/input_ime_api.cc
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/chrome/browser/extensions/api/input_ime/input_ime_api_nonchromeos.cc
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/ui/base/ime/ime_bridge.cc
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/ui/base/ime/ime_bridge.h
[modify] https://crrev.com/0f427007daca2dd5cbbac4908aaffc93d37b184e/ui/base/ime/ime_bridge_observer.h

Woops, that CL above should have been marked BUG= 876510 

Sign in to add a comment