New issue
Advanced search Search tips

Enable keyboard shortcut viewer launching as an app

Project Member Reported by sky@chromium.org, May 8 2018

Issue description

The keyboard shortcut viewer does not use a lot of ash or the WindowService. We should convert it to exclusively launch as an app using the WindowService.
 
Cc: ovanieva@chromium.org wutao@chromium.org
Components: -UI>Shell UI>Input>KeyboardShortcuts

Comment 2 by sky@chromium.org, May 14 2018

Owner: msw@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by msw@chromium.org, May 15 2018

Status: Started (was: Assigned)
I have a WIP CL that runs KSV as a mojo app, but it's definitely not ready to land:
  https://chromium-review.googlesource.com/c/chromium/src/+/1060163

We'll need to address the blocking bugs and these issues:
1) KSV depends on an InputDeviceManager, which doesn't exist -> Instantiate InputDeviceClient (in MusClient?)
2) Opening KSV hits a number of NOTIMPLEMENTED [1], likely some are covered by blocking bugs.
3) At first, the titlebar paints with empty window content; after dragging, the window content paints with a blank titlebar. [2]
4) Attempting to open the KSV a second time after closing/crashing does nothing.
5) The window content doesn't get any input (likely  Issue 837692 )
6) Closing the window causes a crash. [3]

[1] Here are the NOTIMPLEMENTED()s hit when first opening KSV:
[215631:215631:0515/131248.231223:ERROR:window_service.cc(95)] Not implemented reached in void ui::ws2::WindowService::BindClipboardRequest(mojom::ClipboardRequest)
[215631:215631:0515/131251.129983:ERROR:window_service_client.cc(785)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetHitTestMask(ui::Id, const base::Optional<gfx::Rect> &)
[215631:215631:0515/131251.130044:ERROR:window_service_client.cc(789)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetCanAcceptDrops(ui::Id, bool)
[215631:215631:0515/131251.130926:ERROR:window_service_client.cc(875)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetModalType(uint32_t, ui::Id, ui::ModalType)
[215631:215631:0515/131251.132135:ERROR:window_service.cc(105)] Not implemented reached in void ui::ws2::WindowService::BindImeDriverRequest(mojom::IMEDriverRequest)
[215631:215631:0515/131251.550846:ERROR:window_service_client.cc(779)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetClientArea(ui::Id, const gfx::Insets &, const base::Optional<std::vector<gfx::Rect> > &)
[215631:215631:0515/131251.550906:ERROR:window_service_client.cc(785)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetHitTestMask(ui::Id, const base::Optional<gfx::Rect> &)
[215631:215631:0515/131251.555213:ERROR:window_service_client.cc(925)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetFocus(uint32_t, ui::Id)
[215631:215631:0515/131251.555273:ERROR:window_service_client.cc(925)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetFocus(uint32_t, ui::Id)
[215631:215631:0515/131251.555321:ERROR:window_service_client.cc(941)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetWindowTextInputState(ui::Id, ::ui::mojom::TextInputStatePtr)
[215631:215631:0515/131251.555944:ERROR:window_service_client.cc(948)] Not implemented reached in virtual void ui::ws2::WindowServiceClient::SetImeVisibility(ui::Id, bool, ::ui::mojom::TextInputStatePtr)

[2] Here is the NOTIMPLEMENTED() hit when dragging the window the first time:
[215631:215631:0515/131305.289380:ERROR:window_host_frame_sink_client.cc(21)] Not implemented reached in virtual void ui::ws2::WindowHostFrameSinkClient::OnFirstSurfaceActivation(const viz::SurfaceInfo &)

[3] Here is the crash callstack when closing the KSV window:
Received signal 11 SEGV_MAPERR fffffffc0566a2c5
#0 0x7f532bb7b1dc base::debug::StackTrace::StackTrace()
#1 0x7f532bb7acd1 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f532bc3c0c0 <unknown>
#3 0x7f532732c3e6 aura::Window::~Window()
#4 0x7f532732cc3e aura::Window::~Window()
#5 0x7f531cfd57ce ui::ws2::WindowServiceClient::RemoveWindowFromKnownWindows()
#6 0x7f531cfd9637 ui::ws2::WindowServiceClient::OnWindowDestroyed()
#7 0x7f532732c69c aura::Window::~Window()
#8 0x7f532732cc3e aura::Window::~Window()
#9 0x7f5325e83897 _ZN4base8internal7InvokerINS0_9BindStateIMN5views12MouseWatcher8ObserverEFvvEJNS_7WeakPtrIS5_EEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
Project Member

Comment 4 by bugdroid1@chromium.org, May 17 2018

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

commit 10d854c4b70ffffcb28905f08eac7de4903d2793
Author: Mike Wasserman <msw@chromium.org>
Date: Thu May 17 05:29:59 2018

ws: Hookup InputDeviceServer/Client for Window Service as a library.

Add an InputDeviceServer instance to ui::ws2::WindowService.
Add an InputDeviceClient instance to MusClient, remove from Chrome.
(allows mojo apps using ws2 & MusClient to access InputDeviceManager)

Split out TouchDeviceServer ownership to ui::Service.
(IIUC, this won't be needed at all for ash->ws2 usage)

Refine the server interface registration (BindSourceInfo not needed).
Make the shortcut viewer dependency on InputDeviceManager explicit.
Fix the DCHECK in ksv unit tests, and a crash showing the KSV in Mash.

This is a prerequisite for making Keyboard Shortcut Viewer a mojo app:
  https://chromium-review.googlesource.com/c/chromium/src/+/1060163

Bug:  841020 
Change-Id: I9de8e9ccf249aa40e7a297261d9d2e0f2989fbfe
Reviewed-on: https://chromium-review.googlesource.com/1062770
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559428}
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ash/BUILD.gn
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ash/components/shortcut_viewer/BUILD.gn
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ash/components/shortcut_viewer/DEPS
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ash/components/shortcut_viewer/keyboard_shortcut_viewer_metadata.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/chrome/browser/ui/views/chrome_browser_main_extra_parts_views.h
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/input_devices/input_device_server.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/input_devices/input_device_server.h
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/input_devices/touch_device_server.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/input_devices/touch_device_server.h
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/service.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/service.h
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/ws2/BUILD.gn
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/ws2/window_service.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/services/ui/ws2/window_service.h
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ui/views/mus/BUILD.gn
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ui/views/mus/mus_client.cc
[modify] https://crrev.com/10d854c4b70ffffcb28905f08eac7de4903d2793/ui/views/mus/mus_client.h

Project Member

Comment 5 by bugdroid1@chromium.org, May 18 2018

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

commit f20cf06e0b077953a4585688c4d8b78aa6dff2dc
Author: Mike Wasserman <msw@chromium.org>
Date: Fri May 18 21:01:41 2018

ws: Add POINTER_ENTERED and POINTER_CAPTURE_CHANGED mojo event types.

Change one-off conversion functions TypeConverters for tracking.
Make enum naming and ordering match ui::EventType.

This fixes some crashes observed running KSV as a mojo app with ws2:
  https://chromium-review.googlesource.com/c/chromium/src/+/1060163

Bug:  841020 ,  837692 
Change-Id: If3e33ac6a35d5fb09beb4317140c1c04d18b4eb5
Reviewed-on: https://chromium-review.googlesource.com/1065199
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560032}
[modify] https://crrev.com/f20cf06e0b077953a4585688c4d8b78aa6dff2dc/services/ui/ws/event_matcher.cc
[modify] https://crrev.com/f20cf06e0b077953a4585688c4d8b78aa6dff2dc/ui/events/mojo/event_constants.mojom
[modify] https://crrev.com/f20cf06e0b077953a4585688c4d8b78aa6dff2dc/ui/events/mojo/event_struct_traits.cc
[modify] https://crrev.com/f20cf06e0b077953a4585688c4d8b78aa6dff2dc/ui/events/mojo/event_struct_traits.h
[modify] https://crrev.com/f20cf06e0b077953a4585688c4d8b78aa6dff2dc/ui/events/mojo/struct_traits_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 18 2018

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

commit e98cc618c31b190cc8db1cc49745b82736fcaecd
Author: Mike Wasserman <msw@chromium.org>
Date: Fri May 18 22:55:27 2018

ws: Send events to the owning client when the embedded one is null.

This fixes a crash observed running KSV as a mojo app with ws2:
  https://chromium-review.googlesource.com/c/chromium/src/+/1060163

Bug:  841020 ,  837692 
Change-Id: I0ea7f2a9b503473f255004b1effc8dcf72cfa5ff
Reviewed-on: https://chromium-review.googlesource.com/1066742
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560084}
[modify] https://crrev.com/e98cc618c31b190cc8db1cc49745b82736fcaecd/services/ui/ws2/client_window.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 22 2018

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

commit 65386c1cacbfaaaf928f785f67fa2babad1ee9d1
Author: Mike Wasserman <msw@chromium.org>
Date: Tue May 22 16:59:40 2018

mash: Make Keyboard Shortcut Viewer a mojo application

Launches KSV as a mojo app with switch --keyboard-shortcut-viewer-app
(otherwise, use the existing flow via chrome including the library)
Allows development and bug fixing as we bring up the window service.

Add a manifest + mojom, implement Service, register in chrome + utility.
(mostly following the example of the touch_hud application)
Add a switch to run the app, make the app wait for input device listing.

TODO: Fix various bugs before running KSV as an app by default.

Bug:  841020 
Change-Id: I13b4f69a049312d87730f686dee26182c3664b13
Reviewed-on: https://chromium-review.googlesource.com/1060163
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560659}
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/BUILD.gn
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/BUILD.gn
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/OWNERS
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/manifest.json
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/public/mojom/BUILD.gn
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/public/mojom/OWNERS
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/public/mojom/constants.mojom
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/shortcut_viewer_application.cc
[add] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/components/shortcut_viewer/shortcut_viewer_application.h
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/public/cpp/ash_switches.cc
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/ash/public/cpp/ash_switches.h
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/app/BUILD.gn
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/browser/DEPS
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/browser/ash_service_registry.cc
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/browser/ui/ash/ksv/DEPS
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/utility/BUILD.gn
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/utility/DEPS
[modify] https://crrev.com/65386c1cacbfaaaf928f785f67fa2babad1ee9d1/chrome/utility/mash_service_factory.cc

Comment 8 by msw@chromium.org, May 25 2018

On ToT the app is painting and handling key/mouse input in the content area correctly; yay! Remaining known issues:
1) Slow startup: there's a multi-second delay for the window to show up when opening it via chrome://settings/keyboard-overlay
2) Bounds/display: opens on the primary (not active) display, isn't centered ( issue 768908 ?)
3) Caption buttons: Can't click the minimize/close caption buttons
4) Crash on close: Closing the window crashes via CTRL+W* (and maybe via the close caption button?)
5) Activate/Close accelerator: Need to support keyboard accelerator to activate/close (see  Issue 819815 )

* Crash on pressing CTRL+W to close the KSV:
Received signal 11 SEGV_MAPERR 000000000120
#0 0x7f2ec04d56fc base::debug::StackTrace::StackTrace()
#1 0x7f2ec04d51f1 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f2eb3ca80c0 <unknown>
#3 0x7f2ebbcb8200 <unknown>
#4 0x7f2ebbcc0864 aura::WindowObserver::OnObservingWindow()
#5 0x7f2ebbcb81e8 aura::Window::AddObserver()
#6 0x7f2eb160a300 ui::ws2::ClientChange::ClientChange()
#7 0x7f2eb160d36b ui::ws2::FocusHandler::SetFocus()
#8 0x7f2eb1617fa5 ui::ws2::WindowServiceClient::SetFocusImpl()
#9 0x7f2eb161a12f ui::ws2::WindowServiceClient::SetFocus()
#10 0x7f2eb162f33f ui::mojom::WindowTreeStubDispatch::Accept()

Comment 9 by wutao@chromium.org, May 25 2018

Got the same error in #8 when adding "ctrl+alt+/" accelerator to the window.

Comment 10 by msw@chromium.org, May 25 2018

I think that's okay if you get the crash when using the accelerator to close the window, it's just triggering a tangential crash-on-close that shouldn't affect the non-app KSV window.

Comment 11 by sky@chromium.org, May 29 2018

Blockedon: -837715

Comment 12 by sky@chromium.org, May 29 2018

Blockedon: 847613

Comment 13 by sky@chromium.org, May 29 2018

Blockedon: 847615

Comment 14 by sky@chromium.org, May 29 2018

I've filed a couple more bugs. Fix for caption buttons is out for review. Will look into crash on close next.
Project Member

Comment 15 by bugdroid1@chromium.org, May 30 2018

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

commit 939f8f6303c9286666282f344ac00c4f9040f885
Author: Scott Violet <sky@chromium.org>
Date: Wed May 30 00:07:02 2018

chromeos: fixs crash in WindowService if SetFocus(null) was called

BUG= 841020 
TEST=covered by test

Change-Id: I634286b91aae1478d11a7ff7e7a4401ca8802351
Reviewed-on: https://chromium-review.googlesource.com/1077611
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562661}
[modify] https://crrev.com/939f8f6303c9286666282f344ac00c4f9040f885/services/ui/ws2/client_change.cc
[modify] https://crrev.com/939f8f6303c9286666282f344ac00c4f9040f885/services/ui/ws2/focus_handler_unittest.cc
[modify] https://crrev.com/939f8f6303c9286666282f344ac00c4f9040f885/services/ui/ws2/window_service_client_test_helper.cc

Blockedon: 847982
Found a new bug, when KSV window is active, all global accelerators seem do not work, including ctrl + n, launcher, etc.



Comment 18 by msw@chromium.org, Jun 1 2018

Blockedon: 848883

Comment 19 by msw@chromium.org, Jun 1 2018

Blockedon: 848884

Comment 20 by msw@chromium.org, Jun 1 2018

I filed  issue 848883  for the accelerators, thanks.
Project Member

Comment 21 by bugdroid1@chromium.org, Jun 1 2018

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

commit 43e5034df2e87fe2dd959aa2c23e795246711d3a
Author: James Cook <jamescook@chromium.org>
Date: Fri Jun 01 20:51:17 2018

chromeos: Make shortcut_viewer app appear on correct display

The mojo app version of shortcut_viewer can't use an aura::Window to
provide context to choose a display. Instead, provide neither context
nor a parent, which makes ash place the window on the display for new
windows. Don't provide bounds (since that would force the window to
a particular display). Instead, provide a preferred size.

This requires fixes to native widget initialization to make sure
the default aura::Window context (which is RootWindowForNewWindows)
provides the correct default display when there is no parent.

The test for this live in ash_unittests because I didn't want to add
an ash/shell.h dependency to keyboard_shortcut_viewer.

TBR=sky@chromium.org

Bug:  847982 ,  833673 ,  841020 
Test: added
Change-Id: Id3179d1c4d9b39911aded19fe171491f479fe4db
Reviewed-on: https://chromium-review.googlesource.com/1081174
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563786}
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/components/shortcut_viewer/shortcut_viewer_application.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/components/shortcut_viewer/views/keyboard_shortcut_view.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/components/shortcut_viewer/views/keyboard_shortcut_view.h
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/components/shortcut_viewer/views/keyboard_shortcut_view_unittest.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/shell_unittest.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/container_finder.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/container_finder.h
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/non_client_frame_controller.h
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/root_window_finder.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/root_window_finder.h
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ash/wm/top_level_window_factory.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/chrome/browser/ui/ash/ksv/DEPS
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ui/aura/mus/window_tree_host_mus_init_params.cc
[modify] https://crrev.com/43e5034df2e87fe2dd959aa2c23e795246711d3a/ui/views/widget/native_widget_aura.cc

Comment 22 by msw@chromium.org, Jun 4 2018

Cc: dxie@chromium.org

Comment 23 by sky@chromium.org, Jun 4 2018

Blockedon: 849380
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 5 2018

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

commit 549ffb1a7145988316fcd1a8e603557186d81edc
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Tue Jun 05 15:35:45 2018

cros: KSV app quits on last window close

Make KSV mojo app quit when the viewer window is closed. There is no
benefit of keeping the app running and it prevents to show the UI
for 2nd time because the viewer window is only created on start.

Bug:  841020 
Change-Id: Ib6f1a90b1bbdbc016e7fa303109c8ca089d7738c
Reviewed-on: https://chromium-review.googlesource.com/1086234
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564515}
[modify] https://crrev.com/549ffb1a7145988316fcd1a8e603557186d81edc/ash/components/shortcut_viewer/BUILD.gn
[add] https://crrev.com/549ffb1a7145988316fcd1a8e603557186d81edc/ash/components/shortcut_viewer/last_window_closed_observer.cc
[add] https://crrev.com/549ffb1a7145988316fcd1a8e603557186d81edc/ash/components/shortcut_viewer/last_window_closed_observer.h
[modify] https://crrev.com/549ffb1a7145988316fcd1a8e603557186d81edc/ash/components/shortcut_viewer/shortcut_viewer_application.cc
[modify] https://crrev.com/549ffb1a7145988316fcd1a8e603557186d81edc/ash/components/shortcut_viewer/shortcut_viewer_application.h

Comment 25 by msw@chromium.org, Jun 11 2018

Blockedon: 851555

Comment 26 by msw@chromium.org, Jun 11 2018

Blockedon: 851578
Project Member

Comment 27 by bugdroid1@chromium.org, Jun 14 2018

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

commit fcc47e41f0d95b0b354408024c19ece38f166716
Author: Mike Wasserman <msw@chromium.org>
Date: Thu Jun 14 21:24:42 2018

ash: Add a feature flag for the Keyboard Shortcut Viewer mojo app

Default is disabled for now. Remove the old commandline switch.

TBR=isherman@chromium.org

Bug:  841020 
Change-Id: Ie5df91612d6c976b0b7f056c7248dc92cee9be68
Reviewed-on: https://chromium-review.googlesource.com/1101505
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567425}
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/ash/public/cpp/ash_features.cc
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/ash/public/cpp/ash_features.h
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/ash/public/cpp/ash_switches.cc
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/ash/public/cpp/ash_switches.h
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/chrome/browser/about_flags.cc
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc
[modify] https://crrev.com/fcc47e41f0d95b0b354408024c19ece38f166716/tools/metrics/histograms/enums.xml

Comment 28 by sky@chromium.org, Jun 18 2018

Blockedon: -837705
Evan has completed the parts of cursors necessary for ksv. I'm removing cursors as a blocker to this.
Blockedon: 853883

Comment 30 by wutao@chromium.org, Jun 18 2018

Not sure this is a bug or not, when closing the KSV, the client view disappear first but the non-client view (the frame view) takes a little more time to disappear.


Comment 31 by sky@chromium.org, Jun 19 2018

Wu, indeed, we may need to address that at some point.

Comment 32 by sky@chromium.org, Jun 19 2018

Blockedon: 854324
Blockedon: 854742

Comment 34 by msw@chromium.org, Jun 20 2018

Blockedon: 854368
Blockedon: 855658

Comment 36 by dchan@google.com, Jun 25 2018

Cc: abod...@chromium.org dhadd...@chromium.org
+david and ansar since this is launching in M69 
This is mainly a refactor and should not have any visible changes to chromeOS

Comment 37 by dchan@google.com, Jun 25 2018

+msw, dxie, this feature in which version of M69 ? 

Comment 38 by msw@chromium.org, Jun 25 2018

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

commit 11abc1adf573e24e45bea26ddd2da82696fed814
Author: James Cook <jamescook@chromium.org>
Date: Tue Jun 19 00:07:18 2018

chromeos: Turn shortcut viewer app on by default unless a11y enabled

The app version of the shortcut viewer works well, but we're still
waiting on some accessibility support. Turn the app on by default, but
use the old version if certain a11y features are enabled.

Bug:   853883  
Change-Id: I609b155da467bcb051d4d49d660de77863d0e1df
Reviewed-on: https://chromium-review.googlesource.com/1105077
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568242}
[modify] https://crrev.com/11abc1adf573e24e45bea26ddd2da82696fed814/ash/public/cpp/ash_features.cc
[modify] https://crrev.com/11abc1adf573e24e45bea26ddd2da82696fed814/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc

Comment 39 by msw@chromium.org, Jun 25 2018

James' CL above to enable this by default on ToT has not yet reached Chrome OS dev channel

Comment 40 by msw@chromium.org, Jun 25 2018

Labels: M-69
It's unclear if we'll ship on M-69 (perhaps falling back to the non-app version when a11y features are enabled).
We'd like to get some testing and feedback soon, just in case we do try to ship the app version in M-69.

Comment 41 by msw@chromium.org, Jun 25 2018

Blockedon: 856341

Comment 42 by wutao@chromium.org, Jun 26 2018

Blockedon: 856811
Keyboard shortcut viewer opens as an app window on build 10820.0.0, 69.0.3473.0 dev-channel eve. Attached screenshot.

The app doesn't have a maximize button and also unable to resize the app window. Is this expected?
Screenshot 2018-06-27 at 10.15.30 AM.png
3.6 MB View Download
Cc: sdantul...@chromium.org mkarkada@chromium.org

Comment 45 by wutao@chromium.org, Jun 27 2018

#43, yes, not resizable was by design when it was implemented, but we have plan to make it resizable (bug 822377).

Comment 46 by msw@chromium.org, Jun 27 2018

Correct, we only care about differences between the new app version (on-by-default) and the old non-app version.
To check the old non-app version (as a baseline), you can disable "Keyboard Shortcut Viewer mojo app" in chrome://flags
Cc: pbath...@chromium.org
KSV app version works fine on M69 (10820.0.0, 69.0.3473.0).

msw@ c#38 says that old version is used if certain a11y features are enabled. Could you let me know how to verify this?

Comment 48 by msw@chromium.org, Jun 27 2018

#47: For now, you can verify that the old non-app-version is launched when you have ChromeVox enabled (Ctrl-Alt-/), you should be able to get vocalizations about the content within the window. If, on the other hand, you enable ChromeVox while the new app version is already showing, you won't be able to get vocalizations about the content within the window. (you can also tell the difference between app and non-app version by the presence of the "Shortcuts" title in the window frame so long as  Issue 854324  isn't fixed...)
Re #48: Thanks msw@. I verified that it is working as expected. Tested on eve device.

I noticed that the "Keyboard Shortcut Viewer mojo app" flag is not available on other devices like kevin. Is the feature only for eve device? Thanks!!
Please ignore my comment in c#49.

Verified that feature is working fine as expected.

Tested on eve, kevin and minnie devices with build M69 (10820.0.0, 69.0.3473.0).
Blockedon: 867110
Blockedon: 867559
Blockedon: 867654
Blockedon: 868497
Project Member

Comment 55 by bugdroid1@chromium.org, Jul 31

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

commit 3c578fe0ffba6b178296a7c23e54a6281be8f688
Author: James Cook <jamescook@chromium.org>
Date: Tue Jul 31 20:35:49 2018

cros: Use new shortcut viewer app even if a11y features are on

We used to fall back to the old non-app version, but the app works fine
with the focus highlight feature.

Switch access is still under development. dmazzoni said not to
worry about it.

Bug:  841020 
Test: manual, turn on "Highlight object with keyboard focus" and use shortcut viewer app
Change-Id: If45a9e67add82287e39d20633e98c2e992e71119
Reviewed-on: https://chromium-review.googlesource.com/1153350
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579554}
[modify] https://crrev.com/3c578fe0ffba6b178296a7c23e54a6281be8f688/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc

Cc: leberly@chromium.org
Copying myself to spot test this change once it lands in Canary in 70.0.3509.0. 
Blockedon: 870849
Blockedon: 871602
Blockedon: 872891
Blockedon: 871582
Blockedon: 873701
Labels: Proj-Mash-KSV
Blockedon: 873743
Blockedon: 873763
Labels: -M-69 M-70
Project Member

Comment 66 by bugdroid1@chromium.org, Aug 15

Labels: merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b2d84d92648b87ae8aa3cb1209842cb6d352a5a

commit 0b2d84d92648b87ae8aa3cb1209842cb6d352a5a
Author: Mike Wasserman <msw@chromium.org>
Date: Wed Aug 15 21:06:59 2018

Disable the Keyboard Shortcut Viewer App by default on M69

There are too many known defects with the shortcut viewer app.
See the main bug for launching the app at  crbug.com/841020 

Bug:  841020 
Test: KSV (Ctrl-Alt-/) doesn't run as an app by default on M69.
Change-Id: Iae0b9e81e1cbb648740f3c1c23a8f182adac6e32
Reviewed-on: https://chromium-review.googlesource.com/1169541
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#658}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/0b2d84d92648b87ae8aa3cb1209842cb6d352a5a/ash/public/cpp/ash_features.cc

Labels: M-69 Merge-Without-Approval
Revision 0b2d84d92648b87ae8aa3cb1209842cb6d352a5a was merged to refs/branch-heads/3497 branch with no merge approval from a TPM! 

Please explain why this change was merged to the branch!
 
Sorry, I didn't realize that was necessary for landing directly on branches.
(though I do realize that merging from ToT to release branches requires merge requests, so I guess this should too...)

We have been coordinating with dxie@ and others, see the email chain that I just looped you in on.
The CL was needed to postpone the keyboard shortcut viewer mojo app launching codepath from M-69 to M-70.
Please let me know if there are any steps I should take from this point.

Please also consider mentioning TPM approval requirements in these docs:
  http://www.chromium.org/developers/how-tos/get-the-code/working-with-release-branches
Labels: -M-69 -Merge-Without-Approval -merge-merged-3497
Please let me know if there are any remaining issues with the KSV app.
Everything I know about is fixed after ToT @ #584870.
A testing pass is appreciated once #584870 is on Dev.
See Issue 875103 for the launch bug
Status: Fixed (was: Started)
This tracking bug can be marked fixed, new issues may be filed separately.
Project Member

Comment 72 by bugdroid1@chromium.org, Jan 4

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

commit 036f927b641d4c3b83c1a6e0a50d248fb1461dcc
Author: Mike Wasserman <msw@chromium.org>
Date: Fri Jan 04 16:41:26 2019

ksv: Remove flag to control launching KSV as an app

The Keyboard Shortcut Viewer app has been on by default since M-70.
The deprecated codepath to run synchronously in Ash is no longer needed.
Remove the flag and cleanup.

Bug:  841020 
Test: KSV app (Ctrl-Alt-/) works well enough, flag no longer exists.
Change-Id: I1c156705acbd1a5d9a5d5cc3beda50a3d1c4ed26
Reviewed-on: https://chromium-review.googlesource.com/c/1394994
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619970}
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/ash/public/cpp/ash_features.cc
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/ash/public/cpp/ash_features.h
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/about_flags.cc
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/flag-metadata.json
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/ui/ash/ksv/DEPS
[modify] https://crrev.com/036f927b641d4c3b83c1a6e0a50d248fb1461dcc/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_util.cc

Sign in to add a comment